Base for Ash browser_tests on Win8.

Base for Ash browser_tests on Win8.

For now, although the --ash-browsertests switch will launch browser_tests in Ash in Metro, most tests are still hardcoded to open browsers on the native desktop (HOST_DESKTOP_TYPE_NATIVE) -- follow-up CLs will be required to have all tests use GetActiveDesktop() instead and then add some checks to make sure no browsers are left on the wrong desktop when tests complete.

I'm not making this the default browser_tests on Windows 8 because regular Desktop browser_tests should still be runnable, testable, debuggable, etc. on Win8 machines.

BUG=179830
TEST="browser_tests.exe --ash-browsertests" should launch Ash in Metro for every test, each test should pass and exit (although most tests will no open browsers in Ash itself for now -- no checks is currently added to make sure of that so they should pass).

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

[gab, 2013-05-28 17:23:18]

Sir, please take a look.

Thanks,
Gab

[jam, 2013-05-28 17:35:17]

drive-by: content doesn't know about ash. it seems all this code needs to be in
src/chrome. chrome's browser_tests harness is in
chrome/test/base/in_process_browser_test.cc

[gab, 2013-05-28 22:03:35]

Thanks, moved everything into chrome/.

The only difference in the new patch set as far as behavior is concerned is that
every test will try to make chrome default (the call itself doesn't proceed much
further if the test binary is already the default browser) instead of the test
suite itself doing it only once across all tests; it seems however that
browser_tests are built in such a way that every test initializes the
ChromeTestSuite anyways though so I didn't bother putting it there.

Cheers!
Gab

[jam, 2013-05-28 23:08:13]

On 2013/05/28 22:03:35, gab wrote:
> Thanks, moved everything into chrome/.
> 
> The only difference in the new patch set as far as behavior is concerned is
that
> every test will try to make chrome default (the call itself doesn't proceed
much
> further if the test binary is already the default browser) instead of the test
> suite itself doing it only once across all tests; it seems however that
> browser_tests are built in such a way that every test initializes the
> ChromeTestSuite anyways though so I didn't bother putting it there.

that is odd that the prompt comes up at all. 
test_launcher_utils::PrepareBrowserCommandLineForTests adds
kNoDefaultBrowserCheck which is supposed to disable this.

> 
> Cheers!
> Gab

[gab, 2013-05-28 23:12:11]

On 2013/05/28 23:08:13, jam wrote:
> On 2013/05/28 22:03:35, gab wrote:
> > Thanks, moved everything into chrome/.
> > 
> > The only difference in the new patch set as far as behavior is concerned is
> that
> > every test will try to make chrome default (the call itself doesn't proceed
> much
> > further if the test binary is already the default browser) instead of the
test
> > suite itself doing it only once across all tests; it seems however that
> > browser_tests are built in such a way that every test initializes the
> > ChromeTestSuite anyways though so I didn't bother putting it there.
> 
> that is odd that the prompt comes up at all. 
> test_launcher_utils::PrepareBrowserCommandLineForTests adds
> kNoDefaultBrowserCheck which is supposed to disable this.

Sorry, my description was a little bit brief.

This is referring to the win8::MakeTestDefaultBrowserSynchronously() call
introduced in https://src.chromium.org/viewvc/chrome?view=rev&revision=202599 ,
the test binary must be made default on Windows 8 to be able to launch in Metro.

This however technically only needs to happen once for the entire test suite.

> 
> > 
> > Cheers!
> > Gab

[jam, 2013-05-28 23:14:05]

