Add a Mojo example apptest that runs in mojo_shell.

Add a Mojo example apptest that runs in mojo_shell.

Add mojo/example/apptest with a service and client.
Add example_apptest.cc with a GTest Mojo application.

These tests can be run from out/<Debug|Release>/ via:
./mojo_shell --origin=file://`pwd` --disable-cache mojo:mojo_example_apptests

BUG=392646
TEST=The new mojo_example_apptests passes.
R=ben@chromium.org

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

[msw, 2014-07-18 18:42:57]

This is a horrific WIP, but I'd appreciate your high level comments and
hopefully a chance to chat about the direction here and which gaps in my
knowledge I should focus on.

[Ben Goodger (Google), 2014-07-18 20:02:52]

https://codereview.chromium.org/399653004/diff/1/mojo/examples/test/example_s...
File mojo/examples/test/example_service_impl.cc (right):

https://codereview.chromium.org/399653004/diff/1/mojo/examples/test/example_s...
mojo/examples/test/example_service_impl.cc:22: RunLoop::current()->Quit();
What are you trying to achieve here?

https://codereview.chromium.org/399653004/diff/1/mojo/examples/test/example_s...
mojo/examples/test/example_service_impl.cc:28: RunLoop::current()->Quit();
again not sure why you're quitting the runloop here.

https://codereview.chromium.org/399653004/diff/1/mojo/examples/test/example_s...
File mojo/examples/test/example_service_impl.h (right):

https://codereview.chromium.org/399653004/diff/1/mojo/examples/test/example_s...
mojo/examples/test/example_service_impl.h:19: ApplicationDelegate* delegate);
You shouldn't need to pass the delegate, at least not as a ApplicationDelegate*.
The intent of the second parameter is to pass some global state into this
instance.

For every unique connection to your service there is one instance of this class.
The context parameter allows you to have some singleton state shared between all
connection instances.

https://codereview.chromium.org/399653004/diff/1/mojo/examples/test/example_u...
File mojo/examples/test/example_unittest.cc (right):

https://codereview.chromium.org/399653004/diff/1/mojo/examples/test/example_u...
mojo/examples/test/example_unittest.cc:61:
//connection_->DoRunLoopUntilChangesCount(1);
Check out the code I recently landed here:

http://src.chromium.org/viewvc/chrome/trunk/src/mojo/services/window_manager/...

https://codereview.chromium.org/399653004/diff/1/mojo/examples/test/example_u...
mojo/examples/test/example_unittest.cc:82: // TODO(msw): Is this needed ??? Move
this elsewhere ...
No. You should be relying on the shell's service loader for this. See the unit
test I cited above.

https://codereview.chromium.org/399653004/diff/1/mojo/examples/test/example_u...
mojo/examples/test/example_unittest.cc:161: ServiceManager* service_manager =
ServiceManager::GetInstance();
I *think* you should be able to write this test without using any of the code in
mojo/service_manager directly. You should be able to use the methods exposed on
ApplicationImpl to connect to applications & services.

https://codereview.chromium.org/399653004/diff/1/mojo/mojo_examples.gypi
File mojo/mojo_examples.gypi (right):

https://codereview.chromium.org/399653004/diff/1/mojo/mojo_examples.gypi#newc...
mojo/mojo_examples.gypi:94: #'mojo_shell_test_support',           # TODO(msw):
Needed ???
My rule of thumb for these things is if I ever am not sure about what deps I
need, start with an empty target and add services until it links. Better than
cargo-culting in a bunch of stuff you're not sure about.

[msw, 2014-07-21 18:58:23]

How can I get ShellTestHelper working here?

https://codereview.chromium.org/399653004/diff/1/mojo/examples/test/example_s...
File mojo/examples/test/example_service_impl.cc (right):

https://codereview.chromium.org/399653004/diff/1/mojo/examples/test/example_s...
mojo/examples/test/example_service_impl.cc:22: RunLoop::current()->Quit();
On 2014/07/18 20:02:52, Ben Goodger (Google) wrote:
> What are you trying to achieve here?

The process continues running after executing the tests (need to Ctrl+C in the
commandline); RunLoop::Quit calls don't help, so I removed them. Any tips?

