Add Mojo application test support to Chromium.

Add Mojo application test support to Chromium.

Needed to support Chromium Mojo application tests:
For example: https://codereview.chromium.org/709923003
The Mojo CL: https://codereview.chromium.org/695593002
(env changes support the cross-repo ApplicationTestBase)

TODO: Add GYP versions of GN apptest support targets.

BUG=392646
TEST=mojo/application:test_support builds with GN.
R=jamesr@chromium.org

Committed: https://crrev.com/28a4eca5a9e4053312eaca6b853f004450ce010b
Cr-Commit-Position: refs/heads/master@{#303779}

[msw, 2014-11-11 19:29:13]

msw@chromium.org changed reviewers:
+ jamesr@chromium.org

[msw, 2014-11-11 19:29:48]

Hey James, please take a look; thanks!

[jamesr, 2014-11-11 20:18:31]

lgtm

https://codereview.chromium.org/710403002/diff/1/mojo/application/BUILD.gn
File mojo/application/BUILD.gn (right):

https://codereview.chromium.org/710403002/diff/1/mojo/application/BUILD.gn#ne...
mojo/application/BUILD.gn:24: sources = [ "application_test_main_chromium.cc", ]
nit: can kill the trailing commas in these

https://codereview.chromium.org/710403002/diff/1/mojo/application/application...
File mojo/application/application_test_main_chromium.cc (right):

https://codereview.chromium.org/710403002/diff/1/mojo/application/application...
mojo/application/application_test_main_chromium.cc:45:
CHECK(!base::MessageLoop::current());
this seems superfluous to me

[msw, 2014-11-11 20:27:02]

Landing with 1 of 2 nits addressed.

https://codereview.chromium.org/710403002/diff/1/mojo/application/BUILD.gn
File mojo/application/BUILD.gn (right):

https://codereview.chromium.org/710403002/diff/1/mojo/application/BUILD.gn#ne...
mojo/application/BUILD.gn:24: sources = [ "application_test_main_chromium.cc", ]
On 2014/11/11 20:18:31, jamesr wrote:
> nit: can kill the trailing commas in these

Done.

https://codereview.chromium.org/710403002/diff/1/mojo/application/application...
File mojo/application/application_test_main_chromium.cc (right):

https://codereview.chromium.org/710403002/diff/1/mojo/application/application...
mojo/application/application_test_main_chromium.cc:45:
CHECK(!base::MessageLoop::current());
On 2014/11/11 20:18:31, jamesr wrote:
> this seems superfluous to me

Hmm, left as-is; Trung was adamant about killing loops before testing.

[msw, 2014-11-11 20:27:09]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-11-11 20:28:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/710403002/20001

[msw, 2014-11-11 21:06:59]

The CQ bit was unchecked by msw@chromium.org

[msw, 2014-11-11 21:11:11]

Sigh, I'll need to add the
mojo::Environment::[Instantiate|Destroy]DefaultRunLoop helper functions for
ApplicationTestBase, which is mirrored between Mojo and Chromium codebases. I'll
ping here when that's done.

[msw, 2014-11-11 21:53:26]

Patchset #3 (id:40001) has been deleted

[msw, 2014-11-11 22:29:39]

Hey James, please take another look; thanks!
(this latest patch set works for Xiaohan's CL)

[jamesr, 2014-11-11 22:34:37]

https://codereview.chromium.org/710403002/diff/80001/mojo/application/BUILD.gn
File mojo/application/BUILD.gn (right):

https://codereview.chromium.org/710403002/diff/80001/mojo/application/BUILD.g...
mojo/application/BUILD.gn:25: public_deps = [
"//mojo/public/cpp/application:test_support" ]
blank lines between these, plz

https://codereview.chromium.org/710403002/diff/80001/mojo/environment/default...
File mojo/environment/default_run_loop_impl.cc (right):

https://codereview.chromium.org/710403002/diff/80001/mojo/environment/default...
mojo/environment/default_run_loop_impl.cc:14:
CHECK(!base::MessageLoop::current());
there's already a DCHECK in MessageLoop that covers this, don't add a redundant
one

https://codereview.chromium.org/710403002/diff/80001/mojo/environment/default...
mojo/environment/default_run_loop_impl.cc:17: CHECK_EQ(message_loop,
base::MessageLoop::current());
this is over-checking. a post-condition of the MessageLoop constructor is that
it's in the current() slot

https://codereview.chromium.org/710403002/diff/80001/mojo/environment/default...
mojo/environment/default_run_loop_impl.cc:21:
CHECK(base::MessageLoop::current());
ditto

https://codereview.chromium.org/710403002/diff/80001/mojo/environment/default...
mojo/environment/default_run_loop_impl.cc:22: delete
base::MessageLoop::current();
this is icky. do you expect people to be calling this from multiple threads?
seems nicer to keep track of this ourselves

https://codereview.chromium.org/710403002/diff/80001/mojo/environment/default...
mojo/environment/default_run_loop_impl.cc:23:
CHECK(!base::MessageLoop::current());
more over-checking

[msw, 2014-11-11 23:00:07]

James, please advise on the run loop helpers, thanks!

https://codereview.chromium.org/710403002/diff/80001/mojo/application/BUILD.gn
File mojo/application/BUILD.gn (right):

https://codereview.chromium.org/710403002/diff/80001/mojo/application/BUILD.g...
mojo/application/BUILD.gn:25: public_deps = [
"//mojo/public/cpp/application:test_support" ]
On 2014/11/11 22:34:37, jamesr wrote:
> blank lines between these, plz

Done.

https://codereview.chromium.org/710403002/diff/80001/mojo/environment/default...
File mojo/environment/default_run_loop_impl.cc (right):

https://codereview.chromium.org/710403002/diff/80001/mojo/environment/default...
mojo/environment/default_run_loop_impl.cc:14:
CHECK(!base::MessageLoop::current());
On 2014/11/11 22:34:37, jamesr wrote:
> there's already a DCHECK in MessageLoop that covers this, don't add a
redundant
> one

Done.

https://codereview.chromium.org/710403002/diff/80001/mojo/environment/default...
mojo/environment/default_run_loop_impl.cc:17: CHECK_EQ(message_loop,
base::MessageLoop::current());
On 2014/11/11 22:34:37, jamesr wrote:
> this is over-checking. a post-condition of the MessageLoop constructor is that
> it's in the current() slot

Done.

https://codereview.chromium.org/710403002/diff/80001/mojo/environment/default...
mojo/environment/default_run_loop_impl.cc:21:
CHECK(base::MessageLoop::current());
On 2014/11/11 22:34:37, jamesr wrote:
> ditto

Done.

https://codereview.chromium.org/710403002/diff/80001/mojo/environment/default...
mojo/environment/default_run_loop_impl.cc:22: delete
base::MessageLoop::current();
On 2014/11/11 22:34:37, jamesr wrote:
> this is icky. do you expect people to be calling this from multiple threads?
> seems nicer to keep track of this ourselves

For now, this is used by ApplicationTestBase and application_test_main's
MojoMain; I'm not sure what else might appreciate this environment abstraction.
I can probably remove them altogether if I split ApplicationTestBase into
chromium and standalone variants. I'll follow whatever you suggest.

https://codereview.chromium.org/710403002/diff/80001/mojo/environment/default...
mojo/environment/default_run_loop_impl.cc:23:
CHECK(!base::MessageLoop::current());
On 2014/11/11 22:34:37, jamesr wrote:
> more over-checking

Done.

[jamesr, 2014-11-11 23:07:33]

Where's ApplicationTestBase?

https://codereview.chromium.org/710403002/diff/100001/mojo/application/applic...
File mojo/application/application_test_main_chromium.cc (right):

https://codereview.chromium.org/710403002/diff/100001/mojo/application/applic...
mojo/application/application_test_main_chromium.cc:17: base::AtExitManager
at_exit;
this is only needed if we're creating a base::MessageLoop, right? do we know at
compile time that mojo::Environment::Instantiate..() is going to make a
base::MessageLoop here always in that case?

[msw, 2014-11-11 23:18:28]

On 2014/11/11 23:07:33, jamesr wrote:
> Where's ApplicationTestBase?

mojo/public/cpp/application/application_test_base.h
mojo/public/cpp/application/lib/application_test_base.cc

I would [optionally] continue defining the header in
mojo/public/cpp/application:test_support, and then split the implementation into
separate files for the mojo/public/cpp/application:test_support_standalone and
mojo/application:test_support targets, similar to
application_test_main_[chromium].cc

https://codereview.chromium.org/710403002/diff/100001/mojo/application/applic...
File mojo/application/application_test_main_chromium.cc (right):

https://codereview.chromium.org/710403002/diff/100001/mojo/application/applic...
mojo/application/application_test_main_chromium.cc:17: base::AtExitManager
at_exit;
On 2014/11/11 23:07:33, jamesr wrote:
> this is only needed if we're creating a base::MessageLoop, right? do we know
at
> compile time that mojo::Environment::Instantiate..() is going to make a
> base::MessageLoop here always in that case?

Yes, this file (application_test_main_chromium.cc) uses the chromium-environment
and will use base::MessageLoop.

I previously suggested defining mojo::Environment ctors in the
chromium-environment to handle creation of the AtExitManager, so this bit would
be abstracted away like the run loop helpers using base::MessageLoop or
mojo::RunLoop, but Trung really didn't like that idea.

So again, perhaps these files and ApplicationTestBase should be split up so we
can be explicit about the environment everywhere?

[jamesr, 2014-11-11 23:22:21]

https://codereview.chromium.org/710403002/diff/100001/mojo/application/applic...
File mojo/application/application_test_main_chromium.cc (right):

https://codereview.chromium.org/710403002/diff/100001/mojo/application/applic...
mojo/application/application_test_main_chromium.cc:17: base::AtExitManager
at_exit;
On 2014/11/11 23:18:28, msw wrote:
> On 2014/11/11 23:07:33, jamesr wrote:
> > this is only needed if we're creating a base::MessageLoop, right? do we know
> at
> > compile time that mojo::Environment::Instantiate..() is going to make a
> > base::MessageLoop here always in that case?
> 
> Yes, this file (application_test_main_chromium.cc) uses the
chromium-environment
> and will use base::MessageLoop.

OK... then at line 21 why don't you just call base::MessageLoop directly?  Why
call through mojo::Environment?  This file already depends on base.

> So again, perhaps these files and ApplicationTestBase should be split up so we
> can be explicit about the environment everywhere?

Maybe I'm think but it seems like they already are separate files so they can
use different code.

[msw, 2014-11-11 23:29:33]

https://codereview.chromium.org/710403002/diff/100001/mojo/application/applic...
File mojo/application/application_test_main_chromium.cc (right):

https://codereview.chromium.org/710403002/diff/100001/mojo/application/applic...
mojo/application/application_test_main_chromium.cc:17: base::AtExitManager
at_exit;
On 2014/11/11 23:22:21, jamesr wrote:
> On 2014/11/11 23:18:28, msw wrote:
> > On 2014/11/11 23:07:33, jamesr wrote:
> > > this is only needed if we're creating a base::MessageLoop, right? do we
know
> > at
> > > compile time that mojo::Environment::Instantiate..() is going to make a
> > > base::MessageLoop here always in that case?
> > 
> > Yes, this file (application_test_main_chromium.cc) uses the
> chromium-environment
> > and will use base::MessageLoop.
> 
> OK... then at line 21 why don't you just call base::MessageLoop directly?  Why
> call through mojo::Environment?  This file already depends on base.

For consistency with ApplicationTestBase and the standalone main.cc.
(I didn't have any other really compelling reasons)

> > So again, perhaps these files and ApplicationTestBase should be split up so
we
> > can be explicit about the environment everywhere?
> 
> Maybe I'm think but it seems like they already are separate files so they can
> use different code.

Yeah, both MojoMain()s can use loops directly, ApplicationTestBase can't right
now; it seems like they should be consistent.

[jamesr, 2014-11-11 23:38:19]

On 2014/11/11 23:29:33, msw wrote:
>
https://codereview.chromium.org/710403002/diff/100001/mojo/application/applic...
> File mojo/application/application_test_main_chromium.cc (right):
> 
>
https://codereview.chromium.org/710403002/diff/100001/mojo/application/applic...
> mojo/application/application_test_main_chromium.cc:17: base::AtExitManager
> at_exit;
> On 2014/11/11 23:22:21, jamesr wrote:
> > On 2014/11/11 23:18:28, msw wrote:
> > > On 2014/11/11 23:07:33, jamesr wrote:
> > > > this is only needed if we're creating a base::MessageLoop, right? do we
> know
> > > at
> > > > compile time that mojo::Environment::Instantiate..() is going to make a
> > > > base::MessageLoop here always in that case?
> > > 
> > > Yes, this file (application_test_main_chromium.cc) uses the
> > chromium-environment
> > > and will use base::MessageLoop.
> > 
> > OK... then at line 21 why don't you just call base::MessageLoop directly? 
Why
> > call through mojo::Environment?  This file already depends on base.
> 
> For consistency with ApplicationTestBase and the standalone main.cc.
> (I didn't have any other really compelling reasons)
> 
> > > So again, perhaps these files and ApplicationTestBase should be split up
so
> we
> > > can be explicit about the environment everywhere?
> > 
> > Maybe I'm think but it seems like they already are separate files so they
can
> > use different code.
> 
> Yeah, both MojoMain()s can use loops directly, ApplicationTestBase can't right
> now; it seems like they should be consistent.

This consistency does not seem useful. 
mojo/application/application_test_main_chromium.cc already has code that *only*
does what you want if you're using a base::MessageLoop, so going through an
additional abstraction just obscures what's going on without providing any
increase in flexibility.

[msw, 2014-11-11 23:45:26]

On 2014/11/11 23:38:19, jamesr wrote:
> On 2014/11/11 23:29:33, msw wrote:
> >
>
https://codereview.chromium.org/710403002/diff/100001/mojo/application/applic...
> > File mojo/application/application_test_main_chromium.cc (right):
> > 
> >
>
https://codereview.chromium.org/710403002/diff/100001/mojo/application/applic...
> > mojo/application/application_test_main_chromium.cc:17: base::AtExitManager
> > at_exit;
> > On 2014/11/11 23:22:21, jamesr wrote:
> > > On 2014/11/11 23:18:28, msw wrote:
> > > > On 2014/11/11 23:07:33, jamesr wrote:
> > > > > this is only needed if we're creating a base::MessageLoop, right? do
we
> > know
> > > > at
> > > > > compile time that mojo::Environment::Instantiate..() is going to make
a
> > > > > base::MessageLoop here always in that case?
> > > > 
> > > > Yes, this file (application_test_main_chromium.cc) uses the
> > > chromium-environment
> > > > and will use base::MessageLoop.
> > > 
> > > OK... then at line 21 why don't you just call base::MessageLoop directly? 
> Why
> > > call through mojo::Environment?  This file already depends on base.
> > 
> > For consistency with ApplicationTestBase and the standalone main.cc.
> > (I didn't have any other really compelling reasons)
> > 
> > > > So again, perhaps these files and ApplicationTestBase should be split up
> so
> > we
> > > > can be explicit about the environment everywhere?
> > > 
> > > Maybe I'm think but it seems like they already are separate files so they
> can
> > > use different code.
> > 
> > Yeah, both MojoMain()s can use loops directly, ApplicationTestBase can't
right
> > now; it seems like they should be consistent.
> 
> This consistency does not seem useful. 
> mojo/application/application_test_main_chromium.cc already has code that
*only*
> does what you want if you're using a base::MessageLoop, so going through an
> additional abstraction just obscures what's going on without providing any
> increase in flexibility.

The latest patch set uses base::MessageLoop directly in
application_test_main_chromium.cc, and I can make similar changes to the
standalone main file in a Mojo repo CL.

As-is we still need the environment functions for ApplicationTestBase.

[jamesr, 2014-11-11 23:50:20]

The ApplicationTestBase code doesn't appear to be changed in this patch.  Does
it currently not function correctly or not work at all?

[msw, 2014-11-11 23:51:58]

On 2014/11/11 23:50:20, jamesr wrote:
> The ApplicationTestBase code doesn't appear to be changed in this patch.  Does
> it currently not function correctly or not work at all?

Yeah, it was broken in the Chromium repro, it hadn't been used yet.

[jamesr, 2014-11-11 23:53:58]

OK. lgtm

[msw, 2014-11-11 23:58:24]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-11-11 23:59:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/710403002/120001

[commit-bot: I haz the power, 2014-11-12 00:21:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-12 00:21:18]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[msw, 2014-11-12 00:55:20]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-11-12 00:56:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/710403002/140001

[commit-bot: I haz the power, 2014-11-12 02:32:27]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2014-11-12 02:32:58]

Patchset 7 (id:??) landed as
https://crrev.com/28a4eca5a9e4053312eaca6b853f004450ce010b
Cr-Commit-Position: refs/heads/master@{#303779}