Update mojo shell so that --args-for can be used on android

Update mojo shell so that --args-for can be used on android

This refactor the shell startup to factorize the parsing of --args-for argument
so that it can be reused on android.

It also changes the way android gets its application argument. Instead of
expecting it as a specific intent parameter, it does exactly as the desktop
version and looks for it in the generic argument list.

R=sky@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/4fc210c67386a0700d25a30e1bd7282dfc753ca2

[sky, 2014-12-17 16:44:05]

I'm LGTMing, but I think it would be good to split things out for ease of
testing.

https://codereview.chromium.org/816473002/diff/20001/shell/android/apk/src/or...
File shell/android/apk/src/org/chromium/mojo_shell_apk/MojoMain.java (right):

https://codereview.chromium.org/816473002/diff/20001/shell/android/apk/src/or...
shell/android/apk/src/org/chromium/mojo_shell_apk/MojoMain.java:77: * @return
<code>true</code> if an application has been launcher.
launcher->launched

https://codereview.chromium.org/816473002/diff/20001/shell/android/apk/src/or...
shell/android/apk/src/org/chromium/mojo_shell_apk/MojoMain.java:84: * Adds the
given URL to the set of mojo application to run on start.
application->applications

https://codereview.chromium.org/816473002/diff/20001/shell/android/mojo_main.cc
File shell/android/mojo_main.cc (right):

