Add a self-destruct mechanism to user-level Chrome Frame when it detects that system-level Chrome...

Add a self-destruct mechanism to user-level Chrome Frame when it detects that system-level Chrome Frame is present. This is a first step to handling user-to-system-level handoff.

It will cause a user-level install to be correctly replaced with a system-level one once the user logs out and back in again. Additional changelists
will follow that:

1) Ensure full clean-up of the user-level installation.
2) Handle notifications such that logging out and in again isn't required.

BUG=82816
TEST=Install user-level Chrome Frame. 
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=87033

[robertshield, 2011-05-24 22:24:18]

Just to get the basic approach reviewed, I figured I'd ask you to take a look at
this without the reg change notification handling. 

I should be able to add the RegNotifyChangeKey stuff in a fairly independent way
either here or in another CL.

Please let me know what you think,
Robert

[amit, 2011-05-24 22:36:50]

lgtm

A combination of RegNotifyChangeKeyValue and RegisterWaitForSingleObject should
be equally simple to implement :)

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
File chrome_frame/chrome_frame_helper_main.cc (right):

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_main.cc:180: std::wstring args(L"\"");
nit: size concern - if we are using std::string just for this and not anywhere
else then a large enough buffer and sprintf variant should be ok?

[grt (UTC plus 2), 2011-05-25 16:10:06]

This is pretty elegant looking.  Nice work.  It looks like the TEST field for
the CL is missing some details.  Would you mind adding a bit to it?  Thanks.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/bho_loader.cc
File chrome_frame/bho_loader.cc (right):

http://codereview.chromium.org/7065024/diff/1/chrome_frame/bho_loader.cc#newc...
chrome_frame/bho_loader.cc:1: // Copyright (c) 2010 The Chromium Authors. All
rights reserved.
2011

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
File chrome_frame/chrome_frame_helper_main.cc (right):

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_main.cc:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
2011

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_main.cc:21: #include
"chrome_frame/chrome_frame_helper_util.h"
move this above the previous line

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_main.cc:23: // Window class and window names.
nit: wrap as much of this as reasonable in namespace{}

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_main.cc:31:
L"{8BA986DA-5100-405E-AA35-86F34A02ACBF}\\";
nit: I don't think we usually have a trailing separator in these sorts of
strings.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_main.cc:158: KEY_READ,
KEY_READ -> KEY_QUERY_VALUE

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_main.cc:171: 
Consider moving RegCloseKey(reg_handle); reg_handle = NULL; here.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_main.cc:180: std::wstring args(L"\"");
On 2011/05/24 22:36:50, amit wrote:
> nit: size concern - if we are using std::string just for this and not anywhere
> else then a large enough buffer and sprintf variant should be ok?

