Automatic Win8 default browser registration for the ash unittests.

Automatic Win8 default browser registration for the ash unittests. 

Win8 default browser registration code is extracted into a fairly minimal .rgs script with a helper app that is then invoked by the ash unittests.

A full ICommandExecuteImpl implementation turns out to not be needed as the ash tests activate the registered default browser directly by AppUserModelId which avoids the need for
ICommandExecuteImpl and the other stuff in the production delegate_execute.exe.

BUG=154081
TEST=ash_unittests, eventually.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182258

[grt (UTC plus 2), 2013-02-12 15:37:43]

looks really good. mainly comment comments. i'll make another pass after you
factor in these changes.

https://codereview.chromium.org/12096064/diff/25001/ash/test/test_metro_viewe...
File ash/test/test_metro_viewer_process_host.cc (right):

https://codereview.chromium.org/12096064/diff/25001/ash/test/test_metro_viewe...
ash/test/test_metro_viewer_process_host.cc:71: //    at the moment, so a
run-time dependency still exists between this code
If I understand correctly, this code won't have to change if/when something
other than chrome.exe becomes the viewer. If this is correct, I think the
comment about chrome.exe being the viewer process belongs somewhere else (like
whatever would need to be changed when that's no longer the case).

https://codereview.chromium.org/12096064/diff/25001/ash/test/test_metro_viewe...
File ash/test/test_metro_viewer_process_host.h (right):

https://codereview.chromium.org/12096064/diff/25001/ash/test/test_metro_viewe...
ash/test/test_metro_viewer_process_host.h:35: // TODO(robertshield): This
creates a run-time dependency on chrome.exe as the
The dependency is created in the registration rather than here, correct? If so,
please move this part of the comment elsewhere.

https://codereview.chromium.org/12096064/diff/25001/ash/test/test_metro_viewe...
ash/test/test_metro_viewer_process_host.h:38: bool
LaunchImmersiveChromeAndWaitForConnection(
Chrome -> Viewer?

https://codereview.chromium.org/12096064/diff/25001/ash/test/test_suite.cc
File ash/test/test_suite.cc (right):

https://codereview.chromium.org/12096064/diff/25001/ash/test/test_suite.cc#ne...
ash/test/test_suite.cc:47: result = controller.RunSynchronously(NULL, L"http",
L"Test Runner",
Looks like |result| is never used. I propose you not bother capturing it if you
don't use it.

https://codereview.chromium.org/12096064/diff/25001/ash/test/test_suite.cc#ne...
ash/test/test_suite.cc:47: result = controller.RunSynchronously(NULL, L"http",
L"Test Runner",
L"Test Runner" -> kDefaultTestExeName

https://codereview.chromium.org/12096064/diff/25001/win8/test/metro_registrat...
File win8/test/metro_registration_helper.cc (right):

https://codereview.chromium.org/12096064/diff/25001/win8/test/metro_registrat...
win8/test/metro_registration_helper.cc:14: #include
"win8/test/test_registrar_constants.h"
#include "base/win/scoped_handle.h"

https://codereview.chromium.org/12096064/diff/25001/win8/test/metro_registrat...
win8/test/metro_registration_helper.cc:37: LOG(ERROR) << "Could not locate " <<
kChromeExe << " and " << kRegistrar;
and -> and/or

https://codereview.chromium.org/12096064/diff/25001/win8/test/metro_registrat...
win8/test/metro_registration_helper.cc:41: // Perform the registration by
invoking test_delegate_execute.exe with the
update comment

https://codereview.chromium.org/12096064/diff/25001/win8/test/metro_registrat...
win8/test/metro_registration_helper.cc:49: base::ProcessHandle register_handle;
base::win::ScopedHandle register_handle;

https://codereview.chromium.org/12096064/diff/25001/win8/test/metro_registrat...
win8/test/metro_registration_helper.cc:51: &register_handle)) {
register_handle.Receive()

https://codereview.chromium.org/12096064/diff/25001/win8/test/metro_registrat...
win8/test/metro_registration_helper.cc:52: DWORD result =
WaitForSingleObject(register_handle, kRegistrationTimeout);
int ret = 0;
if (base::WaitForExitCodeWithTimeout(register_handle, &ret,
kRegistrationTimeout)) {
  return ret == 0;
}

https://codereview.chromium.org/12096064/diff/25001/win8/test/metro_registrat...
File win8/test/metro_registration_helper.h (right):

https://codereview.chromium.org/12096064/diff/25001/win8/test/metro_registrat...
win8/test/metro_registration_helper.h:14: // test_delegate_execute.exe,
chrome.exe and all needed DLLs in the same
test_delegate_execute.exe doesn't appear to exist anymore. Please update
comment. Also, consider documenting the name under which it is registered since
that's relevant for the setting-of-default.

https://codereview.chromium.org/12096064/diff/25001/win8/win8.gyp
File win8/win8.gyp (right):

https://codereview.chromium.org/12096064/diff/25001/win8/win8.gyp#newcode90
win8/win8.gyp:90: '../base/base.gyp:base',
why?

[robertshield, 2013-02-12 16:57:08]

Thanks! PTAL

https://codereview.chromium.org/12096064/diff/25001/ash/test/test_metro_viewe...
File ash/test/test_metro_viewer_process_host.cc (right):

https://codereview.chromium.org/12096064/diff/25001/ash/test/test_metro_viewe...
ash/test/test_metro_viewer_process_host.cc:71: //    at the moment, so a
run-time dependency still exists between this code
On 2013/02/12 15:37:43, grt wrote:
> If I understand correctly, this code won't have to change if/when something
> other than chrome.exe becomes the viewer. If this is correct, I think the
> comment about chrome.exe being the viewer process belongs somewhere else (like
> whatever would need to be changed when that's no longer the case).

Done.

https://codereview.chromium.org/12096064/diff/25001/ash/test/test_metro_viewe...
File ash/test/test_metro_viewer_process_host.h (right):

https://codereview.chromium.org/12096064/diff/25001/ash/test/test_metro_viewe...
ash/test/test_metro_viewer_process_host.h:35: // TODO(robertshield): This
creates a run-time dependency on chrome.exe as the
On 2013/02/12 15:37:43, grt wrote:
> The dependency is created in the registration rather than here, correct? If
so,
> please move this part of the comment elsewhere.

Done (moved to test_registrar.cc).

https://codereview.chromium.org/12096064/diff/25001/ash/test/test_metro_viewe...
ash/test/test_metro_viewer_process_host.h:38: bool
LaunchImmersiveChromeAndWaitForConnection(
On 2013/02/12 15:37:43, grt wrote:
> Chrome -> Viewer?

Done.

https://codereview.chromium.org/12096064/diff/25001/ash/test/test_suite.cc
File ash/test/test_suite.cc (right):

https://codereview.chromium.org/12096064/diff/25001/ash/test/test_suite.cc#ne...
ash/test/test_suite.cc:47: result = controller.RunSynchronously(NULL, L"http",
L"Test Runner",
On 2013/02/12 15:37:43, grt wrote:
> Looks like |result| is never used. I propose you not bother capturing it if
you
> don't use it.

Done.

https://codereview.chromium.org/12096064/diff/25001/ash/test/test_suite.cc#ne...
ash/test/test_suite.cc:47: result = controller.RunSynchronously(NULL, L"http",
L"Test Runner",
On 2013/02/12 15:37:43, grt wrote:
> L"Test Runner" -> kDefaultTestExeName

Done.

https://codereview.chromium.org/12096064/diff/25001/win8/test/metro_registrat...
File win8/test/metro_registration_helper.cc (right):

https://codereview.chromium.org/12096064/diff/25001/win8/test/metro_registrat...
win8/test/metro_registration_helper.cc:14: #include
"win8/test/test_registrar_constants.h"
On 2013/02/12 15:37:43, grt wrote:
> #include "base/win/scoped_handle.h"

Done.

https://codereview.chromium.org/12096064/diff/25001/win8/test/metro_registrat...
win8/test/metro_registration_helper.cc:37: LOG(ERROR) << "Could not locate " <<
kChromeExe << " and " << kRegistrar;
On 2013/02/12 15:37:43, grt wrote:
> and -> and/or

Done.

https://codereview.chromium.org/12096064/diff/25001/win8/test/metro_registrat...
win8/test/metro_registration_helper.cc:41: // Perform the registration by
invoking test_delegate_execute.exe with the
On 2013/02/12 15:37:43, grt wrote:
> update comment

Done.

https://codereview.chromium.org/12096064/diff/25001/win8/test/metro_registrat...
win8/test/metro_registration_helper.cc:49: base::ProcessHandle register_handle;
On 2013/02/12 15:37:43, grt wrote:
> base::win::ScopedHandle register_handle;

Done.

https://codereview.chromium.org/12096064/diff/25001/win8/test/metro_registrat...
win8/test/metro_registration_helper.cc:51: &register_handle)) {
On 2013/02/12 15:37:43, grt wrote:
> register_handle.Receive()

Done.

https://codereview.chromium.org/12096064/diff/25001/win8/test/metro_registrat...
win8/test/metro_registration_helper.cc:52: DWORD result =
WaitForSingleObject(register_handle, kRegistrationTimeout);
On 2013/02/12 15:37:43, grt wrote:
> int ret = 0;
> if (base::WaitForExitCodeWithTimeout(register_handle, &ret,
> kRegistrationTimeout)) {
>   return ret == 0;
> }

Done.

https://codereview.chromium.org/12096064/diff/25001/win8/test/metro_registrat...
File win8/test/metro_registration_helper.h (right):

https://codereview.chromium.org/12096064/diff/25001/win8/test/metro_registrat...
win8/test/metro_registration_helper.h:14: // test_delegate_execute.exe,
chrome.exe and all needed DLLs in the same
On 2013/02/12 15:37:43, grt wrote:
> test_delegate_execute.exe doesn't appear to exist anymore. Please update
> comment. Also, consider documenting the name under which it is registered
since
> that's relevant for the setting-of-default.

Comment updated. Also, I decided to have this method take in the viewer process
name as then it is more obvious from the calling code that the name is the
meeting point for the registration and the setting-of-default.

https://codereview.chromium.org/12096064/diff/25001/win8/win8.gyp
File win8/win8.gyp (right):

https://codereview.chromium.org/12096064/diff/25001/win8/win8.gyp#newcode90
win8/win8.gyp:90: '../base/base.gyp:base',
On 2013/02/12 15:37:43, grt wrote:
> why?

WHY??!?!?!

Done.

[grt (UTC plus 2), 2013-02-12 18:17:27]

https://codereview.chromium.org/12096064/diff/25020/ash/test/test_suite.cc
File ash/test/test_suite.cc (right):

https://codereview.chromium.org/12096064/diff/25020/ash/test/test_suite.cc#ne...
ash/test/test_suite.cc:46: HRESULT result = S_OK;
remove

https://codereview.chromium.org/12096064/diff/25020/win8/test/metro_registrat...
File win8/test/metro_registration_helper.cc (right):

https://codereview.chromium.org/12096064/diff/25020/win8/test/metro_registrat...
win8/test/metro_registration_helper.cc:50:
register_command.AppendSwitchNative(win8::test::kTestExeName,
i like this change.

https://codereview.chromium.org/12096064/diff/25020/win8/test/metro_registrat...
win8/test/metro_registration_helper.cc:59:
base::TimeDelta::FromMilliseconds(kRegistrationTimeoutMs))) {
how about using seconds rather than milliseconds?

https://codereview.chromium.org/12096064/diff/25020/win8/test/metro_registrat...
win8/test/metro_registration_helper.cc:64: LOG(ERROR) << "Win8 registration
using " << kRegistrar << " timed out.";
consider logging register_command.GetCommandLineString() instead of kRegistrar
since it's a bit more informative. also, the timeout portion of this message is
only relevant if the launch succeeds. consider putting this in an }else{ block
on line 61 and using a different log message for the failure to launch. or just
change the "timed out." to "timed out or failed."

https://codereview.chromium.org/12096064/diff/25020/win8/test/metro_registrat...
File win8/test/metro_registration_helper.h (right):

https://codereview.chromium.org/12096064/diff/25020/win8/test/metro_registrat...
win8/test/metro_registration_helper.h:15: // same directory as the calling
module. The viewer process if then subsequently
if -> is

https://codereview.chromium.org/12096064/diff/25020/win8/test/metro_registrat...
win8/test/metro_registration_helper.h:16: // set as THE default Win8 browser is
activatable using the AppUserModelId in
comma after "browser"?

https://codereview.chromium.org/12096064/diff/25020/win8/win8.gyp
File win8/win8.gyp (right):

https://codereview.chromium.org/12096064/diff/25020/win8/win8.gyp#newcode70
win8/win8.gyp:70: 'dependencies': [
why doesn't this have a build-time dependency on chrome.exe?

[robertshield, 2013-02-12 18:35:34]

thanks, ptal

https://codereview.chromium.org/12096064/diff/25020/ash/test/test_suite.cc
File ash/test/test_suite.cc (right):

https://codereview.chromium.org/12096064/diff/25020/ash/test/test_suite.cc#ne...
ash/test/test_suite.cc:46: HRESULT result = S_OK;
On 2013/02/12 18:17:27, grt wrote:
> remove

Done.

https://codereview.chromium.org/12096064/diff/25020/win8/test/metro_registrat...
File win8/test/metro_registration_helper.cc (right):

https://codereview.chromium.org/12096064/diff/25020/win8/test/metro_registrat...
win8/test/metro_registration_helper.cc:50:
register_command.AppendSwitchNative(win8::test::kTestExeName,
On 2013/02/12 18:17:27, grt wrote:
> i like this change.

thank you.

https://codereview.chromium.org/12096064/diff/25020/win8/test/metro_registrat...
win8/test/metro_registration_helper.cc:59:
base::TimeDelta::FromMilliseconds(kRegistrationTimeoutMs))) {
On 2013/02/12 18:17:27, grt wrote:
> how about using seconds rather than milliseconds?

Done.

https://codereview.chromium.org/12096064/diff/25020/win8/test/metro_registrat...
win8/test/metro_registration_helper.cc:64: LOG(ERROR) << "Win8 registration
using " << kRegistrar << " timed out.";
On 2013/02/12 18:17:27, grt wrote:
> consider logging register_command.GetCommandLineString() instead of kRegistrar
> since it's a bit more informative. also, the timeout portion of this message
is
> only relevant if the launch succeeds. consider putting this in an }else{ block
> on line 61 and using a different log message for the failure to launch. or
just
> change the "timed out." to "timed out or failed."

Done.

https://codereview.chromium.org/12096064/diff/25020/win8/test/metro_registrat...
File win8/test/metro_registration_helper.h (right):

https://codereview.chromium.org/12096064/diff/25020/win8/test/metro_registrat...
win8/test/metro_registration_helper.h:15: // same directory as the calling
module. The viewer process if then subsequently
On 2013/02/12 18:17:27, grt wrote:
> if -> is

Done.

https://codereview.chromium.org/12096064/diff/25020/win8/test/metro_registrat...
win8/test/metro_registration_helper.h:16: // set as THE default Win8 browser is
activatable using the AppUserModelId in
On 2013/02/12 18:17:27, grt wrote:
> comma after "browser"?

Done.

https://codereview.chromium.org/12096064/diff/25020/win8/win8.gyp
File win8/win8.gyp (right):

https://codereview.chromium.org/12096064/diff/25020/win8/win8.gyp#newcode70
win8/win8.gyp:70: 'dependencies': [
On 2013/02/12 18:17:27, grt wrote:
> why doesn't this have a build-time dependency on chrome.exe?

As discussed, now it does.

[grt (UTC plus 2), 2013-02-12 18:55:54]

lgtm w/ nits

https://codereview.chromium.org/12096064/diff/19012/win8/test/metro_registrat...
File win8/test/metro_registration_helper.cc (right):

https://codereview.chromium.org/12096064/diff/19012/win8/test/metro_registrat...
win8/test/metro_registration_helper.cc:71: }
PLOG failure to launch?

https://codereview.chromium.org/12096064/diff/19012/win8/test/test_registrar.cc
File win8/test/test_registrar.cc (right):

https://codereview.chromium.org/12096064/diff/19012/win8/test/test_registrar....
win8/test/test_registrar.cc:130: extern "C" int WINAPI _tWinMain(HINSTANCE ,
HINSTANCE, LPTSTR, int nShowCmd) {
nit: remove space before comma

https://codereview.chromium.org/12096064/diff/19012/win8/test/test_registrar_...
File win8/test/test_registrar_constants.cc (right):

https://codereview.chromium.org/12096064/diff/19012/win8/test/test_registrar_...
win8/test/test_registrar_constants.cc:30: }
}  // namespace test

https://codereview.chromium.org/12096064/diff/19012/win8/test/test_registrar_...
win8/test/test_registrar_constants.cc:31: }
}  // namespace win8

https://codereview.chromium.org/12096064/diff/19012/win8/test/test_registrar_...
File win8/test/test_registrar_constants.h (right):

https://codereview.chromium.org/12096064/diff/19012/win8/test/test_registrar_...
win8/test/test_registrar_constants.h:21: }
}  // namespace test

https://codereview.chromium.org/12096064/diff/19012/win8/test/test_registrar_...
win8/test/test_registrar_constants.h:22: }
}  // namespace win8

