Split application_test_main.cc out of *_base.cc.

Split application_test_main.cc out of *_base.cc.

Move MojoMain for apptests to a new file.
Add mojo::test accessors for the shell handle.

BUG=392646
TEST="mojo_shell mojo:example_apptests" still works.
R=viettrungluu@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/e43d85e22e814c65ca91b564a48c328f9a05d8b8

[msw, 2014-10-28 19:02:11]

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

[msw, 2014-10-28 19:02:30]

Hey Trung, please take a look; thanks!

[viettrungluu, 2014-10-28 23:40:20]

https://codereview.chromium.org/680263004/diff/20001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/lib/DEPS (right):

https://codereview.chromium.org/680263004/diff/20001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/DEPS:1: specific_include_rules = {
It's probably okay to just allow mojo/public/cpp/utility for all of this
directory.

(It's a little fragile, but probably not a huge deal.)

https://codereview.chromium.org/680263004/diff/20001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/lib/application_test_base.cc (right):

https://codereview.chromium.org/680263004/diff/20001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/application_test_base.cc:27:
ScopedMessagePipeHandle GetShellHandleForTest() {
Maybe call this "PassShellHandle()", to indicate that it passes ownership of the
handle?

("ForTest" seems redundant, since it's in the test namespace.)

https://codereview.chromium.org/680263004/diff/20001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/application_test_base.cc:29: return
MakeScopedHandle(test_shell_handle);
This is wrong: Either you should return a (non-owning) MessagePipeHandle (in
which case "Get" is fine), or you should clear test_shell_handle before
returning.

https://codereview.chromium.org/680263004/diff/20001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/application_test_base.cc:32: void
SetShellHandleForTest(ScopedMessagePipeHandle handle) {
...

[msw, 2014-10-29 00:12:38]

Please take another look; thanks!

https://codereview.chromium.org/680263004/diff/20001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/lib/DEPS (right):

https://codereview.chromium.org/680263004/diff/20001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/DEPS:1: specific_include_rules = {
On 2014/10/28 23:40:19, viettrungluu wrote:
> It's probably okay to just allow mojo/public/cpp/utility for all of this
> directory.
> 
> (It's a little fragile, but probably not a huge deal.)

Done.

https://codereview.chromium.org/680263004/diff/20001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/lib/application_test_base.cc (right):

https://codereview.chromium.org/680263004/diff/20001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/application_test_base.cc:27:
ScopedMessagePipeHandle GetShellHandleForTest() {
On 2014/10/28 23:40:19, viettrungluu wrote:
> Maybe call this "PassShellHandle()", to indicate that it passes ownership of
the
> handle?
> 
> ("ForTest" seems redundant, since it's in the test namespace.)

Done.

https://codereview.chromium.org/680263004/diff/20001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/application_test_base.cc:29: return
MakeScopedHandle(test_shell_handle);
On 2014/10/28 23:40:20, viettrungluu wrote:
> This is wrong: Either you should return a (non-owning) MessagePipeHandle (in
> which case "Get" is fine), or you should clear test_shell_handle before
> returning.

Done; went with the latter.

https://codereview.chromium.org/680263004/diff/20001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/application_test_base.cc:32: void
SetShellHandleForTest(ScopedMessagePipeHandle handle) {
On 2014/10/28 23:40:20, viettrungluu wrote:
> ...

Renamed to SetShellHandle, hope that's what you meant.

[viettrungluu, 2014-10-29 01:21:46]

https://codereview.chromium.org/680263004/diff/40001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/lib/application_test_base.cc (right):

https://codereview.chromium.org/680263004/diff/40001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/application_test_base.cc:20: MessagePipeHandle
test_shell_handle;
nit: g_test_shell_handle.

Actually, "test" seems misleading, so maybe just "g_shell_handle".

https://codereview.chromium.org/680263004/diff/40001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/application_test_base.cc:35:
MOJO_CHECK(handle.is_valid());
You should either MOJO_CHECK(!test_shell_handle.is_valid()); or you should close
it when it's invalid.

[msw, 2014-10-29 17:53:19]

Done, please take another look; thanks!

https://codereview.chromium.org/680263004/diff/40001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/lib/application_test_base.cc (right):

https://codereview.chromium.org/680263004/diff/40001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/application_test_base.cc:20: MessagePipeHandle
test_shell_handle;
On 2014/10/29 01:21:46, viettrungluu wrote:
> nit: g_test_shell_handle.
> 
> Actually, "test" seems misleading, so maybe just "g_shell_handle".

Done.

https://codereview.chromium.org/680263004/diff/40001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/application_test_base.cc:35:
MOJO_CHECK(handle.is_valid());
On 2014/10/29 01:21:46, viettrungluu wrote:
> You should either MOJO_CHECK(!test_shell_handle.is_valid()); or you should
close
> it when it's invalid.

Done.

[viettrungluu, 2014-10-29 19:20:20]

LGTM w/a couple of comments.

https://codereview.chromium.org/680263004/diff/60001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/BUILD.gn (right):

https://codereview.chromium.org/680263004/diff/60001/mojo/public/cpp/applicat...
mojo/public/cpp/application/BUILD.gn:49: source_set("test_support") {
Probably you'll want to have a source set (called "test_support") with just
application_test_base.h (and its dependencies), and another one (maybe called
"test_support_standalone") with the .cc files.

You don't need to do that in this change.

https://codereview.chromium.org/680263004/diff/60001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/lib/application_test_base.cc (right):

https://codereview.chromium.org/680263004/diff/60001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/application_test_base.cc:29:
ScopedMessagePipeHandle scoped_handle = MakeScopedHandle(g_shell_handle);
Can't you just do |ScopedMessagePipeHandle scoped_handle(g_shell_handle);|?

https://codereview.chromium.org/680263004/diff/60001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/application_test_base.cc:30:
g_shell_handle.set_value(MOJO_HANDLE_INVALID);
Probably a slightly more idiomatic thing to do would be simply |g_shell_handle =
MessagePipeHandle();|.

[msw, 2014-10-29 19:24:02]

Patchset #5 (id:80001) has been deleted

[msw, 2014-10-29 19:26:45]

Comments addressed, I'll work on test_support_standalone and the environment run
loop stuff separately; landing.

https://codereview.chromium.org/680263004/diff/60001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/BUILD.gn (right):

https://codereview.chromium.org/680263004/diff/60001/mojo/public/cpp/applicat...
mojo/public/cpp/application/BUILD.gn:49: source_set("test_support") {
On 2014/10/29 19:20:19, viettrungluu wrote:
> Probably you'll want to have a source set (called "test_support") with just
> application_test_base.h (and its dependencies), and another one (maybe called
> "test_support_standalone") with the .cc files.
> 
> You don't need to do that in this change.

Acknowledged.

https://codereview.chromium.org/680263004/diff/60001/mojo/public/cpp/applicat...
File mojo/public/cpp/application/lib/application_test_base.cc (right):

https://codereview.chromium.org/680263004/diff/60001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/application_test_base.cc:29:
ScopedMessagePipeHandle scoped_handle = MakeScopedHandle(g_shell_handle);
On 2014/10/29 19:20:19, viettrungluu wrote:
> Can't you just do |ScopedMessagePipeHandle scoped_handle(g_shell_handle);|?

Done.

https://codereview.chromium.org/680263004/diff/60001/mojo/public/cpp/applicat...
mojo/public/cpp/application/lib/application_test_base.cc:30:
g_shell_handle.set_value(MOJO_HANDLE_INVALID);
On 2014/10/29 19:20:19, viettrungluu wrote:
> Probably a slightly more idiomatic thing to do would be simply |g_shell_handle
=
> MessagePipeHandle();|.

Done.

[msw, 2014-10-29 19:34:04]

Committed patchset #6 (id:120001) manually as
e43d85e22e814c65ca91b564a48c328f9a05d8b8 (presubmit successful).