Agreed.  The rest of the code seems to bend over to not use std::basic_string. 
May as well do the same here.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
File chrome_frame/chrome_frame_helper_util.cc (right):

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_util.cc:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
2011

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_util.cc:11: const wchar_t kGetBrowserMessage[]
= L"GetAutomationObject";
nit: wrap as much of these as reasonable in namespace {}

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_util.cc:13: const wchar_t kSystemLevelRegPath[]
= L"Software\\Classes\\CLSID\\%s";
Consider putting "Format" or "Fmt" or something in the name of this constant so
that it's clear that it's a format string.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_util.cc:14: const wchar_t
kChromeFrameClientKey[] =
Same comment about trailing separator.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_util.cc:17: const wchar_t
kChromeFrameVersionValue[] = L"pv";
Hmm.  This is a Google Update value, not a Chrome Frame value.  How about
kGoogleUpdateVersionValue?

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_util.cc:197: 
remove blank line

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_util.cc:242: KEY_READ,
KEY_READ -> KEY_QUERY_VALUE

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_util.cc:251: // If our bho is registered, also
check that we appear to have a valid
Under what circumstances would the BHO be registered but CF not have a "pv"?  Is
it correct to act as if the BHO wasn't there in this case?

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_util.cc:262: wchar_t version_buffer[MAX_PATH] =
{0};
Consider giving the bho_clsid_as_string buffer a more general name and reusing
it here.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_util.cc:267: if (result == ERROR_SUCCESS &&
wcslen(version_buffer) > 0) {
wcslen(version_buffer) > 0  ->  *version_buffer != L'\0'

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_util.cc:269: }
add RegCloseKey(reg_handle); after this line

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
File chrome_frame/chrome_frame_helper_util.h (right):

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_util.h:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
2011

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_launc...
File chrome_frame/chrome_frame_launcher.gyp (right):

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_launc...
chrome_frame/chrome_frame_launcher.gyp:1: # Copyright (c) 2010 The Chromium
Authors. All rights reserved.
2011

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_launc...
chrome_frame/chrome_frame_launcher.gyp:193: 'msvs_settings': {
This looks like copy-n-paste that doesn't belong.  At the very least, the
OutputFile and ProgramDatabaseFile settings are bad.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_tab.cc
File chrome_frame/chrome_tab.cc (right):

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_tab.cc#newc...
chrome_frame/chrome_tab.cc:379: std::wstring
helper_path_cmd(helper_path.value());
I think it's best to quote the path.  Unless, of course, there's some gnarly
reason why Windows will barf on a quoted exe path in this one context.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_tab.cc#newc...
chrome_frame/chrome_tab.cc:381: helper_path_cmd +=
kChromeFrameHelperFirstRunArg;
Why not chrome::switches::kFirstRun?

[robertshield, 2011-05-26 15:14:28]

Thanks for the comments. I added a registry change notification watcher that
observes when the BHO load policy is created by a system-level install. 

Please take another look.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/bho_loader.cc
File chrome_frame/bho_loader.cc (right):

http://codereview.chromium.org/7065024/diff/1/chrome_frame/bho_loader.cc#newc...
chrome_frame/bho_loader.cc:1: // Copyright (c) 2010 The Chromium Authors. All
rights reserved.
On 2011/05/25 16:10:07, grt wrote:
> 2011

Copyright year nits.. grumble.. :)

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
File chrome_frame/chrome_frame_helper_main.cc (right):

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_main.cc:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
On 2011/05/25 16:10:07, grt wrote:
> 2011

Done.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_main.cc:21: #include
"chrome_frame/chrome_frame_helper_util.h"
On 2011/05/25 16:10:07, grt wrote:
> move this above the previous line

Done.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_main.cc:23: // Window class and window names.
On 2011/05/25 16:10:07, grt wrote:
> nit: wrap as much of this as reasonable in namespace{}

Done.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_main.cc:31:
L"{8BA986DA-5100-405E-AA35-86F34A02ACBF}\\";
On 2011/05/25 16:10:07, grt wrote:
> nit: I don't think we usually have a trailing separator in these sorts of
> strings.

Done.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_main.cc:158: KEY_READ,
On 2011/05/25 16:10:07, grt wrote:
> KEY_READ -> KEY_QUERY_VALUE

Done.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_main.cc:171: 
On 2011/05/25 16:10:07, grt wrote:
> Consider moving RegCloseKey(reg_handle); reg_handle = NULL; here.

Done.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_main.cc:180: std::wstring args(L"\"");
On 2011/05/24 22:36:50, amit wrote:
> nit: size concern - if we are using std::string just for this and not anywhere
> else then a large enough buffer and sprintf variant should be ok?

Done.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_main.cc:180: std::wstring args(L"\"");
On 2011/05/25 16:10:07, grt wrote:
> On 2011/05/24 22:36:50, amit wrote:
> > nit: size concern - if we are using std::string just for this and not
anywhere
> > else then a large enough buffer and sprintf variant should be ok?
> 
> Agreed.  The rest of the code seems to bend over to not use std::basic_string.

> May as well do the same here.

Done.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
File chrome_frame/chrome_frame_helper_util.cc (right):

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_util.cc:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
On 2011/05/25 16:10:07, grt wrote:
> 2011

This is because of the beeping UPS, isn't it?

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_util.cc:11: const wchar_t kGetBrowserMessage[]
= L"GetAutomationObject";
On 2011/05/25 16:10:07, grt wrote:
> nit: wrap as much of these as reasonable in namespace {}