[robertshield, 2013-02-12 20:53:09]

Thanks!

https://codereview.chromium.org/12096064/diff/19012/win8/test/metro_registrat...
File win8/test/metro_registration_helper.cc (right):

https://codereview.chromium.org/12096064/diff/19012/win8/test/metro_registrat...
win8/test/metro_registration_helper.cc:71: }
On 2013/02/12 18:55:54, grt wrote:
> PLOG failure to launch?

Done.

https://codereview.chromium.org/12096064/diff/19012/win8/test/test_registrar.cc
File win8/test/test_registrar.cc (right):

https://codereview.chromium.org/12096064/diff/19012/win8/test/test_registrar....
win8/test/test_registrar.cc:130: extern "C" int WINAPI _tWinMain(HINSTANCE ,
HINSTANCE, LPTSTR, int nShowCmd) {
On 2013/02/12 18:55:54, grt wrote:
> nit: remove space before comma

Done.

https://codereview.chromium.org/12096064/diff/19012/win8/test/test_registrar_...
File win8/test/test_registrar_constants.cc (right):

https://codereview.chromium.org/12096064/diff/19012/win8/test/test_registrar_...
win8/test/test_registrar_constants.cc:30: }
On 2013/02/12 18:55:54, grt wrote:
> }  // namespace test

