Simplify resolution of mojo: URLs.

Simplify resolution of mojo: URLs.

Primarily, this eliminates the concept of a "local" mojo: URL by explicitly
mapping said mojo: URL onto a file URL.

BUG=401761
R=aa@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/29e5fc696a6555f7d4629a45ad8c1bad051ac38a

[Nick Bray (chromium), 2015-01-23 00:28:21]

ncbray@chromium.org changed reviewers:
+ aa@chromium.org, abarth@chromium.org

[Nick Bray (chromium), 2015-01-23 00:28:22]

Who would be the best reviewer for this?

Down the road I'll probably move the shell_dir_url onto the context object to
assist resolving relative file:URLs passed on the command line, but that's a
separate change.

[Aaron Boodman, 2015-01-23 00:43:59]

Good reviewers for this change would be me, david moore, qsr, abarth ... maybe
others.

I'm in favor of this, but I think it will prevent a change that abarth and
eseidel wanted to make where we completely remove LocalLoader in
DynamicServiceLoader. There has to be some bootstrapping step that allows us to
get the network service without using the network service. This change works
because we have the separate LocalLoader in DSL that know how to handle file
URLs without using the network service. If we remove that, as abarth/eseidel
have suggested then this change won't work anymore.

[abarth-chromium, 2015-01-23 02:23:15]

ncbray talked a bit in person about removing the special case for file URLs. 
He's pretty convinced that we'll need to have something like mmap for both DSOs
and NEXEs.  To get that to work without a special case for file URLs implies
some notion of a file handle that the network service can return (rather than a
data pipe) and some way to mmap the contents of the handle.  I'm convinced that
we'll need that, but he thinks we should defer solving that problem at this
point.

In that light, this CL LGTM, but it's probably a good idea to have folks who are
more familiar with the details of this code review as well.

https://codereview.chromium.org/868963002/diff/1/shell/context.cc
File shell/context.cc (right):

