Simplify mojo_shell since it's now only used for Mandoline.

Simplify chromium's mojo_shell since it's now only used for Mandoline.

-no need to map kiosk_wm to window_manager, instead, make kiosk_wm's binary output be window_manager.mojom
-no need for app-specific args, instead use base::CommandLine. This is important for Mandoline since we'll be sharing code with Chrome which does use these flags.
-mojo_shell url is enough to start a browser now
-if an argument that starts with mojo: scheme is given (i.e. for an apptest), we load it. otherwise we run mojo:window_manager

Committed: https://crrev.com/143eb32f63bd929f7d730970e2477804f318a7eb
Cr-Commit-Position: refs/heads/master@{#324579}

Committed: https://crrev.com/541ee5bc890db30033673826d3ed5c19ecd9d339
Cr-Commit-Position: refs/heads/master@{#324974}

[jam, 2015-04-09 23:07:16]

jam@chromium.org changed reviewers:
+ ben@chromium.org

[Ben Goodger (Google), 2015-04-09 23:25:22]

https://codereview.chromium.org/1057603003/diff/60001/mojo/services/kiosk_wm/...
File mojo/services/kiosk_wm/BUILD.gn (right):

https://codereview.chromium.org/1057603003/diff/60001/mojo/services/kiosk_wm/...
mojo/services/kiosk_wm/BUILD.gn:8: # kios_wm is the one and only window_manager
used for chromium's mojo_shell, so
"Mojo shell in chromium is only used for Mandoline, and Mandoline only uses
kiosk_wm, so we name the target window_manager to avoid having to remap on the
command line."

https://codereview.chromium.org/1057603003/diff/60001/mojo/services/window_ma...
File mojo/services/window_manager/BUILD.gn (right):

https://codereview.chromium.org/1057603003/diff/60001/mojo/services/window_ma...
mojo/services/window_manager/BUILD.gn:95:
mojo_native_application("window_manager_default") {
maybe test_window_manager?

https://codereview.chromium.org/1057603003/diff/60001/mojo/shell/application_...
File mojo/shell/application_manager/application_manager.cc (right):

https://codereview.chromium.org/1057603003/diff/60001/mojo/shell/application_...
mojo/shell/application_manager/application_manager.cc:153: if
(ConnectToApplicationWithLoader(resolved_url, requestor_url, &services,
strictly speaking, do you need any of these changes for this CL or is this just
more cleanup? if the latter camp, could we defer this?

i think the code in the shell executable (i.e. desktop & android subdirs) will
undoubtedly diverge from the generic mojo shell. i also think this stuff might
end up changing but by doing this we are explicitly making the mojo shell lib
source set incompatible with the upstream one. it's not clear to me just yet
that there isn't an unforkable subset of code here.

anyway the reason I ask is that there seem to be two things you are doing in
this cl: 1: making some useful improvements to usability of the shell for
mandoline and 2: doing some cleanup. i'm wondering if we can split those.

[jam, 2015-04-10 00:06:55]

https://codereview.chromium.org/1057603003/diff/60001/mojo/services/kiosk_wm/...
File mojo/services/kiosk_wm/BUILD.gn (right):

https://codereview.chromium.org/1057603003/diff/60001/mojo/services/kiosk_wm/...
mojo/services/kiosk_wm/BUILD.gn:8: # kios_wm is the one and only window_manager
used for chromium's mojo_shell, so
On 2015/04/09 23:25:22, Ben Goodger (Google) wrote:
> "Mojo shell in chromium is only used for Mandoline, and Mandoline only uses
> kiosk_wm, so we name the target window_manager to avoid having to remap on the
> command line."

Done.

https://codereview.chromium.org/1057603003/diff/60001/mojo/services/window_ma...
File mojo/services/window_manager/BUILD.gn (right):

https://codereview.chromium.org/1057603003/diff/60001/mojo/services/window_ma...
mojo/services/window_manager/BUILD.gn:95:
mojo_native_application("window_manager_default") {
On 2015/04/09 23:25:22, Ben Goodger (Google) wrote:
> maybe test_window_manager?

Done.

https://codereview.chromium.org/1057603003/diff/60001/mojo/shell/application_...
File mojo/shell/application_manager/application_manager.cc (right):

https://codereview.chromium.org/1057603003/diff/60001/mojo/shell/application_...
mojo/shell/application_manager/application_manager.cc:153: if
(ConnectToApplicationWithLoader(resolved_url, requestor_url, &services,
On 2015/04/09 23:25:22, Ben Goodger (Google) wrote:
> strictly speaking, do you need any of these changes for this CL or is this
just
> more cleanup? if the latter camp, could we defer this?
> 
> i think the code in the shell executable (i.e. desktop & android subdirs) will
> undoubtedly diverge from the generic mojo shell. i also think this stuff might
> end up changing but by doing this we are explicitly making the mojo shell lib
> source set incompatible with the upstream one. it's not clear to me just yet
> that there isn't an unforkable subset of code here.
> 
> anyway the reason I ask is that there seem to be two things you are doing in
> this cl: 1: making some useful improvements to usability of the shell for
> mandoline and 2: doing some cleanup. i'm wondering if we can split those.

The two things you mention go hand-in-hand (i.e. removing the args code makes
using mandoline's mojo_shell easier to use). i.e. it's simpler to pass arguments
to both mojo apps (i.e. starting url to html_viewer) and also to shared chromium
code that's part of these services (i.e. gl implementation).

[jam, 2015-04-10 00:52:44]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-04-10 00:54:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1057603003/100001

[Ben Goodger (Google), 2015-04-10 00:55:56]

lgtm to keep things moving, let's discuss the divergence aspect asynchronously.

[jam, 2015-04-10 00:58:45]

The CQ bit was unchecked by jam@chromium.org

[jam, 2015-04-10 00:58:53]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2015-04-10 00:59:14]

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

[commit-bot: I haz the power, 2015-04-10 02:30:40]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-04-10 02:31:59]

Patchset 6 (id:??) landed as
https://crrev.com/143eb32f63bd929f7d730970e2477804f318a7eb
Cr-Commit-Position: refs/heads/master@{#324579}

[msw, 2015-04-10 22:45:51]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1082513002/ by msw@chromium.org.

The reason for reverting is: This CL causes problems for application testing:
1) Removing url-mappings breaks the ability to mock out service dependencies of
test applications.
2) We want to connect directly to the test applications specified on the command
line, not mojo:window_manager for apptests.
3) The apptest_runner.py currently uses --args-for, but that doesn't matter so
much...
I've started working around these issues in WIP a CL,
<https://codereview.chromium.org/1070463003/>, but it would be better to not let
this CL bake if we go intend to revert some of its behavior changes..

[jam, 2015-04-13 16:29:01]

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

[jam, 2015-04-13 16:40:21]

On 2015/04/10 22:45:51, msw wrote:
> A revert of this CL (patchset #6 id:100001) has been created in
> https://codereview.chromium.org/1082513002/ by mailto:msw@chromium.org.
> 
> The reason for reverting is: This CL causes problems for application testing:
> 1) Removing url-mappings breaks the ability to mock out service dependencies
of
> test applications.
> 2) We want to connect directly to the test applications specified on the
command
> line, not mojo:window_manager for apptests.
> 3) The apptest_runner.py currently uses --args-for, but that doesn't matter so
> much...
> I've started working around these issues in WIP a CL,
> <https://codereview.chromium.org/1070463003/>, but it would be better to not
let
> this CL bake if we go intend to revert some of its behavior changes..

PTAL at fixes from patchset 7 to 8. The command line is now plumbed correctly to
gtest, so "--gtest_list_tests" works (from desc of cl linked to above). For 1),
this can be done by individual tests in their setup by calling to
Context::url_resolver()->AddURLMapping(). For 2), this was fixed in the latest
patchset.

[msw, 2015-04-13 18:15:46]

lmk when you've looked at the commandline on linux (maybe see my first attempt:
<https://codereview.chromium.org/1070463003/diff/20001/mojo/application/applic...>)

https://codereview.chromium.org/1057603003/diff/140001/mojo/shell/desktop/lau...
File mojo/shell/desktop/launcher_process.cc (right):

https://codereview.chromium.org/1057603003/diff/140001/mojo/shell/desktop/lau...
mojo/shell/desktop/launcher_process.cc:79: // If a mojo app isn't specified
(i.e. for an apptest), run the mojo shell's
I suggested this to Ben and Scott, but Ben thought that this might not work if
we wanted to specify sites/apps on the commandline for mandoline to run and
still be embedded within the window_manager... We hadn't considered checking for
the mojo scheme, so this may be okay, but Ben suggested using a test commandline
switch, so I started with something like line 148 of
<https://codereview.chromium.org/1070463003/diff/20001/mojo/shell/desktop/laun...>,
or maybe we'd want --test-app=<app name/URL>. Also, this removes the ability to
launch multiple apps, but I'm not sure if that's worth saving...

[jam, 2015-04-13 18:36:18]

fixed linux as well per your patch

https://codereview.chromium.org/1057603003/diff/140001/mojo/shell/desktop/lau...
File mojo/shell/desktop/launcher_process.cc (right):

https://codereview.chromium.org/1057603003/diff/140001/mojo/shell/desktop/lau...
mojo/shell/desktop/launcher_process.cc:79: // If a mojo app isn't specified
(i.e. for an apptest), run the mojo shell's
On 2015/04/13 18:15:46, msw wrote:
> I suggested this to Ben and Scott, but Ben thought that this might not work if
> we wanted to specify sites/apps on the commandline for mandoline to run and
> still be embedded within the window_manager... We hadn't considered checking
for
> the mojo scheme, so this may be okay, but Ben suggested using a test
commandline
> switch, so I started with something like line 148 of
>
<https://codereview.chromium.org/1070463003/diff/20001/mojo/shell/desktop/laun...>,

I'm curious what's lacking with specifying the test app (i.e. current approach)?

> or maybe we'd want --test-app=<app name/URL>. Also, this removes the ability
to
> launch multiple apps, but I'm not sure if that's worth saving...

I don't see why we need that for a mandoline..

[msw, 2015-04-13 19:58:33]

https://codereview.chromium.org/1057603003/diff/140001/mojo/shell/desktop/lau...
File mojo/shell/desktop/launcher_process.cc (right):

https://codereview.chromium.org/1057603003/diff/140001/mojo/shell/desktop/lau...
mojo/shell/desktop/launcher_process.cc:79: // If a mojo app isn't specified
(i.e. for an apptest), run the mojo shell's
On 2015/04/13 18:36:17, jam wrote:
> On 2015/04/13 18:15:46, msw wrote:
> > I suggested this to Ben and Scott, but Ben thought that this might not work
if
> > we wanted to specify sites/apps on the commandline for mandoline to run and
> > still be embedded within the window_manager... We hadn't considered checking
> for
> > the mojo scheme, so this may be okay, but Ben suggested using a test
> commandline
> > switch, so I started with something like line 148 of
> >
>
<https://codereview.chromium.org/1070463003/diff/20001/mojo/shell/desktop/laun...>,
> 
> I'm curious what's lacking with specifying the test app (i.e. current
approach)?

The problem with the approach when this CL first landed, was that when trying to
run any apptest, we connect to window_manager and open a native window. That
connection is unnecessary (and a performance burden), and causes the test to
hang after running (we'd need to explicitly close the window).

I think the trouble with inferring whether or not to connect to window_manager
(as in this patch set), is that some command lines would be ambiguous: maybe "$
mojo_shell mojo:foo" should embed the app within the window_manager, while "$
mojo_shell mojo:foo_apptests" should simply run the tests in a headless manner.
Perhaps Ben can more clearly explain the need for an explicit apptest switch...

> > or maybe we'd want --test-app=<app name/URL>. Also, this removes the ability
> to
> > launch multiple apps, but I'm not sure if that's worth saving...
> 
> I don't see why we need that for a mandoline..

We might not need it, at least for now... It can be added later as needed.

https://codereview.chromium.org/1057603003/diff/160001/mojo/application/appli...
File mojo/application/application_test_main_chromium.cc (right):

https://codereview.chromium.org/1057603003/diff/160001/mojo/application/appli...
mojo/application/application_test_main_chromium.cc:17: // TODO(msw): Plumb
relevant command line args before initializing timeouts.
nit: you can remove this TODO now.

[Ben Goodger (Google), 2015-04-13 20:13:30]

One thought that has occurred to me here is that there should perhaps be two
separate executables here. One for the Mandoline browser that defaults to the
window manager and one for tests. The test runner would be more like the mojo
shell today.

Currently there's not quite a clean separation in practice in the shell between
the "service manager/loader" elements and the executable artifacts (e.g. main
function, command line handling etc). It seems like there should be, then there
could be two different entrypoints, one for tests and one for the browser, so
the browser doesn't have to have all the features we might want for a test
runner.

https://codereview.chromium.org/1057603003/diff/140001/mojo/shell/desktop/lau...
File mojo/shell/desktop/launcher_process.cc (right):

https://codereview.chromium.org/1057603003/diff/140001/mojo/shell/desktop/lau...
mojo/shell/desktop/launcher_process.cc:79: // If a mojo app isn't specified
(i.e. for an apptest), run the mojo shell's
I'm OK with not supporting running multiple apps unless that is needed for
testing.

IMO running multiple apps was always a bit dubious. There should be one app that
is the "orchestrator" that manages the lifetimes etc. of the others.

[Ben Goodger (Google), 2015-04-13 23:11:27]

lgtm

[msw, 2015-04-13 23:23:46]

Maybe update the CL description to mention that this will run the first app on
the commandline with a mojo: scheme, or default to mojo:window_manager.

Can you update mojo/tools/apptest_runner.py and mojo/tools/gtest.py to avoid
--args-for, as I did here (or I can do that asap after you land):
https://codereview.chromium.org/1070463003

[jam, 2015-04-14 00:18:07]

On 2015/04/13 23:23:46, msw wrote:
> Maybe update the CL description to mention that this will run the first app on
> the commandline with a mojo: scheme, or default to mojo:window_manager.

Done

> 
> Can you update mojo/tools/apptest_runner.py and mojo/tools/gtest.py to avoid
> --args-for, as I did here (or I can do that asap after you land):
> https://codereview.chromium.org/1070463003

Done, thanks I missed these.

[jam, 2015-04-14 00:18:19]

The CQ bit was checked by jam@chromium.org

[jam, 2015-04-14 00:18:19]

The patchset sent to the CQ was uploaded after l-g-t-m from ben@chromium.org
Link to the patchset: https://codereview.chromium.org/1057603003/#ps170001
(title: "update scripts")

[msw, 2015-04-14 00:20:29]

lgtm

[commit-bot: I haz the power, 2015-04-14 00:21:46]

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

[commit-bot: I haz the power, 2015-04-14 01:13:18]

Committed patchset #10 (id:170001)

[commit-bot: I haz the power, 2015-04-14 01:14:14]

Patchset 10 (id:??) landed as
https://crrev.com/541ee5bc890db30033673826d3ed5c19ecd9d339
Cr-Commit-Position: refs/heads/master@{#324974}