https://codereview.chromium.org/399653004/diff/1/mojo/examples/test/example_s...
mojo/examples/test/example_service_impl.cc:28: RunLoop::current()->Quit();
On 2014/07/18 20:02:52, Ben Goodger (Google) wrote:
> again not sure why you're quitting the runloop here.

ditto; removed.

https://codereview.chromium.org/399653004/diff/1/mojo/examples/test/example_s...
File mojo/examples/test/example_service_impl.h (right):

https://codereview.chromium.org/399653004/diff/1/mojo/examples/test/example_s...
mojo/examples/test/example_service_impl.h:19: ApplicationDelegate* delegate);
On 2014/07/18 20:02:52, Ben Goodger (Google) wrote:
> You shouldn't need to pass the delegate, at least not as a
ApplicationDelegate*.
> The intent of the second parameter is to pass some global state into this
> instance.
> 
> For every unique connection to your service there is one instance of this
class.
> The context parameter allows you to have some singleton state shared between
all
> connection instances.

Cool, I removed this param the |this| argument from the
connection->AddService<ExampleServiceImpl>() call in
ExampleServiceApplication::ConfigureIncomingConnection().

https://codereview.chromium.org/399653004/diff/1/mojo/examples/test/example_u...
File mojo/examples/test/example_unittest.cc (right):

https://codereview.chromium.org/399653004/diff/1/mojo/examples/test/example_u...
mojo/examples/test/example_unittest.cc:61:
//connection_->DoRunLoopUntilChangesCount(1);
On 2014/07/18 20:02:52, Ben Goodger (Google) wrote:
> Check out the code I recently landed here:
> 
>
http://src.chromium.org/viewvc/chrome/trunk/src/mojo/services/window_manager/...

I'm having issues using ShellTestHelper here, and feel stuck.

mojo_shell.exe's main Context and ShellTestHelper's Context both initialize the
base::LazyInstance<Setup>::Leaky |setup|, and the second call to
gles2::GLES2SupportImpl::Init asserts. Should the shell's Context be shared with
the app? Should the LazyInstance Setup object be shared between the two Context
objects? I'm digging but came up short earlier, prompting my attempts to use
ServiceManager::Instance, etc.

https://codereview.chromium.org/399653004/diff/1/mojo/examples/test/example_u...
mojo/examples/test/example_unittest.cc:82: // TODO(msw): Is this needed ??? Move
this elsewhere ...
On 2014/07/18 20:02:52, Ben Goodger (Google) wrote:
> No. You should be relying on the shell's service loader for this. See the unit
> test I cited above.

Done.

https://codereview.chromium.org/399653004/diff/1/mojo/examples/test/example_u...
mojo/examples/test/example_unittest.cc:161: ServiceManager* service_manager =
ServiceManager::GetInstance();
On 2014/07/18 20:02:52, Ben Goodger (Google) wrote:
> I *think* you should be able to write this test without using any of the code
in
> mojo/service_manager directly. You should be able to use the methods exposed
on
> ApplicationImpl to connect to applications & services.

Done (with a hacky global for now, need help).

https://codereview.chromium.org/399653004/diff/1/mojo/mojo_examples.gypi
File mojo/mojo_examples.gypi (right):

https://codereview.chromium.org/399653004/diff/1/mojo/mojo_examples.gypi#newc...
mojo/mojo_examples.gypi:94: #'mojo_shell_test_support',           # TODO(msw):
Needed ???
On 2014/07/18 20:02:52, Ben Goodger (Google) wrote:
> My rule of thumb for these things is if I ever am not sure about what deps I
> need, start with an empty target and add services until it links. Better than
> cargo-culting in a bunch of stuff you're not sure about.

I've been careful to only include necessary dependencies. I've now added
mojo_shell_test_support for ShellTestHelper, and removed mojo_shell_tests.

[Ben Goodger (Google), 2014-07-21 20:02:39]

https://codereview.chromium.org/399653004/diff/1/mojo/examples/test/example_s...
File mojo/examples/test/example_service_impl.cc (right):