Done.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_util.cc:13: const wchar_t kSystemLevelRegPath[]
= L"Software\\Classes\\CLSID\\%s";
On 2011/05/25 16:10:07, grt wrote:
> Consider putting "Format" or "Fmt" or something in the name of this constant
so
> that it's clear that it's a format string.

Done.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_util.cc:14: const wchar_t
kChromeFrameClientKey[] =
On 2011/05/25 16:10:07, grt wrote:
> Same comment about trailing separator.

Done.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_util.cc:17: const wchar_t
kChromeFrameVersionValue[] = L"pv";
On 2011/05/25 16:10:07, grt wrote:
> Hmm.  This is a Google Update value, not a Chrome Frame value.  How about
> kGoogleUpdateVersionValue?

Done.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_util.cc:197: 
On 2011/05/25 16:10:07, grt wrote:
> remove blank line

Done.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_util.cc:242: KEY_READ,
On 2011/05/25 16:10:07, grt wrote:
> KEY_READ -> KEY_QUERY_VALUE

Done.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_util.cc:251: // If our bho is registered, also
check that we appear to have a valid
On 2011/05/25 16:10:07, grt wrote:
> Under what circumstances would the BHO be registered but CF not have a "pv"? 
Is
> it correct to act as if the BHO wasn't there in this case?


As discussed, this function is now split in two parts, one which verifies the
BHO loading policy and the other that checks for a valid CF system-level
install.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_util.cc:262: wchar_t version_buffer[MAX_PATH] =
{0};
On 2011/05/25 16:10:07, grt wrote:
> Consider giving the bho_clsid_as_string buffer a more general name and reusing
> it here.

I split the function in twine to be more specific, so can't no more.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_util.cc:267: if (result == ERROR_SUCCESS &&
wcslen(version_buffer) > 0) {
On 2011/05/25 16:10:07, grt wrote:
> wcslen(version_buffer) > 0  ->  *version_buffer != L'\0'

Neato.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_util.cc:269: }
On 2011/05/25 16:10:07, grt wrote:
> add RegCloseKey(reg_handle); after this line

Done.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
File chrome_frame/chrome_frame_helper_util.h (right):

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_util.h:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
On 2011/05/25 16:10:07, grt wrote:
> 2011

From hell's heart I stab at thee!

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_launc...
File chrome_frame/chrome_frame_launcher.gyp (right):

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_launc...
chrome_frame/chrome_frame_launcher.gyp:1: # Copyright (c) 2010 The Chromium
Authors. All rights reserved.
On 2011/05/25 16:10:07, grt wrote:
> 2011

