This patch modifies some aspects of GCAPI in support of the new Chrome reactivation functions.

This patch modifies some aspects of GCAPI in support of the new Chrome reactivation functions.

gcapi.cc:  CanOfferReactivation returns TRUE whenever it's invoked from an elevated shell
gcapi.h:  modify LaunchGoogleChrome function pointer to work with GetProcAddress when using run-time dynamic linking
gcapi_reactivation.cc:  don't reactivate Chrome if it's already been reactivated, regardless of what the reactivation brandcode was

TEST=Invoke CanOfferReactivation from an elevated shell.  Try reactivating Chrome with a different brandcode.  Call LaunchGoogleChrome with GetProcAddress.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=129129

[robertshield, 2012-03-15 03:52:32]

Looking pretty good, a few comments below:

https://chromiumcodereview.appspot.com/9704022/diff/1/chrome/installer/gcapi/...
File chrome/installer/gcapi/gcapi.cc (right):

https://chromiumcodereview.appspot.com/9704022/diff/1/chrome/installer/gcapi/...
chrome/installer/gcapi/gcapi.cc:570: if (shell_mode ==
GCAPI_INVOKED_UAC_ELEVATION)
If a partner mistakenly invokes this method while in elevated mode, they will
now always think they can offer reactivation. Is this desired?