https://codereview.chromium.org/399653004/diff/1/mojo/examples/test/example_s...
mojo/examples/test/example_service_impl.cc:22: RunLoop::current()->Quit();
On 2014/07/21 18:58:22, msw wrote:
> On 2014/07/18 20:02:52, Ben Goodger (Google) wrote:
> > What are you trying to achieve here?
> 
> The process continues running after executing the tests (need to Ctrl+C in the
> commandline); RunLoop::Quit calls don't help, so I removed them. Any tips?

You would not be able to add code like this to a production service you were
testing. Instead I think you need the ability to end the service. I think this
code is in flux currently. Dave's code attempts to manage service lifetimes
automagically, and to me that implies that when the total connection count of an
application's services drops to zero, that that application thread would exit,
but he says that's not currently how it works.

https://codereview.chromium.org/399653004/diff/1/mojo/examples/test/example_u...
File mojo/examples/test/example_unittest.cc (right):

https://codereview.chromium.org/399653004/diff/1/mojo/examples/test/example_u...
mojo/examples/test/example_unittest.cc:61:
//connection_->DoRunLoopUntilChangesCount(1);
So I don't think you can use a ShellTestHelper... the purpose of the
ShellTestHelper is to "emulate" the shell in a conventional chrome unit test.
But this test is a mojo app run inside the real Mojo shell - so I am not
surprised you're seeing the issues you're seeing, and this means you shouldn't
be using it here.

https://codereview.chromium.org/399653004/diff/1/mojo/mojo_examples.gypi
File mojo/mojo_examples.gypi (right):

https://codereview.chromium.org/399653004/diff/1/mojo/mojo_examples.gypi#newc...
mojo/mojo_examples.gypi:94: #'mojo_shell_test_support',           # TODO(msw):
Needed ???
See earlier comment about not using ShellTestHelper! :-)

https://codereview.chromium.org/399653004/diff/40001/mojo/examples/test/examp...
File mojo/examples/test/example_unittest.cc (right):

https://codereview.chromium.org/399653004/diff/40001/mojo/examples/test/examp...
mojo/examples/test/example_unittest.cc:114: g_application_impl_hack_ = app;
This is the kind of thing that I think could be improved by the application
infra code stashing a ptr to itself in TLS and providing an API to obtaining it
from anywhere in the app without having to pass it around. Otherwise every test
app will have to reproduce this global setting. I'm CC'ing dave.

[tim (not reviewing), 2014-07-23 01:16:10]

https://codereview.chromium.org/399653004/diff/1/mojo/examples/test/example_s...
File mojo/examples/test/example_service_impl.cc (right):

https://codereview.chromium.org/399653004/diff/1/mojo/examples/test/example_s...
mojo/examples/test/example_service_impl.cc:22: RunLoop::current()->Quit();
On 2014/07/21 20:02:38, Ben Goodger (Google) wrote:
> On 2014/07/21 18:58:22, msw wrote:
> > On 2014/07/18 20:02:52, Ben Goodger (Google) wrote:
> > > What are you trying to achieve here?
> > 
> > The process continues running after executing the tests (need to Ctrl+C in
the
> > commandline); RunLoop::Quit calls don't help, so I removed them. Any tips?
> 
> You would not be able to add code like this to a production service you were
> testing. Instead I think you need the ability to end the service. I think this
> code is in flux currently. Dave's code attempts to manage service lifetimes
> automagically, and to me that implies that when the total connection count of
an
> application's services drops to zero, that that application thread would exit,
> but he says that's not currently how it works.

I think that the way it is written now, ExampleServiceImpl is an example of a
"well behaved" app in that it quits itself when done. Kind of like
examples/wget.cc.  Ideally, most apps would have a condition they would meet and
eventually quit their run loops.

For cases where they do not, we need to have other ways of killing them
especially for tests.  This is what I am working on currently; see
https://docs.google.com/a/chromium.org/document/d/1tKZw3cnnGEuoOD1gYLmPQtqYHN...

But for getting started here, writing a well-defined app like this that quits
itself is probably OK?

[msw, 2014-07-23 09:29:10]

Please take a fresh high-level look after great pointers from Tim and Trung.
I'd especially appreciate any opinions or tips on "???" and "..." comments.

[msw, 2014-07-24 17:50:02]

Ping! Is this going in the right direction? Do we want to actually add these
examples or should I abandon this and start converting an existing test to work
roughly in this manner?