Done.

https://codereview.chromium.org/12096064/diff/19012/win8/test/test_registrar_...
win8/test/test_registrar_constants.cc:31: }
On 2013/02/12 18:55:54, grt wrote:
> }  // namespace win8

Done.

https://codereview.chromium.org/12096064/diff/19012/win8/test/test_registrar_...
File win8/test/test_registrar_constants.h (right):

https://codereview.chromium.org/12096064/diff/19012/win8/test/test_registrar_...
win8/test/test_registrar_constants.h:21: }
On 2013/02/12 18:55:54, grt wrote:
> }  // namespace test

Done.

https://codereview.chromium.org/12096064/diff/19012/win8/test/test_registrar_...
win8/test/test_registrar_constants.h:22: }
On 2013/02/12 18:55:54, grt wrote:
> }  // namespace win8

Done.

[robertshield, 2013-02-12 20:54:12]

+Scott for OWNERS

[sky, 2013-02-12 22:31:42]

LGTM

https://codereview.chromium.org/12096064/diff/18005/ash/test/test_suite.cc
File ash/test/test_suite.cc (right):

https://codereview.chromium.org/12096064/diff/18005/ash/test/test_suite.cc#ne...
ash/test/test_suite.cc:39: CHECK(win8::RegisterTestDefaultBrowser(
ASSERT

[commit-bot: I haz the power, 2013-02-13 15:18:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robertshield@chromium.org/12096064/32001

[commit-bot: I haz the power, 2013-02-13 18:25:44]

Change committed as 182258

[Dirk Pranke, 2013-02-15 20:51:31]

On 2013/02/13 18:25:44, I haz the power (commit-bot) wrote:
> Change committed as 182258

This change breaks the webkit.org win build, by adding a gyp dependency from
win8.gyp -> chrome.gyp (which we don't check out on those bots). The full
dependency chain is webkit_support -> printing -> win8 -> chrome :(.

I've posted a workaround in https://codereview.chromium.org/12207186/ but I'm
not sure what you'll want to do as a long-term fix. Seems bad that win8 would
depend on chrome?

[robertshield, 2013-02-15 22:18:17]

On Fri, Feb 15, 2013 at 3:51 PM, <dpranke@chromium.org> wrote:

> On 2013/02/13 18:25:44, I haz the power (commit-bot) wrote:
>
>> Change committed as 182258
>>
>
> This change breaks the webkit.org win build, by adding a gyp dependency
> from
> win8.gyp -> chrome.gyp (which we don't check out on those bots). The full
> dependency chain is webkit_support -> printing -> win8 -> chrome :(.
>
> I've posted a workaround in
https://codereview.chromium.**org/12207186/<https://codereview.chromium.org/1...
I'm
> not sure what you'll want to do as a long-term fix. Seems bad that win8
> would
> depend on chrome?
>

Ugh, darn. Thanks for the workaround. The long term plan is to have
win8_test provide its own metro viewer process and to not use Chrome.

The workaround looks great, though I'd rather replace it with a real win8
viewer process which I guess I will have to do a bit sooner than I'd hoped.
Oh well.

Thanks again for the fix! :)



>
>
https://chromiumcodereview.**appspot.com/12096064/<https://chromiumcodereview...
>