Done.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_launc...
chrome_frame/chrome_frame_launcher.gyp:193: 'msvs_settings': {
On 2011/05/25 16:10:07, grt wrote:
> This looks like copy-n-paste that doesn't belong.  At the very least, the
> OutputFile and ProgramDatabaseFile settings are bad.

Oops, removed those. Still need the shlwapi dep though.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_tab.cc
File chrome_frame/chrome_tab.cc (right):

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_tab.cc#newc...
chrome_frame/chrome_tab.cc:379: std::wstring
helper_path_cmd(helper_path.value());
On 2011/05/25 16:10:07, grt wrote:
> I think it's best to quote the path.  Unless, of course, there's some gnarly
> reason why Windows will barf on a quoted exe path in this one context.

Done.

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_tab.cc#newc...
chrome_frame/chrome_tab.cc:381: helper_path_cmd +=
kChromeFrameHelperFirstRunArg;
On 2011/05/25 16:10:07, grt wrote:
> Why not chrome::switches::kFirstRun?

they do different things, sorry I hadn't uploaded the last edit that included
the string change.

[grt (UTC plus 2), 2011-05-26 17:08:58]

Change ALL the copyright years!

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
File chrome_frame/chrome_frame_helper_util.cc (right):

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_util.cc:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
On 2011/05/26 15:14:28, robertshield wrote:
> On 2011/05/25 16:10:07, grt wrote:
> > 2011
> 
> This is because of the beeping UPS, isn't it?

Are you making a statement by not fixing this one?  :-)

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/chrome_frame_h...
File chrome_frame/chrome_frame_helper_main.cc (right):

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/chrome_frame_h...
chrome_frame/chrome_frame_helper_main.cc:24: // Window class and window names.
nit: newline before this

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/chrome_frame_h...
chrome_frame/chrome_frame_helper_main.cc:195: _snwprintf(argument_string,
int argument_string_len = _snwprintf(...);
if (argument_string_len > 0 && CreateProcess(...)) {...

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/chrome_frame_h...
chrome_frame/chrome_frame_helper_main.cc:196: MAX_PATH * 3,
_countof(argument_string) would be more safe, no? (include <stdlib.h> if so)

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/chrome_frame_h...
chrome_frame/chrome_frame_helper_main.cc:223: // Post message.
This comment adds nothing.  Please either make it meaningful or remove it.

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/chrome_frame_h...
File chrome_frame/chrome_frame_helper_util.cc (right):

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/chrome_frame_h...
chrome_frame/chrome_frame_helper_util.cc:12: const wchar_t kGetBrowserMessage[]
= L"GetAutomationObject";
nit: add a blank line before this (and before the end of the namespace.

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/chrome_frame_h...
chrome_frame/chrome_frame_helper_util.cc:271: if (result == ERROR_SUCCESS &&
*version_buffer != L'\0') {
gak!  make that version_buffer[0] != L'\0' since version_buffer is an array
rather than a pointer.  apologies for the initially ugly suggestion.

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/chrome_frame_h...
File chrome_frame/chrome_frame_helper_util.h (right):

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/chrome_frame_h...
chrome_frame/chrome_frame_helper_util.h:84: // Returns true if system-level
Chrome Frame is installed and the BHO is
This should be simply "Returns true if system-level Chrome Frame is installed." 
no?

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/chrome_frame_l...
File chrome_frame/chrome_frame_launcher.gyp (right):

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/chrome_frame_l...
chrome_frame/chrome_frame_launcher.gyp:198: 'SubSystem': '2',
I'm not sure why this is needed; the linker isn't used to build a static lib, is
it?

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/registry_watch...
File chrome_frame/registry_watcher.cc (right):

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/registry_watch...
chrome_frame/registry_watcher.cc:67: }
 else { CloseHandle(wait_event_); wait_event_ = NULL; }
so that a failed call to StartWatching leaves the instance in the same state as
before the call.

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/registry_watch...
File chrome_frame/registry_watcher.h (right):

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/registry_watch...
chrome_frame/registry_watcher.h:16: RegistryWatcher(HKEY hive, const wchar_t*
path, WAITORTIMERCALLBACK callback);
Change this so that users of this class don't need this boilerplate:
  RegistryWatcher* watcher = reinterpret_cast<RegistryWatcher*>(param);
  if (watcher->stopping())
    return;
One way would be to add a private static func that is used for the
WAITORTIMERCALLBACK internally.  This func would do the cast and delegate to the
user's callback func iff !stopping.

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/test/registry_...
File chrome_frame/test/registry_watcher_unittest.cc (right):

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/test/registry_...
chrome_frame/test/registry_watcher_unittest.cc:14: const wchar_t test_root[] =
L"CFRegistryWatcherTest";
kTestRoot

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/test/registry_...
chrome_frame/test/registry_watcher_unittest.cc:26:
temp_key_.Open(HKEY_CURRENT_USER, L"", KEY_ALL_ACCESS);
Don't need all access to delete a subkey.  KEY_QUERY_VALUE will suffice.

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/test/registry_...
chrome_frame/test/registry_watcher_unittest.cc:39:
temp_key_.Open(HKEY_CURRENT_USER, L"", KEY_ALL_ACCESS);
Don't need all access to delete a subkey.  KEY_QUERY_VALUE will suffice.

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/test/registry_...
chrome_frame/test/registry_watcher_unittest.cc:41: 
temp_key_.Close();

[robertshield, 2011-05-27 03:34:59]

Thanks, PTAL

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
File chrome_frame/chrome_frame_helper_util.cc (right):

http://codereview.chromium.org/7065024/diff/1/chrome_frame/chrome_frame_helpe...
chrome_frame/chrome_frame_helper_util.cc:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
On 2011/05/26 17:08:58, grt wrote:
> On 2011/05/26 15:14:28, robertshield wrote:
> > On 2011/05/25 16:10:07, grt wrote:
> > > 2011
> > 
> > This is because of the beeping UPS, isn't it?
> 
> Are you making a statement by not fixing this one?  :-)

Fixed.

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/chrome_frame_h...
File chrome_frame/chrome_frame_helper_main.cc (right):

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/chrome_frame_h...
chrome_frame/chrome_frame_helper_main.cc:24: // Window class and window names.
On 2011/05/26 17:08:58, grt wrote:
> nit: newline before this

Done.

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/chrome_frame_h...
chrome_frame/chrome_frame_helper_main.cc:195: _snwprintf(argument_string,
On 2011/05/26 17:08:58, grt wrote:
> int argument_string_len = _snwprintf(...);
> if (argument_string_len > 0 && CreateProcess(...)) {...

Done.

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/chrome_frame_h...
chrome_frame/chrome_frame_helper_main.cc:196: MAX_PATH * 3,
On 2011/05/26 17:08:58, grt wrote:
> _countof(argument_string) would be more safe, no? (include <stdlib.h> if so)

_countof() - 1, just to ensure that the string remains null-terminated.

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/chrome_frame_h...
chrome_frame/chrome_frame_helper_main.cc:223: // Post message.
On 2011/05/26 17:08:58, grt wrote:
> This comment adds nothing.  Please either make it meaningful or remove it.

Done.

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/chrome_frame_h...
File chrome_frame/chrome_frame_helper_util.cc (right):

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/chrome_frame_h...
chrome_frame/chrome_frame_helper_util.cc:12: const wchar_t kGetBrowserMessage[]
= L"GetAutomationObject";
On 2011/05/26 17:08:58, grt wrote:
> nit: add a blank line before this (and before the end of the namespace.

Done.

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/chrome_frame_h...
chrome_frame/chrome_frame_helper_util.cc:271: if (result == ERROR_SUCCESS &&
*version_buffer != L'\0') {
On 2011/05/26 17:08:58, grt wrote:
> gak!  make that version_buffer[0] != L'\0' since version_buffer is an array
> rather than a pointer.  apologies for the initially ugly suggestion.

Done.

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/chrome_frame_h...
File chrome_frame/chrome_frame_helper_util.h (right):

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/chrome_frame_h...
chrome_frame/chrome_frame_helper_util.h:84: // Returns true if system-level
Chrome Frame is installed and the BHO is
On 2011/05/26 17:08:58, grt wrote:
> This should be simply "Returns true if system-level Chrome Frame is
installed." 
> no?

Stale comment, fixed, thanks.

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/chrome_frame_l...
File chrome_frame/chrome_frame_launcher.gyp (right):

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/chrome_frame_l...
chrome_frame/chrome_frame_launcher.gyp:198: 'SubSystem': '2',
On 2011/05/26 17:08:58, grt wrote:
> I'm not sure why this is needed; the linker isn't used to build a static lib,
is
> it?

Stuff in this static lib calls shlwapi functions, so whatever exe / dll targets
depend on this lib needs to have slwapi.lib somewhere in their linker command
lines. Gyp's exuberant dependency propagation actually seems to take care of
that through a different byzantine path, but I feel it cleaner to leave this
here and it's certainly not harmful. I removed it from the dll target above
which no longer depends on it directly.

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/registry_watch...
File chrome_frame/registry_watcher.cc (right):

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/registry_watch...
chrome_frame/registry_watcher.cc:67: }
On 2011/05/26 17:08:58, grt wrote:
>  else { CloseHandle(wait_event_); wait_event_ = NULL; }
> so that a failed call to StartWatching leaves the instance in the same state
as
> before the call.

Need to do this if either the RegisterWait..() or the RegNotify..() fail so
moved below.

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/registry_watch...
File chrome_frame/registry_watcher.h (right):

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/registry_watch...
chrome_frame/registry_watcher.h:16: RegistryWatcher(HKEY hive, const wchar_t*
path, WAITORTIMERCALLBACK callback);
On 2011/05/26 17:08:58, grt wrote:
> Change this so that users of this class don't need this boilerplate:
>   RegistryWatcher* watcher = reinterpret_cast<RegistryWatcher*>(param);
>   if (watcher->stopping())
>     return;
> One way would be to add a private static func that is used for the
> WAITORTIMERCALLBACK internally.  This func would do the cast and delegate to
the
> user's callback func iff !stopping.

Done.

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/test/registry_...
File chrome_frame/test/registry_watcher_unittest.cc (right):

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/test/registry_...
chrome_frame/test/registry_watcher_unittest.cc:14: const wchar_t test_root[] =
L"CFRegistryWatcherTest";
On 2011/05/26 17:08:58, grt wrote:
> kTestRoot

Done.

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/test/registry_...
chrome_frame/test/registry_watcher_unittest.cc:26:
temp_key_.Open(HKEY_CURRENT_USER, L"", KEY_ALL_ACCESS);
On 2011/05/26 17:08:58, grt wrote:
> Don't need all access to delete a subkey.  KEY_QUERY_VALUE will suffice.

Done.

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/test/registry_...
chrome_frame/test/registry_watcher_unittest.cc:39:
temp_key_.Open(HKEY_CURRENT_USER, L"", KEY_ALL_ACCESS);
On 2011/05/26 17:08:58, grt wrote:
> Don't need all access to delete a subkey.  KEY_QUERY_VALUE will suffice.

Done.

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/test/registry_...
chrome_frame/test/registry_watcher_unittest.cc:41: 
On 2011/05/26 17:08:58, grt wrote:
> temp_key_.Close();

Done.

[grt (UTC plus 2), 2011-05-27 14:07:58]

Very nice!

LGTM with one last nit about the .gyp file.

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/chrome_frame_h...
File chrome_frame/chrome_frame_helper_main.cc (right):

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/chrome_frame_h...
chrome_frame/chrome_frame_helper_main.cc:196: MAX_PATH * 3,
On 2011/05/27 03:34:59, robertshield wrote:
> On 2011/05/26 17:08:58, grt wrote:
> > _countof(argument_string) would be more safe, no? (include <stdlib.h> if so)
> 
> _countof() - 1, just to ensure that the string remains null-terminated.

Good call.

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/chrome_frame_l...
File chrome_frame/chrome_frame_launcher.gyp (right):

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/chrome_frame_l...
chrome_frame/chrome_frame_launcher.gyp:198: 'SubSystem': '2',
On 2011/05/27 03:34:59, robertshield wrote:
> On 2011/05/26 17:08:58, grt wrote:
> > I'm not sure why this is needed; the linker isn't used to build a static
lib,
> is
> > it?
> 
> Stuff in this static lib calls shlwapi functions, so whatever exe / dll
targets
> depend on this lib needs to have slwapi.lib somewhere in their linker command
> lines. Gyp's exuberant dependency propagation actually seems to take care of
> that through a different byzantine path, but I feel it cleaner to leave this
> here and it's certainly not harmful. I removed it from the dll target above
> which no longer depends on it directly.

Gotcha.  I think you can nuke the /SUBSYSTEM stuff.  At least, I don't see how
it can be used here.

[robertshield, 2011-05-27 14:29:47]

Thanks!

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/chrome_frame_l...
File chrome_frame/chrome_frame_launcher.gyp (right):

http://codereview.chromium.org/7065024/diff/16001/chrome_frame/chrome_frame_l...
chrome_frame/chrome_frame_launcher.gyp:198: 'SubSystem': '2',
On 2011/05/27 14:07:58, grt wrote:
> On 2011/05/27 03:34:59, robertshield wrote:
> > On 2011/05/26 17:08:58, grt wrote:
> > > I'm not sure why this is needed; the linker isn't used to build a static
> lib,
> > is
> > > it?
> > 
> > Stuff in this static lib calls shlwapi functions, so whatever exe / dll
> targets
> > depend on this lib needs to have slwapi.lib somewhere in their linker
command
> > lines. Gyp's exuberant dependency propagation actually seems to take care of
> > that through a different byzantine path, but I feel it cleaner to leave this
> > here and it's certainly not harmful. I removed it from the dll target above
> > which no longer depends on it directly.
> 
> Gotcha.  I think you can nuke the /SUBSYSTEM stuff.  At least, I don't see how
> it can be used here.

Done.