[Ben Goodger (Google), 2014-07-30 16:53:19]

https://codereview.chromium.org/399653004/diff/120001/mojo/examples/test/exam...
File mojo/examples/test/example_service_impl.h (right):

https://codereview.chromium.org/399653004/diff/120001/mojo/examples/test/exam...
mojo/examples/test/example_service_impl.h:17: explicit
ExampleServiceImpl(ApplicationConnection* connection);
nit: extra 1-space indent for these 2 lines

https://codereview.chromium.org/399653004/diff/120001/mojo/examples/test/exam...
mojo/examples/test/example_service_impl.h:20: protected:
Move to private

https://codereview.chromium.org/399653004/diff/120001/mojo/examples/test/exam...
File mojo/examples/test/example_unittest.cc (right):

https://codereview.chromium.org/399653004/diff/120001/mojo/examples/test/exam...
mojo/examples/test/example_unittest.cc:10: #include
"mojo/examples/test/example_service_impl.h"
You shouldn't need to include this here. If you do, there's some kind of
layering violation.

https://codereview.chromium.org/399653004/diff/120001/mojo/examples/test/exam...
mojo/examples/test/example_unittest.cc:21: // TODO(msw): Avoid globals ...
// TODO(msw): Remove these once we can get ApplicationImpl from TLS.

https://codereview.chromium.org/399653004/diff/120001/mojo/examples/test/exam...
mojo/examples/test/example_unittest.cc:23: mojo::RunLoop* g_run_loop_hack =
NULL;
I think this is fine.

https://codereview.chromium.org/399653004/diff/120001/mojo/examples/test/exam...
mojo/examples/test/example_unittest.cc:35: virtual ~ExampleClientImpl() {}
nit: newline after

https://codereview.chromium.org/399653004/diff/120001/mojo/examples/test/exam...
mojo/examples/test/example_unittest.cc:38: protected:
move to private

https://codereview.chromium.org/399653004/diff/140001/mojo/examples/test/exam...
File mojo/examples/test/example_unittest.cc (right):

https://codereview.chromium.org/399653004/diff/140001/mojo/examples/test/exam...
mojo/examples/test/example_unittest.cc:110: mojo::ApplicationDelegate delegate;
I think you can implement an ApplicationDelegate in this file and move all the
subsequent gtest stuff to Initialize().

https://codereview.chromium.org/399653004/diff/140001/mojo/examples/test/exam...
mojo/examples/test/example_unittest.cc:122: return RUN_ALL_TESTS() == 0 ?
MOJO_RESULT_OK : MOJO_RESULT_UNKNOWN;
lifetime issues being handled by timsteele it seems.

You can just return MOJO_RESULT_OK for the time being.

https://codereview.chromium.org/399653004/diff/140001/mojo/mojo_examples.gypi
File mojo/mojo_examples.gypi (right):

https://codereview.chromium.org/399653004/diff/140001/mojo/mojo_examples.gypi...
mojo/mojo_examples.gypi:134: 'target_name': 'mojo_example_unittests',
as discussed, _shelltests or _apptests... your choice.

[msw, 2014-08-06 22:34:40]

Hey Ben, please take another look; thanks!

https://codereview.chromium.org/399653004/diff/120001/mojo/examples/test/exam...
File mojo/examples/test/example_service_impl.h (right):

https://codereview.chromium.org/399653004/diff/120001/mojo/examples/test/exam...
mojo/examples/test/example_service_impl.h:17: explicit
ExampleServiceImpl(ApplicationConnection* connection);
On 2014/07/30 16:53:18, Ben Goodger (Google) wrote:
> nit: extra 1-space indent for these 2 lines

Done.

https://codereview.chromium.org/399653004/diff/120001/mojo/examples/test/exam...
mojo/examples/test/example_service_impl.h:20: protected:
On 2014/07/30 16:53:18, Ben Goodger (Google) wrote:
> Move to private

Done.

https://codereview.chromium.org/399653004/diff/120001/mojo/examples/test/exam...
File mojo/examples/test/example_unittest.cc (right):