https://chromiumcodereview.appspot.com/9704022/diff/1/chrome/installer/gcapi/...
chrome/installer/gcapi/gcapi.cc:617: DWORD* error_code) {
If we are short-circuiting CanOfferReactivation when shell_mode is
GCAPI_INVOKED_UAC_ELEVATION, shouldn't we abort early here too?

https://chromiumcodereview.appspot.com/9704022/diff/1/chrome/installer/gcapi/...
File chrome/installer/gcapi/gcapi.def (right):

https://chromiumcodereview.appspot.com/9704022/diff/1/chrome/installer/gcapi/...
chrome/installer/gcapi/gcapi.def:5: ; to maintain parity with the old gcapi
code. LaunchGoogleChrome was
If we're deleting the ordinals then this comment block can be deleted too.

https://chromiumcodereview.appspot.com/9704022/diff/1/chrome/installer/gcapi/...
File chrome/installer/gcapi/gcapi.h (right):

https://chromiumcodereview.appspot.com/9704022/diff/1/chrome/installer/gcapi/...
chrome/installer/gcapi/gcapi.h:109: typedef BOOL (__stdcall
*GCCC_LaunchGCWithDimensions)(int, int, int, int, bool);
good catch!

https://chromiumcodereview.appspot.com/9704022/diff/1/chrome/installer/gcapi/...
File chrome/installer/gcapi/gcapi_reactivation.cc (right):

https://chromiumcodereview.appspot.com/9704022/diff/1/chrome/installer/gcapi/...
chrome/installer/gcapi/gcapi_reactivation.cc:28: bool
HasBeenReactivatedByBrandCodes(
change this method's name to "HasBeenReactivated" and remove the brand_codes
parameter.

https://chromiumcodereview.appspot.com/9704022/diff/1/chrome/installer/gcapi/...
File chrome/installer/gcapi/gcapi_reactivation_test.cc (left):

https://chromiumcodereview.appspot.com/9704022/diff/1/chrome/installer/gcapi/...
chrome/installer/gcapi/gcapi_reactivation_test.cc:130:
EXPECT_TRUE(HasBeenReactivatedByBrandCodes(check_codes));
update this test to use the new HasBeenReactivated function.

[mgraboski, 2012-03-22 00:33:25]

Hi Robert,

I've updated the GCAPI patch according to your feedback.  Sorry for the delay in
getting these back to you.  

Thanks!
Mike

https://chromiumcodereview.appspot.com/9704022/diff/1/chrome/installer/gcapi/...
File chrome/installer/gcapi/gcapi.cc (right):

https://chromiumcodereview.appspot.com/9704022/diff/1/chrome/installer/gcapi/...
chrome/installer/gcapi/gcapi.cc:570: if (shell_mode ==
GCAPI_INVOKED_UAC_ELEVATION)
Rather than always returning true, I've modified this function to now only run
the code that checks HKCU values if running from a non-elevated shell.

https://chromiumcodereview.appspot.com/9704022/diff/1/chrome/installer/gcapi/...
chrome/installer/gcapi/gcapi.cc:617: DWORD* error_code) {
We need to run this function in non-elevated mode, to write the
reactivationbrand and Omaha experiment label to HKCU; and in elevated mode, to
write the Omaha experiment label to HKLM.

https://chromiumcodereview.appspot.com/9704022/diff/1/chrome/installer/gcapi/...
File chrome/installer/gcapi/gcapi.def (right):

https://chromiumcodereview.appspot.com/9704022/diff/1/chrome/installer/gcapi/...
chrome/installer/gcapi/gcapi.def:5: ; to maintain parity with the old gcapi
code. LaunchGoogleChrome was
On 2012/03/15 03:52:32, robertshield wrote:
> If we're deleting the ordinals then this comment block can be deleted too.

Done.

https://chromiumcodereview.appspot.com/9704022/diff/1/chrome/installer/gcapi/...
File chrome/installer/gcapi/gcapi_reactivation.cc (right):

https://chromiumcodereview.appspot.com/9704022/diff/1/chrome/installer/gcapi/...
chrome/installer/gcapi/gcapi_reactivation.cc:28: bool
HasBeenReactivatedByBrandCodes(
On 2012/03/15 03:52:32, robertshield wrote:
> change this method's name to "HasBeenReactivated" and remove the brand_codes
> parameter.

Done.

https://chromiumcodereview.appspot.com/9704022/diff/1/chrome/installer/gcapi/...
File chrome/installer/gcapi/gcapi_reactivation_test.cc (left):

https://chromiumcodereview.appspot.com/9704022/diff/1/chrome/installer/gcapi/...
chrome/installer/gcapi/gcapi_reactivation_test.cc:130:
EXPECT_TRUE(HasBeenReactivatedByBrandCodes(check_codes));
On 2012/03/15 03:52:32, robertshield wrote:
> update this test to use the new HasBeenReactivated function.

Done.

[robertshield, 2012-03-23 17:52:16]

http://codereview.chromium.org/9704022/diff/1/chrome/installer/gcapi/gcapi.cc
File chrome/installer/gcapi/gcapi.cc (right):

http://codereview.chromium.org/9704022/diff/1/chrome/installer/gcapi/gcapi.cc...
chrome/installer/gcapi/gcapi.cc:570: if (shell_mode ==
GCAPI_INVOKED_UAC_ELEVATION)
On 2012/03/22 00:33:25, mgraboski wrote:
> Rather than always returning true, I've modified this function to now only run
> the code that checks HKCU values if running from a non-elevated shell.

Looks good!

http://codereview.chromium.org/9704022/diff/9001/chrome/installer/gcapi/gcapi.cc
File chrome/installer/gcapi/gcapi.cc (right):

http://codereview.chromium.org/9704022/diff/9001/chrome/installer/gcapi/gcapi...
chrome/installer/gcapi/gcapi.cc:586: bool has_user_install =
IsChromeInstalled(HKEY_CURRENT_USER);
for brevity, consider deleting these temp variables and placing the
IsChromeInstalled() calls directly in the if statement below.

http://codereview.chromium.org/9704022/diff/9001/chrome/installer/gcapi/gcapi...
chrome/installer/gcapi/gcapi.cc:600: }
you can delete the above 7 lines if reactivation_brands is no longer used.

http://codereview.chromium.org/9704022/diff/9001/chrome/installer/gcapi/gcapi...
File chrome/installer/gcapi/gcapi_reactivation.cc (right):

http://codereview.chromium.org/9704022/diff/9001/chrome/installer/gcapi/gcapi...
chrome/installer/gcapi/gcapi_reactivation.cc:29: bool success = false;
delete this line

http://codereview.chromium.org/9704022/diff/9001/chrome/installer/gcapi/gcapi...
chrome/installer/gcapi/gcapi_reactivation.cc:38: return success;
change the above four lines to:
return reactivation_key.Valid();

http://codereview.chromium.org/9704022/diff/9001/chrome/installer/gcapi/gcapi...
File chrome/installer/gcapi/gcapi_reactivation_test.cc (right):

http://codereview.chromium.org/9704022/diff/9001/chrome/installer/gcapi/gcapi...
chrome/installer/gcapi/gcapi_reactivation_test.cc:128: std::vector<std::wstring>
check_codes;
you can delete check_codes and just have a single 
EXPECT_TRUE(HasBeenReactivated());

[mgraboski, 2012-03-23 21:40:35]

Hi Robert,

I have applied your latest review suggestions to the GCAPI files.

Thanks,
Mike

http://codereview.chromium.org/9704022/diff/9001/chrome/installer/gcapi/gcapi.cc
File chrome/installer/gcapi/gcapi.cc (right):

http://codereview.chromium.org/9704022/diff/9001/chrome/installer/gcapi/gcapi...
chrome/installer/gcapi/gcapi.cc:586: bool has_user_install =
IsChromeInstalled(HKEY_CURRENT_USER);
On 2012/03/23 17:52:16, robertshield wrote:
> for brevity, consider deleting these temp variables and placing the
> IsChromeInstalled() calls directly in the if statement below.

Done.

http://codereview.chromium.org/9704022/diff/9001/chrome/installer/gcapi/gcapi...
chrome/installer/gcapi/gcapi.cc:600: }
On 2012/03/23 17:52:16, robertshield wrote:
> you can delete the above 7 lines if reactivation_brands is no longer used.

Done.

http://codereview.chromium.org/9704022/diff/9001/chrome/installer/gcapi/gcapi...
File chrome/installer/gcapi/gcapi_reactivation.cc (right):

http://codereview.chromium.org/9704022/diff/9001/chrome/installer/gcapi/gcapi...
chrome/installer/gcapi/gcapi_reactivation.cc:29: bool success = false;
On 2012/03/23 17:52:16, robertshield wrote:
> delete this line

Done.

http://codereview.chromium.org/9704022/diff/9001/chrome/installer/gcapi/gcapi...
chrome/installer/gcapi/gcapi_reactivation.cc:38: return success;
On 2012/03/23 17:52:16, robertshield wrote:
> change the above four lines to:
> return reactivation_key.Valid();

Done.

http://codereview.chromium.org/9704022/diff/9001/chrome/installer/gcapi/gcapi...
File chrome/installer/gcapi/gcapi_reactivation_test.cc (right):

http://codereview.chromium.org/9704022/diff/9001/chrome/installer/gcapi/gcapi...
chrome/installer/gcapi/gcapi_reactivation_test.cc:128: std::vector<std::wstring>
check_codes;
On 2012/03/23 17:52:16, robertshield wrote:
> you can delete check_codes and just have a single 
> EXPECT_TRUE(HasBeenReactivated());

Done.

[mgraboski, 2012-03-27 00:03:55]

Uploaded patch set 5 to remove remaining references to "previous brand codes"
from CanOfferReactivation and ReactivateChrome.

[robertshield, 2012-03-27 00:12:55]

lgtm

[commit-bot: I haz the power, 2012-03-27 00:15:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgraboski@google.com/9704022/25001

[commit-bot: I haz the power, 2012-03-27 00:44:56]

Try job failure for 9704022-25001 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-03-27 02:38:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgraboski@google.com/9704022/25003

[commit-bot: I haz the power, 2012-03-27 04:14:43]

Change committed as 129129