Simple multi-url support for mojo apps

Simple multi-url support for mojo apps.

This patch makes it so that querystrings are no longer considered part of a mojo application's identity.

So if you connect to http://a?foo, then http://a?bar, you'll end up talking to the same application, just two different connections.

The URL of each connection is sent to the app via a new param in AcceptConnection().

This patch also adds some app tests that exercise http app loading, which wasn't tested before.

R=qsr@chromium.org

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

[Aaron Boodman, 2015-02-27 13:51:18]

Patchset #1 (id:1) has been deleted

[Aaron Boodman, 2015-02-27 13:51:25]

Patchset #1 (id:20001) has been deleted

[Aaron Boodman, 2015-02-27 13:51:31]

Patchset #1 (id:40001) has been deleted

[Aaron Boodman, 2015-02-27 14:03:22]

aa@chromium.org changed reviewers:
+ qsr@chromium.org

[qsr, 2015-02-27 15:01:41]

First pass, as we discussed.

https://codereview.chromium.org/943053003/diff/80001/examples/forwarding_cont...
File examples/forwarding_content_handler/forwarding_content_handler.cc (right):

https://codereview.chromium.org/943053003/diff/80001/examples/forwarding_cont...
examples/forwarding_content_handler/forwarding_content_handler.cc:25:
std::string target_url)
I don't understand this change.

This content handler expect to read a txt file that contains the URL of the app
to connect to.

You seems to have just ignore this parameter altogether.

https://codereview.chromium.org/943053003/diff/80001/shell/application_manage...
File shell/application_manager/application_manager.cc (right):