https://codereview.chromium.org/399653004/diff/120001/mojo/examples/test/exam...
mojo/examples/test/example_unittest.cc:10: #include
"mojo/examples/test/example_service_impl.h"
On 2014/07/30 16:53:18, Ben Goodger (Google) wrote:
> You shouldn't need to include this here. If you do, there's some kind of
> layering violation.

Done.

https://codereview.chromium.org/399653004/diff/120001/mojo/examples/test/exam...
mojo/examples/test/example_unittest.cc:21: // TODO(msw): Avoid globals ...
On 2014/07/30 16:53:18, Ben Goodger (Google) wrote:
> // TODO(msw): Remove these once we can get ApplicationImpl from TLS.

Done, but what is TLS? :-/

https://codereview.chromium.org/399653004/diff/120001/mojo/examples/test/exam...
mojo/examples/test/example_unittest.cc:23: mojo::RunLoop* g_run_loop_hack =
NULL;
On 2014/07/30 16:53:18, Ben Goodger (Google) wrote:
> I think this is fine.

Acknowledged, but I switched to RunLoop::current().

https://codereview.chromium.org/399653004/diff/120001/mojo/examples/test/exam...
mojo/examples/test/example_unittest.cc:35: virtual ~ExampleClientImpl() {}
On 2014/07/30 16:53:18, Ben Goodger (Google) wrote:
> nit: newline after

Done.

https://codereview.chromium.org/399653004/diff/120001/mojo/examples/test/exam...
mojo/examples/test/example_unittest.cc:38: protected:
On 2014/07/30 16:53:18, Ben Goodger (Google) wrote:
> move to private

Done.

https://codereview.chromium.org/399653004/diff/140001/mojo/examples/test/exam...
File mojo/examples/test/example_unittest.cc (right):

https://codereview.chromium.org/399653004/diff/140001/mojo/examples/test/exam...
mojo/examples/test/example_unittest.cc:110: mojo::ApplicationDelegate delegate;
On 2014/07/30 16:53:18, Ben Goodger (Google) wrote:
> I think you can implement an ApplicationDelegate in this file and move all the
> subsequent gtest stuff to Initialize().

After moving the client service impl and application delegate to separate files,
I don't think that's the right way to go. Additionally, calling RUN_ALL_TESTS
from ExampleClientApplication::Initialize yields SEH exceptions in the tests;
perhaps Initialize is to early to actually run the tests? I can dig deeper or
post a RunAllTests task from Initialize and spin the message loop from MojoMain,
if you prefer that (patch set 7):
https://codereview.chromium.org/399653004/diff/220001/mojo/examples/test/exam...

https://codereview.chromium.org/399653004/diff/140001/mojo/examples/test/exam...
mojo/examples/test/example_unittest.cc:122: return RUN_ALL_TESTS() == 0 ?
MOJO_RESULT_OK : MOJO_RESULT_UNKNOWN;
On 2014/07/30 16:53:18, Ben Goodger (Google) wrote:
> lifetime issues being handled by timsteele it seems.
> 
> You can just return MOJO_RESULT_OK for the time being.

Done.

https://codereview.chromium.org/399653004/diff/140001/mojo/mojo_examples.gypi
File mojo/mojo_examples.gypi (right):

https://codereview.chromium.org/399653004/diff/140001/mojo/mojo_examples.gypi...
mojo/mojo_examples.gypi:134: 'target_name': 'mojo_example_unittests',
On 2014/07/30 16:53:19, Ben Goodger (Google) wrote:
> as discussed, _shelltests or _apptests... your choice.
> 

Done; Tim said _apptests wouldn't conflict with the existing shell tests.

[Ben Goodger (Google), 2014-08-07 15:53:09]

https://codereview.chromium.org/399653004/diff/260001/mojo/examples/apptest/e...
File mojo/examples/apptest/example_apptest.cc (right):

https://codereview.chromium.org/399653004/diff/260001/mojo/examples/apptest/e...
mojo/examples/apptest/example_apptest.cc:35: example_service_.reset();
should not be necessary.

https://codereview.chromium.org/399653004/diff/260001/mojo/examples/apptest/e...
mojo/examples/apptest/example_apptest.cc:93: printf("Exiting MojoMain.\n");
delete

https://codereview.chromium.org/399653004/diff/260001/mojo/examples/apptest/e...
File mojo/examples/apptest/example_service_application.cc (right):