https://codereview.chromium.org/816473002/diff/20001/shell/android/mojo_main....
shell/android/mojo_main.cc:161: if
(!base::CommandLine::ForCurrentProcess()->GetArgs().size()) {
nit: no {}

https://codereview.chromium.org/816473002/diff/20001/shell/command_line_util.cc
File shell/command_line_util.cc (right):

https://codereview.chromium.org/816473002/diff/20001/shell/command_line_util....
shell/command_line_util.cc:34: return GURL::EmptyGURL();
GURL() is fine here. GURL::EmptyGURL() is only necessary if you were returning a
const GURL&.

https://codereview.chromium.org/816473002/diff/20001/shell/command_line_util....
shell/command_line_util.cc:41:
context->application_manager()->SetArgsForURL(argv, app_url);
Is it expected that you supply the url in argv? I would have though you needed
to do argv.erase(argv.begin()).

https://codereview.chromium.org/816473002/diff/20001/shell/command_line_util....
shell/command_line_util.cc:45: bool IsArgsFor(const std::string& arg,
std::string* value) {
Add a description here. Especially what |value| is set to.

I would be inclined to name this GetArgsForValue().

https://codereview.chromium.org/816473002/diff/20001/shell/command_line_util....
shell/command_line_util.cc:51: std::string argsfor_switch(kArgsForSwitches[i]);
const std::string&

https://codereview.chromium.org/816473002/diff/20001/shell/command_line_util.h
File shell/command_line_util.h (right):

https://codereview.chromium.org/816473002/diff/20001/shell/command_line_util....
shell/command_line_util.h:13: // Apply arguments for an application from a line
with the following format:
WDYT of splitting out the extraction functions so they don't take |context|?
That way you could write a test for them. I'm thinking you keep the functions
you have here, but add additional ones that don't use context.
ApplyApplicationArgs/RunCommandLineApps would could the non-context ones and
apply the results to the context.

https://codereview.chromium.org/816473002/diff/20001/shell/command_line_util....
shell/command_line_util.h:16: void ApplyApplicationArgs(Context* context, const
std::string args);
const std::string&

[qsr, 2014-12-18 14:21:36]

https://codereview.chromium.org/816473002/diff/20001/shell/android/apk/src/or...
File shell/android/apk/src/org/chromium/mojo_shell_apk/MojoMain.java (right):

https://codereview.chromium.org/816473002/diff/20001/shell/android/apk/src/or...
shell/android/apk/src/org/chromium/mojo_shell_apk/MojoMain.java:77: * @return
<code>true</code> if an application has been launcher.
On 2014/12/17 16:44:04, sky wrote:
> launcher->launched

Done.

https://codereview.chromium.org/816473002/diff/20001/shell/android/apk/src/or...
shell/android/apk/src/org/chromium/mojo_shell_apk/MojoMain.java:84: * Adds the
given URL to the set of mojo application to run on start.
On 2014/12/17 16:44:04, sky wrote:
> application->applications

Done.

https://codereview.chromium.org/816473002/diff/20001/shell/android/mojo_main.cc
File shell/android/mojo_main.cc (right):

https://codereview.chromium.org/816473002/diff/20001/shell/android/mojo_main....
shell/android/mojo_main.cc:161: if
(!base::CommandLine::ForCurrentProcess()->GetArgs().size()) {
On 2014/12/17 16:44:04, sky wrote:
> nit: no {}

Done.

https://codereview.chromium.org/816473002/diff/20001/shell/command_line_util.cc
File shell/command_line_util.cc (right):

https://codereview.chromium.org/816473002/diff/20001/shell/command_line_util....
shell/command_line_util.cc:34: return GURL::EmptyGURL();
On 2014/12/17 16:44:05, sky wrote:
> GURL() is fine here. GURL::EmptyGURL() is only necessary if you were returning
a
> const GURL&.

 Done.

 I agree that it is less useful in the current case, but why not to use it
still? At worst it doesn't change anything (if the compiler optimize the
creation/copy into a single creation), at best, it prevents an object creation)

 (Also note that all this code was just copied away)

https://codereview.chromium.org/816473002/diff/20001/shell/command_line_util....
shell/command_line_util.cc:41:
context->application_manager()->SetArgsForURL(argv, app_url);
On 2014/12/17 16:44:05, sky wrote:
> Is it expected that you supply the url in argv? I would have though you needed
> to do argv.erase(argv.begin()).

 Hum... This code has just moved here. Now, I guess we let the url there so that
the first argument is the name of the application, the same way argv[0] is the
name of the application.

https://codereview.chromium.org/816473002/diff/20001/shell/command_line_util....
shell/command_line_util.cc:45: bool IsArgsFor(const std::string& arg,
std::string* value) {
On 2014/12/17 16:44:05, sky wrote:
> Add a description here. Especially what |value| is set to.
> 
> I would be inclined to name this GetArgsForValue().

 Added a description. Renamed this ParseArgsFor

https://codereview.chromium.org/816473002/diff/20001/shell/command_line_util....
shell/command_line_util.cc:51: std::string argsfor_switch(kArgsForSwitches[i]);
On 2014/12/17 16:44:05, sky wrote:
> const std::string&

Done.

https://codereview.chromium.org/816473002/diff/20001/shell/command_line_util.h
File shell/command_line_util.h (right):

https://codereview.chromium.org/816473002/diff/20001/shell/command_line_util....
shell/command_line_util.h:13: // Apply arguments for an application from a line
with the following format:
On 2014/12/17 16:44:05, sky wrote:
> WDYT of splitting out the extraction functions so they don't take |context|?
> That way you could write a test for them. I'm thinking you keep the functions
> you have here, but add additional ones that don't use context.
> ApplyApplicationArgs/RunCommandLineApps would could the non-context ones and
> apply the results to the context.

 I didn't do exactly this, but I refactored this to extract the methods that do
actual work, and added some unit tests, tell me if that's fine with you.

https://codereview.chromium.org/816473002/diff/20001/shell/command_line_util....
shell/command_line_util.h:16: void ApplyApplicationArgs(Context* context, const
std::string args);
On 2014/12/17 16:44:05, sky wrote:
> const std::string&

Done.

[sky, 2014-12-18 17:01:15]

LGTM

https://codereview.chromium.org/816473002/diff/20001/shell/command_line_util.cc
File shell/command_line_util.cc (right):

https://codereview.chromium.org/816473002/diff/20001/shell/command_line_util....
shell/command_line_util.cc:34: return GURL::EmptyGURL();
On 2014/12/18 14:21:35, qsr wrote:
> On 2014/12/17 16:44:05, sky wrote:
> > GURL() is fine here. GURL::EmptyGURL() is only necessary if you were
returning
> a
> > const GURL&.
> 
>  Done.
> 
>  I agree that it is less useful in the current case, but why not to use it
> still?

Mainly because it isn't overly obvious what that function is doing (I had to
look). With GURL() it's pretty clear what is happening.

> At worst it doesn't change anything (if the compiler optimize the
> creation/copy into a single creation), at best, it prevents an object
creation)
> 
>  (Also note that all this code was just copied away)