https://codereview.chromium.org/868963002/diff/1/shell/context.cc#newcode116
shell/context.cc:116: MojoURLResolver *resolver) {
s/base::CommandLine */base::CommandLine* /
Same for MojoURLResolver

https://codereview.chromium.org/868963002/diff/1/shell/context.cc#newcode116
shell/context.cc:116: MojoURLResolver *resolver) {
s/base::CommandLine */base::CommandLine* /
Same for MojoURLResolver

[Aaron Boodman, 2015-01-23 02:34:05]

aa@chromium.org changed reviewers:
+ davemoore@chromium.org
- abarth@chromium.org

[Aaron Boodman, 2015-01-23 02:34:06]

I think that davemoore should also take a look at this as he is working in same
area.

https://codereview.chromium.org/868963002/diff/1/shell/context.cc
File shell/context.cc (right):

https://codereview.chromium.org/868963002/diff/1/shell/context.cc#newcode120
shell/context.cc:120: GURL shell_dir_url =
AddTrailingSlashIfNeeded(FilePathToFileURL(shell_dir));
Is there really ambiguity about whether FilePathToFileURL will return a trailing
slash or not? Seems like it would always do the same thing for directories.

https://codereview.chromium.org/868963002/diff/1/shell/context.cc#newcode126
shell/context.cc:126: base_url = AddTrailingSlashIfNeeded(base_url);
Why add the trailing slash in this case? Seems like the person providing the
switch should add the trailing slash if that's what they want.

https://codereview.chromium.org/868963002/diff/1/shell/context.cc#newcode130
shell/context.cc:130: if (!base_url.is_valid()) {
no braces.

https://codereview.chromium.org/868963002/diff/1/shell/context.cc#newcode185
shell/context.cc:185: if (!ConfigureURLMappings(command_line,
&mojo_url_resolver_)) {
This file uses the no-braces-for-one-line-if style. Please match.

[qsr, 2015-01-23 08:36:15]

I'm pretty sure this is breaking the shell on Android.

On android the network service is special. It is bundled with the application
and it is not next to the executable (for the very good reason that 1) there is
not really an executable, 2) even if the shared library containing the code for
the mojo_shell can be considered the executable, given the layout of an apk, the
network service cannot be next to it. We are using the local resolution to solve
this, so we either need some kind of equivalent, or we need to change
shell/android/mojo_main.cc to explicitly map the network_service to its file
location (as I agree that removing a special case is a good idea, going for 2 is
probably the right solution).

[DaveMoore, 2015-01-23 18:45:07]

Please don't check this in until we decide on a viable path forward for mojo:.
I've sent an email to the team.

[Nick Bray (chromium), 2015-01-23 19:13:47]

On 2015/01/23 18:45:07, DaveMoore wrote:
> Please don't check this in until we decide on a viable path forward for mojo:.
> I've sent an email to the team.

(Currently I am chipping away at unifying Android into the bigger picture.)

This change does not make any of the things you're proposing harder.  Rather, it
makes them all easier.  What are your concerns?  This is a behavior-preserving
transformation trying to put the right information in the right place.

re: aa@ removing local loader - I believe this change is neutral to what you're
proposing.  Making the MojoURLResolver know about the network service is bad
layering (it just maps from one URL to another).  It would seem more reasonable
to bootstrap into the network loader by pre-registering it as a live service or
have the loading logic itself know about bootstrapping into the network loader.

[DaveMoore, 2015-01-23 19:23:51]

I suppose it's true that it's a noop to the changes I want to make. It seems a
bit pointless, since hopefully we're about to change or even eliminate mojo:,
but I remove my objection.

[Nick Bray (chromium), 2015-01-24 01:09:04]

OK, apologies for a little bit of CL creep.  Fixing the Android issue required
storing mojo_shell's directory in the context object, and this ended up
stumbling into a change I had planned to do in a second CL - tweaking how file:
URLs are resolved on the command line.  (See the unittest file for an example.) 
I can split out this change into another CL, but I think leaving it in makes the
treatment of mojo:network_service more principled.  Adam's call.

https://codereview.chromium.org/868963002/diff/1/shell/context.cc
File shell/context.cc (right):

https://codereview.chromium.org/868963002/diff/1/shell/context.cc#newcode116
shell/context.cc:116: MojoURLResolver *resolver) {
On 2015/01/23 02:23:15, abarth wrote:
> s/base::CommandLine */base::CommandLine* /
> Same for MojoURLResolver

Done.

https://codereview.chromium.org/868963002/diff/1/shell/context.cc#newcode116
shell/context.cc:116: MojoURLResolver *resolver) {
On 2015/01/23 02:23:15, abarth wrote:
> s/base::CommandLine */base::CommandLine* /
> Same for MojoURLResolver

Done.

https://codereview.chromium.org/868963002/diff/1/shell/context.cc#newcode120
shell/context.cc:120: GURL shell_dir_url =
AddTrailingSlashIfNeeded(FilePathToFileURL(shell_dir));
On 2015/01/23 02:34:06, Aaron Boodman wrote:
> Is there really ambiguity about whether FilePathToFileURL will return a
trailing
> slash or not? Seems like it would always do the same thing for directories.

I believe it never returns a trailing slash.  There will still be ambiguity when
dealing with user input, and I'd be inclined to sanitize even unambiguous input
for self-similarity.  I see virtue in your question, however.  I could turn it
into a straight-up append, but with a mild preference not to.  What's your
inclination?

https://codereview.chromium.org/868963002/diff/1/shell/context.cc#newcode126
shell/context.cc:126: base_url = AddTrailingSlashIfNeeded(base_url);
On 2015/01/23 02:34:06, Aaron Boodman wrote:
> Why add the trailing slash in this case? Seems like the person providing the
> switch should add the trailing slash if that's what they want.

Ther difference is subtle:
GURL("http://foo/bar").Resolve("baz") => "http://foo/baz"
but
GURL("http://foo/bar/").Resolve("baz") => "http://foo/bar/baz"

It is almost certain the user does not intend the first, and if the user makes
that mistake the resulting behavior may be subtle an confounding.  Forcing the
trailing slash for something that should always be a directory is a good safety
measure.

https://codereview.chromium.org/868963002/diff/1/shell/context.cc#newcode130
shell/context.cc:130: if (!base_url.is_valid()) {
On 2015/01/23 02:34:06, Aaron Boodman wrote:
> no braces.

Sad panda about this style - goto fail, and all that, but you are correct. :/

https://codereview.chromium.org/868963002/diff/1/shell/context.cc#newcode185
shell/context.cc:185: if (!ConfigureURLMappings(command_line,
&mojo_url_resolver_)) {
On 2015/01/23 02:34:06, Aaron Boodman wrote:
> This file uses the no-braces-for-one-line-if style. Please match.

Done.

[Nick Bray (chromium), 2015-01-24 17:10:24]

Hold off on reviewing this.  On reflection, I made a subtle design mistake. 
Bah.

[Nick Bray (chromium), 2015-01-26 19:22:56]

Fixed.  PTAL.

[Aaron Boodman, 2015-01-27 00:37:13]

sorry for the review latency, this slipped off my todo list. Please ping me next
time if I take so long.

https://codereview.chromium.org/868963002/diff/1/shell/context.cc
File shell/context.cc (right):

https://codereview.chromium.org/868963002/diff/1/shell/context.cc#newcode126
shell/context.cc:126: base_url = AddTrailingSlashIfNeeded(base_url);
On 2015/01/24 01:09:04, Nick Bray (chromium) wrote:
> On 2015/01/23 02:34:06, Aaron Boodman wrote:
> > Why add the trailing slash in this case? Seems like the person providing the
> > switch should add the trailing slash if that's what they want.
> 
> Ther difference is subtle:
> GURL("http://foo/bar").Resolve("baz") => "http://foo/baz"
> but
> GURL("http://foo/bar/").Resolve("baz") => "http://foo/bar/baz"
> 
> It is almost certain the user does not intend the first, and if the user makes
> that mistake the resulting behavior may be subtle an confounding.  Forcing the
> trailing slash for something that should always be a directory is a good
safety
> measure.

In that case, I think that we should ensure the caller does *not* add a trailing
slash, and spit out an error if they do. Then always add one.

https://codereview.chromium.org/868963002/diff/80001/shell/context.h
File shell/context.h (right):

https://codereview.chromium.org/868963002/diff/80001/shell/context.h#newcode28
shell/context.h:28: void SetShellFileRoot(const base::FilePath& path);
Maybe a better name would be: SetMojoURLRoot?

https://codereview.chromium.org/868963002/diff/80001/shell/context.h#newcode29
shell/context.h:29: GURL ResolveShellFileURL(const std::string& path);
These two methods need comments.

[Nick Bray (chromium), 2015-01-27 01:44:35]

https://codereview.chromium.org/868963002/diff/1/shell/context.cc
File shell/context.cc (right):

https://codereview.chromium.org/868963002/diff/1/shell/context.cc#newcode126
shell/context.cc:126: base_url = AddTrailingSlashIfNeeded(base_url);
On 2015/01/27 00:37:13, Aaron Boodman wrote:
> On 2015/01/24 01:09:04, Nick Bray (chromium) wrote:
> > On 2015/01/23 02:34:06, Aaron Boodman wrote:
> > > Why add the trailing slash in this case? Seems like the person providing
the
> > > switch should add the trailing slash if that's what they want.
> > 
> > Ther difference is subtle:
> > GURL("http://foo/bar").Resolve("baz") => "http://foo/baz"
> > but
> > GURL("http://foo/bar/").Resolve("baz") => "http://foo/bar/baz"
> > 
> > It is almost certain the user does not intend the first, and if the user
makes
> > that mistake the resulting behavior may be subtle an confounding.  Forcing
the
> > trailing slash for something that should always be a directory is a good
> safety
> > measure.
> 
> In that case, I think that we should ensure the caller does *not* add a
trailing
> slash, and spit out an error if they do. Then always add one.

Requiring no slash will interact badly with tab completion.  That's the next
step - make mojo_shell behave as expected on the command line.
See: https://codereview.chromium.org/873253003/
This will make it easier to point mojo_shell at .nexe files while I bootstrap.

I tried doing it the other way - requiring a slash - but there are a bunch of
tests that pass in URLs without trailing slashes.  After changing a few I
backtracked and gave up for this CL.  This sort of interface change needs to be
done in a separate CL.

Also, either way, mojo_shell is pretty crappy about giving user feedback. 
Establishing a pattern for user feedback would be  another yak shave - likely
all the command line parameters would need to be interpreted, massaged, and
shoved into a config structure up front.

So, damned if you do, damned if you don't.  I think things are the way the are
for a reason. :)

https://codereview.chromium.org/868963002/diff/80001/shell/context.h
File shell/context.h (right):

https://codereview.chromium.org/868963002/diff/80001/shell/context.h#newcode28
shell/context.h:28: void SetShellFileRoot(const base::FilePath& path);
On 2015/01/27 00:37:13, Aaron Boodman wrote:
> Maybe a better name would be: SetMojoURLRoot?

I'll comment this, but the fact it is the _file_ root is quite important.  (It's
used to resolving things installed along with mojo_shell.)

https://codereview.chromium.org/868963002/diff/80001/shell/context.h#newcode29
shell/context.h:29: GURL ResolveShellFileURL(const std::string& path);
On 2015/01/27 00:37:13, Aaron Boodman wrote:
> These two methods need comments.

Done.  <troll>But you could say that's against the style of this particular
file.</troll>

[Aaron Boodman, 2015-01-27 07:29:54]

lgtm

https://codereview.chromium.org/868963002/diff/1/shell/context.cc
File shell/context.cc (right):

https://codereview.chromium.org/868963002/diff/1/shell/context.cc#newcode126
shell/context.cc:126: base_url = AddTrailingSlashIfNeeded(base_url);
On 2015/01/27 01:44:35, Nick Bray (chromium) wrote:
> On 2015/01/27 00:37:13, Aaron Boodman wrote:
> > On 2015/01/24 01:09:04, Nick Bray (chromium) wrote:
> > > On 2015/01/23 02:34:06, Aaron Boodman wrote:
> > > > Why add the trailing slash in this case? Seems like the person providing
> the
> > > > switch should add the trailing slash if that's what they want.
> > > 
> > > Ther difference is subtle:
> > > GURL("http://foo/bar").Resolve("baz") => "http://foo/baz"
> > > but
> > > GURL("http://foo/bar/").Resolve("baz") => "http://foo/bar/baz"
> > > 
> > > It is almost certain the user does not intend the first, and if the user
> makes
> > > that mistake the resulting behavior may be subtle an confounding.  Forcing
> the
> > > trailing slash for something that should always be a directory is a good
> > safety
> > > measure.
> > 
> > In that case, I think that we should ensure the caller does *not* add a
> trailing
> > slash, and spit out an error if they do. Then always add one.
> 
> Requiring no slash will interact badly with tab completion.  That's the next
> step - make mojo_shell behave as expected on the command line.
> See: https://codereview.chromium.org/873253003/
> This will make it easier to point mojo_shell at .nexe files while I bootstrap.
> 
> I tried doing it the other way - requiring a slash - but there are a bunch of
> tests that pass in URLs without trailing slashes.  After changing a few I
> backtracked and gave up for this CL.  This sort of interface change needs to
be
> done in a separate CL.

OK, I was initially going to suggest requiring a slash, but thought it would be
nicer to have to type less.

Either way, but I do think it would be a good change (later).

> Also, either way, mojo_shell is pretty crappy about giving user feedback. 

Agree.

> Establishing a pattern for user feedback would be  another yak shave - likely
> all the command line parameters would need to be interpreted, massaged, and
> shoved into a config structure up front.

Agree.

> So, damned if you do, damned if you don't.  I think things are the way the are
> for a reason. :)

The reason is just that it hasn't been fixed yet.

https://codereview.chromium.org/868963002/diff/80001/shell/context.h
File shell/context.h (right):

https://codereview.chromium.org/868963002/diff/80001/shell/context.h#newcode29
shell/context.h:29: GURL ResolveShellFileURL(const std::string& path);
On 2015/01/27 01:44:35, Nick Bray (chromium) wrote:
> On 2015/01/27 00:37:13, Aaron Boodman wrote:
> > These two methods need comments.
> 
> Done.  <troll>But you could say that's against the style of this particular
> file.</troll>

True, but the reason I brought this up in particular was that it wasn't at all
clear to me what a "shell file URL" was, whereas the other members of this class
are more self-explanatory, at least to me.

[Nick Bray (chromium), 2015-01-27 16:36:25]

https://codereview.chromium.org/868963002/diff/80001/shell/context.h
File shell/context.h (right):

https://codereview.chromium.org/868963002/diff/80001/shell/context.h#newcode29
shell/context.h:29: GURL ResolveShellFileURL(const std::string& path);
On 2015/01/27 07:29:54, Aaron Boodman wrote:
> On 2015/01/27 01:44:35, Nick Bray (chromium) wrote:
> > On 2015/01/27 00:37:13, Aaron Boodman wrote:
> > > These two methods need comments.
> > 
> > Done.  <troll>But you could say that's against the style of this particular
> > file.</troll>
> 
> True, but the reason I brought this up in particular was that it wasn't at all
> clear to me what a "shell file URL" was, whereas the other members of this
class
> are more self-explanatory, at least to me.

More comments are always better, particularly when a reviewer asks for
clarification.  Pure trolling on my part.

[Nick Bray (chromium), 2015-01-27 20:21:37]

Committed patchset #6 (id:100001) manually as
29e5fc696a6555f7d4629a45ad8c1bad051ac38a (presubmit successful).