https://codereview.chromium.org/399653004/diff/260001/mojo/examples/apptest/e...
mojo/examples/apptest/example_service_application.cc:18:
connection->AddService<ExampleService>(this);
... and then register it here, unless you plan on doing something fancy with
your ExampleServiceImpl lifetime...

https://codereview.chromium.org/399653004/diff/260001/mojo/examples/apptest/e...
File mojo/examples/apptest/example_service_application.h (right):

https://codereview.chromium.org/399653004/diff/260001/mojo/examples/apptest/e...
mojo/examples/apptest/example_service_application.h:23: // ApplicationDelegate
implementation.
I have a habit of pushing interface implementation methods to private unless you
expect someone to need to call them via a ptr to ExampleServiceApplication. That
keeps the public API focused.

https://codereview.chromium.org/399653004/diff/260001/mojo/examples/apptest/e...
mojo/examples/apptest/example_service_application.h:28: virtual void
Create(ApplicationConnection* connection,
You could just add an InterfaceFactoryImpl member instead rather than
implementing this method.

[msw, 2014-08-07 18:31:16]

Comments addressed, please take another look; thanks!

https://codereview.chromium.org/399653004/diff/260001/mojo/examples/apptest/e...
File mojo/examples/apptest/example_apptest.cc (right):

https://codereview.chromium.org/399653004/diff/260001/mojo/examples/apptest/e...
mojo/examples/apptest/example_apptest.cc:35: example_service_.reset();
On 2014/08/07 15:53:09, Ben Goodger (Google) wrote:
> should not be necessary.

Done.

https://codereview.chromium.org/399653004/diff/260001/mojo/examples/apptest/e...
mojo/examples/apptest/example_apptest.cc:93: printf("Exiting MojoMain.\n");
On 2014/08/07 15:53:09, Ben Goodger (Google) wrote:
> delete

Done.

https://codereview.chromium.org/399653004/diff/260001/mojo/examples/apptest/e...
File mojo/examples/apptest/example_service_application.cc (right):

https://codereview.chromium.org/399653004/diff/260001/mojo/examples/apptest/e...
mojo/examples/apptest/example_service_application.cc:18:
connection->AddService<ExampleService>(this);
On 2014/08/07 15:53:09, Ben Goodger (Google) wrote:
> ... and then register it here, unless you plan on doing something fancy with
> your ExampleServiceImpl lifetime...

Done.

https://codereview.chromium.org/399653004/diff/260001/mojo/examples/apptest/e...
File mojo/examples/apptest/example_service_application.h (right):

https://codereview.chromium.org/399653004/diff/260001/mojo/examples/apptest/e...
mojo/examples/apptest/example_service_application.h:23: // ApplicationDelegate
implementation.
On 2014/08/07 15:53:09, Ben Goodger (Google) wrote:
> I have a habit of pushing interface implementation methods to private unless
you
> expect someone to need to call them via a ptr to ExampleServiceApplication.
That
> keeps the public API focused.

Done.

https://codereview.chromium.org/399653004/diff/260001/mojo/examples/apptest/e...
mojo/examples/apptest/example_service_application.h:28: virtual void
Create(ApplicationConnection* connection,
On 2014/08/07 15:53:09, Ben Goodger (Google) wrote:
> You could just add an InterfaceFactoryImpl member instead rather than
> implementing this method.

Done.

[Ben Goodger (Google), 2014-08-07 20:01:35]

lgtm

[msw, 2014-08-07 20:02:30]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-08-07 20:06:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/msw@chromium.org/399653004/280001

[msw, 2014-08-07 20:48:17]

The CQ bit was unchecked by msw@chromium.org

[msw, 2014-08-07 21:15:11]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-08-07 21:22:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/msw@chromium.org/399653004/300001

[msw, 2014-08-08 00:29:21]

The CQ bit was unchecked by msw@chromium.org

[msw, 2014-08-08 00:30:28]

The CQ bit was checked by msw@chromium.org

[commit-bot: I haz the power, 2014-08-08 00:54:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/msw@chromium.org/399653004/320001

[commit-bot: I haz the power, 2014-08-08 08:14:15]

Change committed as 288250