Introduce a single call to grab default browser on Win8 for tests.

Introduce a single call to grab default browser on Win8 for tests.

Required to avoid duplicating code for upcoming Ash browser_tests on Windows.

Also avoids calling into OpenWithDialogController if the test is already default browser.

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

[gab, 2013-05-27 15:55:11]

Sir, please take a look.

Cheers!
Gab

https://codereview.chromium.org/15755016/diff/4001/win8/test/metro_registrati...
File win8/test/metro_registration_helper.cc (left):

https://codereview.chromium.org/15755016/diff/4001/win8/test/metro_registrati...
win8/test/metro_registration_helper.cc:48:
register_command.AppendSwitchNative(win8::test::kTestAppUserModelId,
test_registrar already uses these as default values if they are not specified:
https://code.google.com/p/chromium/codesearch#chromium/src/win8/test/test_reg...

I determined last week with Robert that it wasn't really worth adding them to
the interface since tests just want to grab default browser and don't care to
pass in a custom app_id.

[grt (UTC plus 2), 2013-05-27 16:11:14]

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

https://codereview.chromium.org/15755016/diff/4001/ash/test/test_suite.cc#new...
ash/test/test_suite.cc:43: base::win::ScopedCOMInitializer com_initializer;
COM shouldn't be initialized and uninitialized like this (see
http://crbug.com/145095 for reference). initialize it once on the thread and
only tear it down at exit.

https://codereview.chromium.org/15755016/diff/4001/win8/test/metro_registrati...
File win8/test/metro_registration_helper.cc (left):

https://codereview.chromium.org/15755016/diff/4001/win8/test/metro_registrati...
win8/test/metro_registration_helper.cc:48:
register_command.AppendSwitchNative(win8::test::kTestAppUserModelId,
On 2013/05/27 15:55:11, gab wrote:
> test_registrar already uses these as default values if they are not specified:
>
https://code.google.com/p/chromium/codesearch#chromium/src/win8/test/test_reg...
> 
> I determined last week with Robert that it wasn't really worth adding them to
> the interface since tests just want to grab default browser and don't care to
> pass in a custom app_id.

Thanks for clarifying. I was just about to ask that. :-)

https://codereview.chromium.org/15755016/diff/4001/win8/test/metro_registrati...
File win8/test/metro_registration_helper.cc (right):

https://codereview.chromium.org/15755016/diff/4001/win8/test/metro_registrati...
win8/test/metro_registration_helper.cc:35: // up in the default browser
selection dialog as |viewer_process_name|. Intended
please update comment (there is no |viewer_process_name| parameter).

https://codereview.chromium.org/15755016/diff/4001/win8/test/metro_registrati...
win8/test/metro_registration_helper.cc:37: // presence of test_registrar.exe, a
viewer process and all needed DLLs in the
nit: comma before " and"

https://codereview.chromium.org/15755016/diff/4001/win8/test/metro_registrati...
win8/test/metro_registration_helper.cc:83: bool IsTestDefaultForProtocol(const
wchar_t protocol[]) {
const wchar_t* protocol

https://codereview.chromium.org/15755016/diff/4001/win8/test/metro_registrati...
win8/test/metro_registration_helper.cc:110: if (!RegisterTestDefaultBrowser()) {
nit: no braces

https://codereview.chromium.org/15755016/diff/4001/win8/test/metro_registrati...
win8/test/metro_registration_helper.cc:120: // be prefixed by "Keep using").
space after "using"? :-)

https://codereview.chromium.org/15755016/diff/4001/win8/test/metro_registrati...
win8/test/metro_registration_helper.cc:124: ui::win::CreateATLModuleIfNeeded();
i don't this this should be done within a helper function like this. ATL modules
should be created by the target into which this helper is being linked (i.e.,
something close to a foo_main.cc or test_suite.cc or similar).

https://codereview.chromium.org/15755016/diff/4001/win8/test/metro_registrati...
File win8/test/metro_registration_helper.h (right):

https://codereview.chromium.org/15755016/diff/4001/win8/test/metro_registrati...
win8/test/metro_registration_helper.h:16: // chrome.exe (the viewer process) and
all needed DLLs in the same directory
nit: comma before " and"

https://codereview.chromium.org/15755016/diff/4001/win8/test/metro_registrati...
win8/test/metro_registration_helper.h:18: // NOTE: COM should be initialized
prior to calling this method.
should be or must be? the latter, i think.

[gab, 2013-05-27 16:47:57]

Thanks, comments addressed, PTAL!

Cheers,
Gab

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

https://codereview.chromium.org/15755016/diff/4001/ash/test/test_suite.cc#new...
ash/test/test_suite.cc:43: base::win::ScopedCOMInitializer com_initializer;
On 2013/05/27 16:11:14, grt wrote:
> COM shouldn't be initialized and uninitialized like this (see
> http://crbug.com/145095 for reference). initialize it once on the thread and
> only tear it down at exit.

I see, moved it as a member of AuraShellTestSuite (and made sure this didn't
result in double COM initialization errors).

https://codereview.chromium.org/15755016/diff/4001/win8/test/metro_registrati...
File win8/test/metro_registration_helper.cc (right):

https://codereview.chromium.org/15755016/diff/4001/win8/test/metro_registrati...
win8/test/metro_registration_helper.cc:35: // up in the default browser
selection dialog as |viewer_process_name|. Intended
On 2013/05/27 16:11:14, grt wrote:
> please update comment (there is no |viewer_process_name| parameter).

Done.

https://codereview.chromium.org/15755016/diff/4001/win8/test/metro_registrati...
win8/test/metro_registration_helper.cc:37: // presence of test_registrar.exe, a
viewer process and all needed DLLs in the
On 2013/05/27 16:11:14, grt wrote:
> nit: comma before " and"

Done.

https://codereview.chromium.org/15755016/diff/4001/win8/test/metro_registrati...
win8/test/metro_registration_helper.cc:83: bool IsTestDefaultForProtocol(const
wchar_t protocol[]) {
On 2013/05/27 16:11:14, grt wrote:
> const wchar_t* protocol

Done.

https://codereview.chromium.org/15755016/diff/4001/win8/test/metro_registrati...
win8/test/metro_registration_helper.cc:110: if (!RegisterTestDefaultBrowser()) {
On 2013/05/27 16:11:14, grt wrote:
> nit: no braces

Done.

https://codereview.chromium.org/15755016/diff/4001/win8/test/metro_registrati...
win8/test/metro_registration_helper.cc:120: // be prefixed by "Keep using").
On 2013/05/27 16:11:14, grt wrote:
> space after "using"? :-)

Done.

https://codereview.chromium.org/15755016/diff/4001/win8/test/metro_registrati...
win8/test/metro_registration_helper.cc:124: ui::win::CreateATLModuleIfNeeded();
On 2013/05/27 16:11:14, grt wrote:
> i don't this this should be done within a helper function like this. ATL
modules
> should be created by the target into which this helper is being linked (i.e.,
> something close to a foo_main.cc or test_suite.cc or similar).

Ok, I'll take your word for it :)!

https://codereview.chromium.org/15755016/diff/4001/win8/test/metro_registrati...
File win8/test/metro_registration_helper.h (right):

https://codereview.chromium.org/15755016/diff/4001/win8/test/metro_registrati...
win8/test/metro_registration_helper.h:16: // chrome.exe (the viewer process) and
all needed DLLs in the same directory
On 2013/05/27 16:11:14, grt wrote:
> nit: comma before " and"

Done.

https://codereview.chromium.org/15755016/diff/4001/win8/test/metro_registrati...
win8/test/metro_registration_helper.h:18: // NOTE: COM should be initialized
prior to calling this method.
On 2013/05/27 16:11:14, grt wrote:
> should be or must be? the latter, i think.

"must" indeed :)

[grt (UTC plus 2), 2013-05-27 18:46:04]

https://codereview.chromium.org/15755016/diff/10001/ash/test/test_suite.h
File ash/test/test_suite.h (right):

https://codereview.chromium.org/15755016/diff/10001/ash/test/test_suite.h#new...
ash/test/test_suite.h:29: base::win::ScopedCOMInitializer com_initializer;
I think it's best to follow the pattern in base/threading/thread.cc and make
this a scoped_ptr<ScopedCOMInitializer> that you instantiate in Initialize() and
reset in Shutdown().

Also add a trailing underscore to the name.

[gab, 2013-05-27 20:22:10]

Done, PTAL.

Thanks!
Gab

https://codereview.chromium.org/15755016/diff/10001/ash/test/test_suite.h
File ash/test/test_suite.h (right):

https://codereview.chromium.org/15755016/diff/10001/ash/test/test_suite.h#new...
ash/test/test_suite.h:29: base::win::ScopedCOMInitializer com_initializer;
On 2013/05/27 18:46:04, grt wrote:
> I think it's best to follow the pattern in base/threading/thread.cc and make
> this a scoped_ptr<ScopedCOMInitializer> that you instantiate in Initialize()
and
> reset in Shutdown().
> 
> Also add a trailing underscore to the name.

Done.

[grt (UTC plus 2), 2013-05-28 01:17:48]

lgtm w/ one tweak.

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

https://codereview.chromium.org/15755016/diff/17002/ash/test/test_suite.cc#ne...
ash/test/test_suite.cc:64: #if defined(OS_WIN)
move this down after the two ui:: cleanups so that Shutdown is a mirror of
Initialize.

[gab, 2013-05-28 13:49:43]

Thanks, done.

@sky for OWNER approval on ash/test/test_suite.[cc|h]

Cheers,
Gab

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

https://codereview.chromium.org/15755016/diff/17002/ash/test/test_suite.cc#ne...
ash/test/test_suite.cc:64: #if defined(OS_WIN)
On 2013/05/28 01:17:48, grt wrote:
> move this down after the two ui:: cleanups so that Shutdown is a mirror of
> Initialize.

Done.

[sky, 2013-05-28 15:41:06]

LGTM

[commit-bot: I haz the power, 2013-05-28 15:57:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/15755016/27001

[commit-bot: I haz the power, 2013-05-28 17:56:10]

Change committed as 202599