On 2013/05/28 23:12:11, gab wrote:
> On 2013/05/28 23:08:13, jam wrote:
> > On 2013/05/28 22:03:35, gab wrote:
> > > Thanks, moved everything into chrome/.
> > > 
> > > The only difference in the new patch set as far as behavior is concerned
is
> > that
> > > every test will try to make chrome default (the call itself doesn't
proceed
> > much
> > > further if the test binary is already the default browser) instead of the
> test
> > > suite itself doing it only once across all tests; it seems however that
> > > browser_tests are built in such a way that every test initializes the
> > > ChromeTestSuite anyways though so I didn't bother putting it there.
> > 
> > that is odd that the prompt comes up at all. 
> > test_launcher_utils::PrepareBrowserCommandLineForTests adds
> > kNoDefaultBrowserCheck which is supposed to disable this.
> 
> Sorry, my description was a little bit brief.
> 
> This is referring to the win8::MakeTestDefaultBrowserSynchronously() call
> introduced in https://src.chromium.org/viewvc/chrome?view=rev&revision=202599
,
> the test binary must be made default on Windows 8 to be able to launch in
Metro.
> 
> This however technically only needs to happen once for the entire test suite.

got it. lgtm. you can tweak things for performance later if necessary if this
call is slow :)

[grt (UTC plus 2), 2013-05-29 02:22:51]

https://codereview.chromium.org/16022003/diff/55001/chrome/browser/browser_pr...
File chrome/browser/browser_process_platform_part_aurawin.cc (right):