Actually, I'm not entirely sure which is faster. ::EmptyGURL() does have some
potential overhead associated with it.

https://codereview.chromium.org/816473002/diff/40001/shell/android/mojo_main.cc
File shell/android/mojo_main.cc (right):

https://codereview.chromium.org/816473002/diff/40001/shell/android/mojo_main....
shell/android/mojo_main.cc:96: for (auto& args : parameters) {
nit: no {}

https://codereview.chromium.org/816473002/diff/40001/shell/command_line_util.cc
File shell/command_line_util.cc (right):

https://codereview.chromium.org/816473002/diff/40001/shell/command_line_util....
shell/command_line_util.cc:52: args->erase(std::remove_if(args->begin(),
args->end(), IsEmpty), args->end());
nit: I'm pretty sure you could use mem_fun here and avoid IsEmpty.

https://codereview.chromium.org/816473002/diff/40001/shell/command_line_util.h
File shell/command_line_util.h (right):

https://codereview.chromium.org/816473002/diff/40001/shell/command_line_util....
shell/command_line_util.h:13: // Parce the given arg, looking for an --args-for
switch. If this is not the
parce->parse

https://codereview.chromium.org/816473002/diff/40001/shell/command_line_util....
shell/command_line_util.h:14: // // case, returns |false|. Otherwise, returns
|true| and set |*value| to the
you've got double the comments here: // //

[qsr, 2014-12-18 17:17:56]

https://codereview.chromium.org/816473002/diff/40001/shell/android/mojo_main.cc
File shell/android/mojo_main.cc (right):

https://codereview.chromium.org/816473002/diff/40001/shell/android/mojo_main....
shell/android/mojo_main.cc:96: for (auto& args : parameters) {
On 2014/12/18 17:01:15, sky wrote:
> nit: no {}

Done.

https://codereview.chromium.org/816473002/diff/40001/shell/command_line_util.cc
File shell/command_line_util.cc (right):

https://codereview.chromium.org/816473002/diff/40001/shell/command_line_util....
shell/command_line_util.cc:52: args->erase(std::remove_if(args->begin(),
args->end(), IsEmpty), args->end());
On 2014/12/18 17:01:15, sky wrote:
> nit: I'm pretty sure you could use mem_fun here and avoid IsEmpty.

Done.

https://codereview.chromium.org/816473002/diff/40001/shell/command_line_util.h
File shell/command_line_util.h (right):

https://codereview.chromium.org/816473002/diff/40001/shell/command_line_util....
shell/command_line_util.h:13: // Parce the given arg, looking for an --args-for
switch. If this is not the
On 2014/12/18 17:01:15, sky wrote:
> parce->parse

Done.

https://codereview.chromium.org/816473002/diff/40001/shell/command_line_util....
shell/command_line_util.h:14: // // case, returns |false|. Otherwise, returns
|true| and set |*value| to the
On 2014/12/18 17:01:15, sky wrote:
> you've got double the comments here: // //

Done.

[qsr, 2014-12-18 17:18:10]

Committed patchset #4 (id:60001) manually as
4fc210c67386a0700d25a30e1bd7282dfc753ca2 (presubmit successful).