https://codereview.chromium.org/943053003/diff/80001/shell/application_manage...
shell/application_manager/application_manager.cc:128: if
(ConnectToRunningApplication(requested_url, mapped_url, requestor_url,
I think the URL that the application see should be the URL after all
mapping/redirect etc. So we should not pass requested_url here but mapped_url
with the needed query string.

https://codereview.chromium.org/943053003/diff/80001/shell/application_manage...
shell/application_manager/application_manager.cc:181: ShellImpl* shell_impl =
GetShellImpl(application_url);
Any reason you do not always strip the query here?

https://codereview.chromium.org/943053003/diff/80001/shell/application_manage...
shell/application_manager/application_manager.cc:241: const GURL& resolved_url, 
// TODO(aa): Remove
I think that it is requested_url that we can do without. We just have to make
sure resolved_url is the real url of the shell with the query path.

https://codereview.chromium.org/943053003/diff/80001/shell/application_manage...
File shell/application_manager/shell_impl.h (right):

https://codereview.chromium.org/943053003/diff/80001/shell/application_manage...
shell/application_manager/shell_impl.h:19: enum QueryHandlingBehavior {
Can you comment on this, and on what behavior it does change.

https://codereview.chromium.org/943053003/diff/80001/shell/application_manage...
shell/application_manager/shell_impl.h:42: bool handles_query() const { return
handles_query_; }
As discussed offline, this is not necessary.

https://codereview.chromium.org/943053003/diff/80001/shell/shell_apptest.cc
File shell/shell_apptest.cc (right):

https://codereview.chromium.org/943053003/diff/80001/shell/shell_apptest.cc#n...
shell/shell_apptest.cc:138: // segfault+smashed stack. I suspect something in
HttpServer having to do with
Collin is using the http server with headers and didn't have the same issue.
There is an issue on the network service when it does shutdown, but it should
never happen in this case...

[Aaron Boodman, 2015-02-27 16:54:45]

ptal

haven't actually exercised the new url field yet though.

https://codereview.chromium.org/943053003/diff/80001/examples/forwarding_cont...
File examples/forwarding_content_handler/forwarding_content_handler.cc (right):

https://codereview.chromium.org/943053003/diff/80001/examples/forwarding_cont...
examples/forwarding_content_handler/forwarding_content_handler.cc:25:
std::string target_url)
On 2015/02/27 15:01:40, qsr wrote:
> I don't understand this change.
> 
> This content handler expect to read a txt file that contains the URL of the
app
> to connect to.
> 
> You seems to have just ignore this parameter altogether.

Whoopsie. fixed.

https://codereview.chromium.org/943053003/diff/80001/shell/application_manage...
File shell/application_manager/application_manager.cc (right):

https://codereview.chromium.org/943053003/diff/80001/shell/application_manage...
shell/application_manager/application_manager.cc:128: if
(ConnectToRunningApplication(requested_url, mapped_url, requestor_url,
On 2015/02/27 15:01:41, qsr wrote:
> I think the URL that the application see should be the URL after all
> mapping/redirect etc. So we should not pass requested_url here but mapped_url
> with the needed query string.

Done.

https://codereview.chromium.org/943053003/diff/80001/shell/application_manage...
shell/application_manager/application_manager.cc:181: ShellImpl* shell_impl =
GetShellImpl(application_url);
On 2015/02/27 15:01:41, qsr wrote:
> Any reason you do not always strip the query here?

Done.

https://codereview.chromium.org/943053003/diff/80001/shell/application_manage...
shell/application_manager/application_manager.cc:241: const GURL& resolved_url, 
// TODO(aa): Remove
On 2015/02/27 15:01:41, qsr wrote:
> I think that it is requested_url that we can do without. We just have to make
> sure resolved_url is the real url of the shell with the query path.

Done.

https://codereview.chromium.org/943053003/diff/80001/shell/application_manage...
File shell/application_manager/shell_impl.h (right):

https://codereview.chromium.org/943053003/diff/80001/shell/application_manage...
shell/application_manager/shell_impl.h:19: enum QueryHandlingBehavior {
On 2015/02/27 15:01:41, qsr wrote:
> Can you comment on this, and on what behavior it does change.

removed.

https://codereview.chromium.org/943053003/diff/80001/shell/application_manage...
shell/application_manager/shell_impl.h:42: bool handles_query() const { return
handles_query_; }
On 2015/02/27 15:01:41, qsr wrote:
> As discussed offline, this is not necessary.

agree, done.

[qsr, 2015-02-27 17:15:59]

https://codereview.chromium.org/943053003/diff/100001/mojo/public/dart/src/ap...
File mojo/public/dart/src/application.dart (right):

https://codereview.chromium.org/943053003/diff/100001/mojo/public/dart/src/ap...
mojo/public/dart/src/application.dart:38:
_application._acceptConnection(requestorUrl, services,  exposedServices);
Don't know if you want to solve this now, but this seems wrong. We should also
expose requested_url to dart.

https://codereview.chromium.org/943053003/diff/100001/mojo/public/interfaces/...
File mojo/public/interfaces/application/application.mojom (right):

https://codereview.chromium.org/943053003/diff/100001/mojo/public/interfaces/...
mojo/public/interfaces/application/application.mojom:46: // found at, if the
application is handling multiple URLs.
Did you try putting resolved_url first, then annotation each thing with
[MinVersion=X] where X is 1,2 3 for the existing parameter, and 4 for the last
one.

I think this should be equivalent as this one on the write, but will generate
the code in the right order.

https://codereview.chromium.org/943053003/diff/100001/services/window_manager...
File services/window_manager/window_manager_app.cc (right):

https://codereview.chromium.org/943053003/diff/100001/services/window_manager...
services/window_manager/window_manager_app.cc:144: ApplicationConnection*
connection,
Indenttion seems wrong. Did you run git cl format?

https://codereview.chromium.org/943053003/diff/100001/shell/application_manag...
File shell/application_manager/application_manager_unittest.cc (right):

https://codereview.chromium.org/943053003/diff/100001/shell/application_manag...
shell/application_manager/application_manager_unittest.cc:694:
TEST_F(ApplicationManagerTest, MappedURLsShouldNotCauseDuplicateLoad) {
Don't you want to add test, checking that url with query string are handled
correctly?

https://codereview.chromium.org/943053003/diff/100001/shell/shell_apptest.cc
File shell/shell_apptest.cc (right):

https://codereview.chromium.org/943053003/diff/100001/shell/shell_apptest.cc#...
shell/shell_apptest.cc:30: }
// namespace

https://codereview.chromium.org/943053003/diff/100001/shell/shell_apptest.cc#...
shell/shell_apptest.cc:37: app_path_ =
app_path_.DirName().Append("pingable_app.mojo");
I think you will have issue on android. You might be better connecting to this
using relative URL from where you are yourself, or mojo: url.

Which goes to the next thing, this will probably not work for file URL (or maybe
even for our test server), so as a suggestion should our fetch strategy retry
fetching without the query string if the request failed with. It would allow to
use query string whithout needing any special configuration on the server.

https://codereview.chromium.org/943053003/diff/100001/shell/test/pingable.mojom
File shell/test/pingable.mojom (right):

https://codereview.chromium.org/943053003/diff/100001/shell/test/pingable.moj...
shell/test/pingable.mojom:8: Ping(string message) => (string app_url, string
message);
Even if you do not want to write the apptest test yet, can you add a
requested_url response to this?

[qsr, 2015-02-27 17:17:43]

https://codereview.chromium.org/943053003/diff/100001/shell/application_manag...
File shell/application_manager/application_manager.cc (right):

https://codereview.chromium.org/943053003/diff/100001/shell/application_manag...
shell/application_manager/application_manager.cc:127: GURL mapped_url =
delegate_->ResolveMappings(requested_url);
Don't you need to modify those methods to keep the query string, or does this
already works perfectly fine?

[Aaron Boodman, 2015-02-28 19:08:23]

https://codereview.chromium.org/943053003/diff/100001/mojo/public/dart/src/ap...
File mojo/public/dart/src/application.dart (right):

https://codereview.chromium.org/943053003/diff/100001/mojo/public/dart/src/ap...
mojo/public/dart/src/application.dart:38:
_application._acceptConnection(requestorUrl, services,  exposedServices);
On 2015/02/27 17:15:58, qsr wrote:
> Don't know if you want to solve this now, but this seems wrong. We should also
> expose requested_url to dart.

Agree, but I will leave that to the dartisans.

https://codereview.chromium.org/943053003/diff/100001/mojo/public/interfaces/...
File mojo/public/interfaces/application/application.mojom (right):

https://codereview.chromium.org/943053003/diff/100001/mojo/public/interfaces/...
mojo/public/interfaces/application/application.mojom:46: // found at, if the
application is handling multiple URLs.
On 2015/02/27 17:15:58, qsr wrote:
> Did you try putting resolved_url first, then annotation each thing with
> [MinVersion=X] where X is 1,2 3 for the existing parameter, and 4 for the last
> one.
> 
> I think this should be equivalent as this one on the write, but will generate
> the code in the right order.

I tried, it doesn't seem to be fully implemented. Results in validation errors.
I also talked to Yuzhu and he said there was no way to get the params in right
order right now.

https://codereview.chromium.org/943053003/diff/100001/services/window_manager...
File services/window_manager/window_manager_app.cc (right):

https://codereview.chromium.org/943053003/diff/100001/services/window_manager...
services/window_manager/window_manager_app.cc:144: ApplicationConnection*
connection,
On 2015/02/27 17:15:58, qsr wrote:
> Indenttion seems wrong. Did you run git cl format?

Done.

https://codereview.chromium.org/943053003/diff/100001/shell/application_manag...
File shell/application_manager/application_manager.cc (right):

https://codereview.chromium.org/943053003/diff/100001/shell/application_manag...
shell/application_manager/application_manager.cc:127: GURL mapped_url =
delegate_->ResolveMappings(requested_url);
On 2015/02/27 17:17:43, qsr wrote:
> Don't you need to modify those methods to keep the query string, or does this
> already works perfectly fine?

I don't follow you.

https://codereview.chromium.org/943053003/diff/100001/shell/application_manag...
File shell/application_manager/application_manager_unittest.cc (right):

https://codereview.chromium.org/943053003/diff/100001/shell/application_manag...
shell/application_manager/application_manager_unittest.cc:694:
TEST_F(ApplicationManagerTest, MappedURLsShouldNotCauseDuplicateLoad) {
On 2015/02/27 17:15:58, qsr wrote:
> Don't you want to add test, checking that url with query string are handled
> correctly?

I did test that in the apptest. This file is really more about testing the
custom loaders. I'm not sure what the right behavior is there:

if I set a custom loader for test:test, should it fire for test:test?monkey ?

I guess so... but the loaders are already so gnarly and hateful, I'm kind of
loathe to add more features to them. They should die anyway.

In any case, feels like separate change.

https://codereview.chromium.org/943053003/diff/100001/shell/shell_apptest.cc
File shell/shell_apptest.cc (right):

https://codereview.chromium.org/943053003/diff/100001/shell/shell_apptest.cc#...
shell/shell_apptest.cc:37: app_path_ =
app_path_.DirName().Append("pingable_app.mojo");
On 2015/02/27 17:15:58, qsr wrote:
> I think you will have issue on android. You might be better connecting to this
> using relative URL from where you are yourself, or mojo: url.

I'm not sure what that would mean. I'm trying to test that http-served apps
work. That means I need to read the file from somewhere to actually serve it.

> Which goes to the next thing, this will probably not work for file URL (or
maybe
> even for our test server), so as a suggestion should our fetch strategy retry
> fetching without the query string if the request failed with. It would allow
to
> use query string whithout needing any special configuration on the server.

It can definitely work with file URLs. The LocalFetcher just needs to strip the
querystring before fetching the file. I added this and a test to exercise it.

Typically server developers have to go out of their way to disallow
querystrings, so I'm not really convinced by the other part of your argument. I
definitely don't think we should magically retry connections behind the scenes
with different URLs though -- the URLs are the contract between an app and its
clients. The app developer should know whether querystrings are supported on his
server :). If a client requests a URL and it doesn't load, we should faithfully
report that back to the client.

https://codereview.chromium.org/943053003/diff/100001/shell/test/pingable.mojom
File shell/test/pingable.mojom (right):

https://codereview.chromium.org/943053003/diff/100001/shell/test/pingable.moj...
shell/test/pingable.mojom:8: Ping(string message) => (string app_url, string
message);
On 2015/02/27 17:15:59, qsr wrote:
> Even if you do not want to write the apptest test yet, can you add a
> requested_url response to this?

Added tests.

[Aaron Boodman, 2015-02-28 19:12:23]

Patchset #4 (id:120001) has been deleted

[Aaron Boodman, 2015-02-28 19:12:33]

Patchset #4 (id:140001) has been deleted

[Aaron Boodman, 2015-02-28 19:22:10]

Patchset #4 (id:160001) has been deleted

[qsr, 2015-03-02 08:51:31]

https://codereview.chromium.org/943053003/diff/100001/shell/application_manag...
File shell/application_manager/application_manager.cc (right):

https://codereview.chromium.org/943053003/diff/100001/shell/application_manag...
shell/application_manager/application_manager.cc:127: GURL mapped_url =
delegate_->ResolveMappings(requested_url);
On 2015/02/28 19:08:23, Aaron Boodman wrote:
> On 2015/02/27 17:17:43, qsr wrote:
> > Don't you need to modify those methods to keep the query string, or does
this
> > already works perfectly fine?
> 
> I don't follow you.

 I think URLResolver::ApplyMappings needs to be updated. Right now it looks if
the URL is in its known mapping, and I think it should do the match without the
query string, and if it found one, should add the query string on it.

 Also, URLResolver::ResolveMojoURL seens correct, but you might want to check if
you agree with me.

https://codereview.chromium.org/943053003/diff/100001/shell/shell_apptest.cc
File shell/shell_apptest.cc (right):

https://codereview.chromium.org/943053003/diff/100001/shell/shell_apptest.cc#...
shell/shell_apptest.cc:37: app_path_ =
app_path_.DirName().Append("pingable_app.mojo");
On 2015/02/28 19:08:23, Aaron Boodman wrote:
> On 2015/02/27 17:15:58, qsr wrote:
> > I think you will have issue on android. You might be better connecting to
this
> > using relative URL from where you are yourself, or mojo: url.
> 
> I'm not sure what that would mean. I'm trying to test that http-served apps
> work. That means I need to read the file from somewhere to actually serve it.
> 
> > Which goes to the next thing, this will probably not work for file URL (or
> maybe
> > even for our test server), so as a suggestion should our fetch strategy
retry
> > fetching without the query string if the request failed with. It would allow
> to
> > use query string whithout needing any special configuration on the server.
> 
> It can definitely work with file URLs. The LocalFetcher just needs to strip
the
> querystring before fetching the file. I added this and a test to exercise it.
> 
> Typically server developers have to go out of their way to disallow
> querystrings, so I'm not really convinced by the other part of your argument.
I
> definitely don't think we should magically retry connections behind the scenes
> with different URLs though -- the URLs are the contract between an app and its
> clients. The app developer should know whether querystrings are supported on
his
> server :). If a client requests a URL and it doesn't load, we should
faithfully
> report that back to the client.

 Ok.

 For android, the issue is that you cannot use
PathService::Get(base::FILE_MODULE, )) and hope to find something that you build
there, because you are running on the device.

 You should do what we do with icu and bundle the app data in the apptest
executable.

https://codereview.chromium.org/943053003/diff/180001/mojo/public/dart/src/ap...
File mojo/public/dart/src/application.dart (right):

https://codereview.chromium.org/943053003/diff/180001/mojo/public/dart/src/ap...
mojo/public/dart/src/application.dart:13: core.MojoMessagePipeEndpoint endpoint)
{
Any reason for all the formatting changes, is this some automatic tool work?

https://codereview.chromium.org/943053003/diff/180001/shell/application_manag...
File shell/application_manager/application_manager.cc (right):

https://codereview.chromium.org/943053003/diff/180001/shell/application_manag...
shell/application_manager/application_manager.cc:220:
shell->InitializeApplication(GetArgsForURL(original_url));
Should that be app_url? We only initialize an app once, so arguments should
probably given with the app identity, not the particular url used to connect to
it the first time.

[Aaron Boodman, 2015-03-02 15:36:36]

https://codereview.chromium.org/943053003/diff/100001/shell/application_manag...
File shell/application_manager/application_manager.cc (right):

https://codereview.chromium.org/943053003/diff/100001/shell/application_manag...
shell/application_manager/application_manager.cc:127: GURL mapped_url =
delegate_->ResolveMappings(requested_url);
On 2015/03/02 08:51:31, qsr wrote:
> On 2015/02/28 19:08:23, Aaron Boodman wrote:
> > On 2015/02/27 17:17:43, qsr wrote:
> > > Don't you need to modify those methods to keep the query string, or does
> this
> > > already works perfectly fine?
> > 
> > I don't follow you.
> 
>  I think URLResolver::ApplyMappings needs to be updated. Right now it looks if
> the URL is in its known mapping, and I think it should do the match without
the
> query string, and if it found one, should add the query string on it.
> 
>  Also, URLResolver::ResolveMojoURL seens correct, but you might want to check
if
> you agree with me.

Perhaps. I think that should be a separate change though. The existing cl allows
people's scripts to continue working as they are.

https://codereview.chromium.org/943053003/diff/100001/shell/shell_apptest.cc
File shell/shell_apptest.cc (right):

https://codereview.chromium.org/943053003/diff/100001/shell/shell_apptest.cc#...
shell/shell_apptest.cc:37: app_path_ =
app_path_.DirName().Append("pingable_app.mojo");
On 2015/03/02 08:51:31, qsr wrote:
> On 2015/02/28 19:08:23, Aaron Boodman wrote:
> > On 2015/02/27 17:15:58, qsr wrote:
> > > I think you will have issue on android. You might be better connecting to
> this
> > > using relative URL from where you are yourself, or mojo: url.
> > 
> > I'm not sure what that would mean. I'm trying to test that http-served apps
> > work. That means I need to read the file from somewhere to actually serve
it.
> > 
> > > Which goes to the next thing, this will probably not work for file URL (or
> > maybe
> > > even for our test server), so as a suggestion should our fetch strategy
> retry
> > > fetching without the query string if the request failed with. It would
allow
> > to
> > > use query string whithout needing any special configuration on the server.
> > 
> > It can definitely work with file URLs. The LocalFetcher just needs to strip
> the
> > querystring before fetching the file. I added this and a test to exercise
it.
> > 
> > Typically server developers have to go out of their way to disallow
> > querystrings, so I'm not really convinced by the other part of your
argument.
> I
> > definitely don't think we should magically retry connections behind the
scenes
> > with different URLs though -- the URLs are the contract between an app and
its
> > clients. The app developer should know whether querystrings are supported on
> his
> > server :). If a client requests a URL and it doesn't load, we should
> faithfully
> > report that back to the client.
> 
>  Ok.
> 
>  For android, the issue is that you cannot use
> PathService::Get(base::FILE_MODULE, )) and hope to find something that you
build
> there, because you are running on the device.
> 
>  You should do what we do with icu and bundle the app data in the apptest
> executable.

I will look at this today. However, as it is my last day, and I would like to
land what I have, I have uploaded a change that disables the tests that rely on
the data on Android. Can I get an LGTM on this?

https://codereview.chromium.org/943053003/diff/180001/shell/application_manag...
File shell/application_manager/application_manager.cc (right):

https://codereview.chromium.org/943053003/diff/180001/shell/application_manag...
shell/application_manager/application_manager.cc:220:
shell->InitializeApplication(GetArgsForURL(original_url));
On 2015/03/02 08:51:31, qsr wrote:
> Should that be app_url? We only initialize an app once, so arguments should
> probably given with the app identity, not the particular url used to connect
to
> it the first time.

Yes it should be app_url. Even before this change using the requested_url was
weird (because you can end up with different args depending on which alias you
started with).

But again, separate change :).