https://codereview.chromium.org/16022003/diff/55001/chrome/browser/browser_pr...
chrome/browser/browser_process_platform_part_aurawin.cc:38: } else if
(command_line.HasSwitch(switches::kAshBrowserTests)) {
i'm a little uncomfortable with this test-only code being in the product. is
there precedent for this? i guess it's a bit similar to kTestingChannelID. hmm.
please have one of the reviewers in chrome/OWNERS weigh in.

https://codereview.chromium.org/16022003/diff/55001/chrome/test/base/in_proce...
File chrome/test/base/in_process_browser_test.cc (right):

https://codereview.chromium.org/16022003/diff/55001/chrome/test/base/in_proce...
chrome/test/base/in_process_browser_test.cc:127: if (base::win::GetVersion() >=
base::win::VERSION_WIN8 &&
this only needs to happen once per process rather than once per test, right? if
so, it belongs in SetUpTestCase.

https://codereview.chromium.org/16022003/diff/55001/chrome/test/base/in_proce...
chrome/test/base/in_process_browser_test.cc:231: #if defined(OS_WIN) &&
defined(USE_AURA)
likewise, TearDownTestCase

[gab, 2013-05-29 14:52:20]

Partially addressed + comments below.

Cheers,
Gab

https://codereview.chromium.org/16022003/diff/55001/chrome/browser/browser_pr...
File chrome/browser/browser_process_platform_part_aurawin.cc (right):

https://codereview.chromium.org/16022003/diff/55001/chrome/browser/browser_pr...
chrome/browser/browser_process_platform_part_aurawin.cc:38: } else if
(command_line.HasSwitch(switches::kAshBrowserTests)) {
On 2013/05/29 02:22:51, grt wrote:
> i'm a little uncomfortable with this test-only code being in the product. is
> there precedent for this? i guess it's a bit similar to kTestingChannelID.
hmm.
> please have one of the reviewers in chrome/OWNERS weigh in.

Ya, I agree that this is ugly, especially that I also have to make this target
depend on test_registrar_constants (incidentally this week's edition of TotT
talks about avoiding exactly this!)...

How about I add a generic switch to the chrome switches (e.g.
kViewerLaunchViaAppId) which would only be set by browser tests for now when
they find the kAshBrowserTests switch (which could then be a test-only switch
instead of being in chrome_switches)?

A bit more involved, but it feels cleaner, WDYT?

https://codereview.chromium.org/16022003/diff/55001/chrome/test/base/in_proce...
File chrome/test/base/in_process_browser_test.cc (right):

https://codereview.chromium.org/16022003/diff/55001/chrome/test/base/in_proce...
chrome/test/base/in_process_browser_test.cc:127: if (base::win::GetVersion() >=
base::win::VERSION_WIN8 &&
On 2013/05/29 02:22:51, grt wrote:
> this only needs to happen once per process rather than once per test, right?
if
> so, it belongs in SetUpTestCase.

Right, these are static methods though so they can't be overridden per-say, but
they can be shadowed.

It also implies a static LazyInstance for the ScopedCOMInitializer which is
slightly ugly I find.

https://codereview.chromium.org/16022003/diff/55001/chrome/test/base/in_proce...
chrome/test/base/in_process_browser_test.cc:231: #if defined(OS_WIN) &&
defined(USE_AURA)
On 2013/05/29 02:22:51, grt wrote:
> likewise, TearDownTestCase

Done.

[gab, 2013-05-29 14:55:17]

Partially addressed + comments below.

Cheers,
Gab

[grt (UTC plus 2), 2013-05-29 15:51:03]

https://codereview.chromium.org/16022003/diff/55001/chrome/browser/browser_pr...
File chrome/browser/browser_process_platform_part_aurawin.cc (right):

https://codereview.chromium.org/16022003/diff/55001/chrome/browser/browser_pr...
chrome/browser/browser_process_platform_part_aurawin.cc:38: } else if
(command_line.HasSwitch(switches::kAshBrowserTests)) {
On 2013/05/29 14:52:20, gab wrote:
> On 2013/05/29 02:22:51, grt wrote:
> > i'm a little uncomfortable with this test-only code being in the product. is
> > there precedent for this? i guess it's a bit similar to kTestingChannelID.
> hmm.
> > please have one of the reviewers in chrome/OWNERS weigh in.
> 
> Ya, I agree that this is ugly, especially that I also have to make this target
> depend on test_registrar_constants (incidentally this week's edition of TotT
> talks about avoiding exactly this!)...
> 
> How about I add a generic switch to the chrome switches (e.g.
> kViewerLaunchViaAppId) which would only be set by browser tests for now when
> they find the kAshBrowserTests switch (which could then be a test-only switch
> instead of being in chrome_switches)?
> 
> A bit more involved, but it feels cleaner, WDYT?

If I understand, yeah, this seems better. For what it's worth, I just became
aware of kTestCompositor, which lives along with the regular content switches,
so I see that there is precedent for putting handling of test-only switches in
the product. This makes me feel better.

https://codereview.chromium.org/16022003/diff/55001/chrome/test/base/in_proce...
File chrome/test/base/in_process_browser_test.cc (right):

https://codereview.chromium.org/16022003/diff/55001/chrome/test/base/in_proce...
chrome/test/base/in_process_browser_test.cc:127: if (base::win::GetVersion() >=
base::win::VERSION_WIN8 &&
On 2013/05/29 14:52:20, gab wrote:
> On 2013/05/29 02:22:51, grt wrote:
> > this only needs to happen once per process rather than once per test, right?
> if
> > so, it belongs in SetUpTestCase.
> 
> Right, these are static methods though so they can't be overridden per-say,
but
> they can be shadowed.

This is their raison d'être; see
https://code.google.com/p/googletest/wiki/AdvancedGuide#Sharing_Resources_Bet....

> It also implies a static LazyInstance for the ScopedCOMInitializer which is
> slightly ugly I find.

Please use a raw pointer with new/delete.

[gab, 2013-05-29 23:09:42]

Please take another look, see reply below.

Cheers!
Gab

https://codereview.chromium.org/16022003/diff/55001/chrome/browser/browser_pr...
File chrome/browser/browser_process_platform_part_aurawin.cc (right):

https://codereview.chromium.org/16022003/diff/55001/chrome/browser/browser_pr...
chrome/browser/browser_process_platform_part_aurawin.cc:38: } else if
(command_line.HasSwitch(switches::kAshBrowserTests)) {
On 2013/05/29 15:51:03, grt wrote:
> On 2013/05/29 14:52:20, gab wrote:
> > On 2013/05/29 02:22:51, grt wrote:
> > > i'm a little uncomfortable with this test-only code being in the product.
is
> > > there precedent for this? i guess it's a bit similar to kTestingChannelID.
> > hmm.
> > > please have one of the reviewers in chrome/OWNERS weigh in.
> > 
> > Ya, I agree that this is ugly, especially that I also have to make this
target
> > depend on test_registrar_constants (incidentally this week's edition of TotT
> > talks about avoiding exactly this!)...
> > 
> > How about I add a generic switch to the chrome switches (e.g.
> > kViewerLaunchViaAppId) which would only be set by browser tests for now when
> > they find the kAshBrowserTests switch (which could then be a test-only
switch
> > instead of being in chrome_switches)?
> > 
> > A bit more involved, but it feels cleaner, WDYT?
> 
> If I understand, yeah, this seems better. For what it's worth, I just became
> aware of kTestCompositor, which lives along with the regular content switches,
> so I see that there is precedent for putting handling of test-only switches in
> the product. This makes me feel better.

Okay, did that in patch set 6, let me know if you like it.

I don't like checking success with a CHECK() now that this is no longer
"test-only" code though... any better idea?

At least chrome code no longer depends on a test target...

Still having trouble linking though since weirdly enough
test/base/test_switches.cc is linked into 'browser' not 'test_support_common'
where test/base/in_process_browser_test.cc is linked into...

https://codereview.chromium.org/16022003/diff/55001/chrome/test/base/in_proce...
File chrome/test/base/in_process_browser_test.cc (right):

https://codereview.chromium.org/16022003/diff/55001/chrome/test/base/in_proce...
chrome/test/base/in_process_browser_test.cc:127: if (base::win::GetVersion() >=
base::win::VERSION_WIN8 &&
On 2013/05/29 15:51:03, grt wrote:
> On 2013/05/29 14:52:20, gab wrote:
> > On 2013/05/29 02:22:51, grt wrote:
> > > this only needs to happen once per process rather than once per test,
right?
> > if
> > > so, it belongs in SetUpTestCase.
> > 
> > Right, these are static methods though so they can't be overridden per-say,
> but
> > they can be shadowed.
> 
> This is their raison d'être; see
>
https://code.google.com/p/googletest/wiki/AdvancedGuide#Sharing_Resources_Bet....

I see.

> 
> > It also implies a static LazyInstance for the ScopedCOMInitializer which is
> > slightly ugly I find.
> 
> Please use a raw pointer with new/delete.

Done.

[jam, 2013-06-11 20:36:18]

On 2013/05/29 23:09:42, gab wrote:
> Please take another look, see reply below.
> 
> Cheers!
> Gab
> 
>
https://codereview.chromium.org/16022003/diff/55001/chrome/browser/browser_pr...
> File chrome/browser/browser_process_platform_part_aurawin.cc (right):
> 
>
https://codereview.chromium.org/16022003/diff/55001/chrome/browser/browser_pr...
> chrome/browser/browser_process_platform_part_aurawin.cc:38: } else if
> (command_line.HasSwitch(switches::kAshBrowserTests)) {
> On 2013/05/29 15:51:03, grt wrote:
> > On 2013/05/29 14:52:20, gab wrote:
> > > On 2013/05/29 02:22:51, grt wrote:
> > > > i'm a little uncomfortable with this test-only code being in the
product.
> is
> > > > there precedent for this? i guess it's a bit similar to
kTestingChannelID.
> > > hmm.
> > > > please have one of the reviewers in chrome/OWNERS weigh in.
> > > 
> > > Ya, I agree that this is ugly, especially that I also have to make this
> target
> > > depend on test_registrar_constants (incidentally this week's edition of
TotT
> > > talks about avoiding exactly this!)...
> > > 
> > > How about I add a generic switch to the chrome switches (e.g.
> > > kViewerLaunchViaAppId) which would only be set by browser tests for now
when
> > > they find the kAshBrowserTests switch (which could then be a test-only
> switch
> > > instead of being in chrome_switches)?
> > > 
> > > A bit more involved, but it feels cleaner, WDYT?
> > 
> > If I understand, yeah, this seems better. For what it's worth, I just became
> > aware of kTestCompositor, which lives along with the regular content
switches,
> > so I see that there is precedent for putting handling of test-only switches
in
> > the product. This makes me feel better.
> 
> Okay, did that in patch set 6, let me know if you like it.
> 
> I don't like checking success with a CHECK() now that this is no longer
> "test-only" code though... any better idea?
> 
> At least chrome code no longer depends on a test target...
> 
> Still having trouble linking though since weirdly enough
> test/base/test_switches.cc is linked into 'browser' not 'test_support_common'
> where test/base/in_process_browser_test.cc is linked into...
> 
>
https://codereview.chromium.org/16022003/diff/55001/chrome/test/base/in_proce...
> File chrome/test/base/in_process_browser_test.cc (right):
> 
>
https://codereview.chromium.org/16022003/diff/55001/chrome/test/base/in_proce...
> chrome/test/base/in_process_browser_test.cc:127: if (base::win::GetVersion()
>=
> base::win::VERSION_WIN8 &&
> On 2013/05/29 15:51:03, grt wrote:
> > On 2013/05/29 14:52:20, gab wrote:
> > > On 2013/05/29 02:22:51, grt wrote:
> > > > this only needs to happen once per process rather than once per test,
> right?
> > > if
> > > > so, it belongs in SetUpTestCase.
> > > 
> > > Right, these are static methods though so they can't be overridden
per-say,
> > but
> > > they can be shadowed.
> > 
> > This is their raison d'être; see
> >
>
https://code.google.com/p/googletest/wiki/AdvancedGuide#Sharing_Resources_Bet....
> 
> I see.
> 
> > 
> > > It also implies a static LazyInstance for the ScopedCOMInitializer which
is
> > > slightly ugly I find.
> > 
> > Please use a raw pointer with new/delete.
> 
> Done.

[jam, 2013-06-11 20:36:36]

On 2013/05/29 23:09:42, gab wrote:
> Please take another look, see reply below.

who is this aimed at?

[gab, 2013-06-18 19:58:24]

Just back from vacation, grt PTAL.

Thanks!
Gab

[gab, 2013-06-20 21:56:49]

@grt: ping

Thanks,
Gab

[grt (UTC plus 2), 2013-06-20 23:10:06]

lgtm with the modifications below. thanks!

https://codereview.chromium.org/16022003/diff/120001/chrome/test/base/in_proc...
File chrome/test/base/in_process_browser_test.cc (right):

https://codereview.chromium.org/16022003/diff/120001/chrome/test/base/in_proc...
chrome/test/base/in_process_browser_test.cc:123: delete com_initializer_;
com_initializer_ = NULL; after calling delete

https://codereview.chromium.org/16022003/diff/120001/chrome/test/base/in_proc...
chrome/test/base/in_process_browser_test.cc:445:
base::win::ScopedCOMInitializer* InProcessBrowserTest::com_initializer_ = NULL;
i find it natural for static member definitions like this to come immediately
before the ctor definition, even though they don't appear that way in the
header. that in mind, please move this to just before
InProcessBrowserTest::InProcessBrowserTest.

https://codereview.chromium.org/16022003/diff/120001/chrome/test/base/in_proc...
File chrome/test/base/in_process_browser_test.h (right):

https://codereview.chromium.org/16022003/diff/120001/chrome/test/base/in_proc...
chrome/test/base/in_process_browser_test.h:109: // test case have ran.
nit: ran -> run

[gab, 2013-06-20 23:47:21]

Thanks, @jam: still LGTY?

https://codereview.chromium.org/16022003/diff/120001/chrome/test/base/in_proc...
File chrome/test/base/in_process_browser_test.cc (right):

https://codereview.chromium.org/16022003/diff/120001/chrome/test/base/in_proc...
chrome/test/base/in_process_browser_test.cc:123: delete com_initializer_;
On 2013/06/20 23:10:06, grt wrote:
> com_initializer_ = NULL; after calling delete

Done.

https://codereview.chromium.org/16022003/diff/120001/chrome/test/base/in_proc...
chrome/test/base/in_process_browser_test.cc:445:
base::win::ScopedCOMInitializer* InProcessBrowserTest::com_initializer_ = NULL;
On 2013/06/20 23:10:06, grt wrote:
> i find it natural for static member definitions like this to come immediately
> before the ctor definition, even though they don't appear that way in the
> header. that in mind, please move this to just before
> InProcessBrowserTest::InProcessBrowserTest.

Done.

https://codereview.chromium.org/16022003/diff/120001/chrome/test/base/in_proc...
File chrome/test/base/in_process_browser_test.h (right):

https://codereview.chromium.org/16022003/diff/120001/chrome/test/base/in_proc...
chrome/test/base/in_process_browser_test.h:109: // test case have ran.
On 2013/06/20 23:10:06, grt wrote:
> nit: ran -> run

Arg, I keep making this grammatical mistake!

[grt (UTC plus 2), 2013-06-21 01:16:47]

lgtm++

[gab, 2013-06-21 15:37:54]

Slight tweak: instead of adding switches::kDisableTestCompositor in
InProcessBrowserTest::PrepareTestCommandLine(), call ui::DisableTestCompositor()
in InProcessBrowserTest::SetUpTestCase().

Otherwise ContentTestSuiteBase::Initialize() calls ui::SetupTestCompositor()
(which is the call that checks for switches::kDisableTestCompositor) before the
switch is even added.

Confirmed that "browser_tests --ash-browsertests" now work.

[gab, 2013-06-21 15:40:20]

@jam: Please take another look (this no longer touches content/ so I technically
could ask another reviewer since you are listed as an owner/not-reviewer for
chrome/ if you prefer... figured I'd ask you first since you reviewed the
original patch set already).

Thanks!
Gab

[jam, 2013-06-21 16:32:05]

https://codereview.chromium.org/16022003/diff/159001/chrome/test/base/in_proc...
File chrome/test/base/in_process_browser_test.cc (right):

https://codereview.chromium.org/16022003/diff/159001/chrome/test/base/in_proc...
chrome/test/base/in_process_browser_test.cc:113: void
InProcessBrowserTest::SetUpTestCase() {
I'm not sure I understand the usage of this static override? that seems useful
when multiple tests run in the same process, but for browser tests only one test
runs in a process, so why not put this in SetUp()/TearDown()

[gab, 2013-06-21 16:47:00]

Thanks, PTAL.

https://codereview.chromium.org/16022003/diff/159001/chrome/test/base/in_proc...
File chrome/test/base/in_process_browser_test.cc (right):

https://codereview.chromium.org/16022003/diff/159001/chrome/test/base/in_proc...
chrome/test/base/in_process_browser_test.cc:113: void
InProcessBrowserTest::SetUpTestCase() {
On 2013/06/21 16:32:05, jam wrote:
> I'm not sure I understand the usage of this static override? that seems useful
> when multiple tests run in the same process, but for browser tests only one
test
> runs in a process, so why not put this in SetUp()/TearDown()

Ah ok, sounds good, I had noticed this was the case locally, but wasn't sure
this was always the case. Thanks for clarifying!

Done.

[jam, 2013-06-21 16:58:13]

lgtm

[commit-bot: I haz the power, 2013-06-21 17:03:11]

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

[commit-bot: I haz the power, 2013-06-21 22:31:33]

Change committed as 207936