[qsr, 2015-03-02 15:50:24]

lgtm

[Aaron Boodman, 2015-03-02 18:05:39]

Committed patchset #6 (id:220001) manually as
b34bcbff38cb0151c7c0048c0d46b638bdc0074e.

[jamesr, 2015-03-05 20:23:59]

jamesr@chromium.org changed reviewers:
+ jamesr@chromium.org

[jamesr, 2015-03-05 20:24:01]

https://codereview.chromium.org/943053003/diff/220001/mojo/public/python/mojo...
File mojo/public/python/mojo_application/application_impl.py (right):

https://codereview.chromium.org/943053003/diff/220001/mojo/public/python/mojo...
mojo/public/python/mojo_application/application_impl.py:30: resolved_url):
pylint complains that this is unused when I upload unrelated patches:

Running presubmit upload checks ...

** Presubmit Warnings **
Pylint (111 files) (9.69s) failed
************* Module mojo_application.application_impl
W: 30,23: Unused argument 'resolved_url' (unused-argument)

What's the right way to silence this?  Should I simply omit this param or give
it a special name?

[qsr, 2015-03-06 16:46:35]

On 2015/03/05 20:24:01, jamesr wrote:
>
https://codereview.chromium.org/943053003/diff/220001/mojo/public/python/mojo...
> File mojo/public/python/mojo_application/application_impl.py (right):
> 
>
https://codereview.chromium.org/943053003/diff/220001/mojo/public/python/mojo...
> mojo/public/python/mojo_application/application_impl.py:30: resolved_url):
> pylint complains that this is unused when I upload unrelated patches:
> 
> Running presubmit upload checks ...
> 
> ** Presubmit Warnings **
> Pylint (111 files) (9.69s) failed
> ************* Module mojo_application.application_impl
> W: 30,23: Unused argument 'resolved_url' (unused-argument)
> 
> What's the right way to silence this?  Should I simply omit this param or give
> it a special name?

Fixed by https://codereview.chromium.org/988653002/. Thanks