Move fetching logic out of ApplicationManager, eliminate url mappings.

Move fetching logic out of ApplicationManager, eliminate url mappings.

BUG=

Committed: https://crrev.com/7134773ceeef31531b84818158902e6d7c74f110
Cr-Commit-Position: refs/heads/master@{#348659}

Committed: https://crrev.com/c7ef4d2131963307ea537fc269bb5acee9383de2
Cr-Commit-Position: refs/heads/master@{#348954}

Committed: https://crrev.com/f4e3b26f16471576ed034e74d863ec6e347b0417
Cr-Commit-Position: refs/heads/master@{#349035}

[Ben Goodger (Google), 2015-09-11 22:41:18]

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

[sky, 2015-09-11 23:05:27]

LGTM

https://codereview.chromium.org/1342503003/diff/60001/mojo/fetcher/base_appli...
File mojo/fetcher/base_application_fetcher.cc (right):

https://codereview.chromium.org/1342503003/diff/60001/mojo/fetcher/base_appli...
mojo/fetcher/base_application_fetcher.cc:59: new LocalFetcher(
It's worth a comment as to who owns the fetcher, same comment on 85.

https://codereview.chromium.org/1342503003/diff/60001/mojo/fetcher/base_appli...
File mojo/fetcher/base_application_fetcher.h (right):

https://codereview.chromium.org/1342503003/diff/60001/mojo/fetcher/base_appli...
mojo/fetcher/base_application_fetcher.h:14: class BaseApplicationFetcher :
public shell::ApplicationFetcher {
Description (it isn't clear that a base application is without looking at the
code).

https://codereview.chromium.org/1342503003/diff/60001/mojo/fetcher/base_appli...
mojo/fetcher/base_application_fetcher.h:27: shell::ApplicationManager*
application_manager_;
initialize to null.

https://codereview.chromium.org/1342503003/diff/60001/mojo/fetcher/base_appli...
mojo/fetcher/base_application_fetcher.h:29: bool disable_cache_;
nit: const

https://codereview.chromium.org/1342503003/diff/60001/mojo/fetcher/url_resolv...
File mojo/fetcher/url_resolver.h (right):

https://codereview.chromium.org/1342503003/diff/60001/mojo/fetcher/url_resolv...
mojo/fetcher/url_resolver.h:30: GURL mojo_base_url_;
nit: const

https://codereview.chromium.org/1342503003/diff/60001/mojo/runner/context.h
File mojo/runner/context.h (right):

https://codereview.chromium.org/1342503003/diff/60001/mojo/runner/context.h#n...
mojo/runner/context.h:28: Context(const base::FilePath& shell_file_root);
explicit.

https://codereview.chromium.org/1342503003/diff/60001/mojo/shell/application_...
File mojo/shell/application_fetcher.h (right):

https://codereview.chromium.org/1342503003/diff/60001/mojo/shell/application_...
mojo/shell/application_fetcher.h:18: class ApplicationFetcher {
Add description.

https://codereview.chromium.org/1342503003/diff/60001/mojo/shell/application_...
mojo/shell/application_fetcher.h:32: // TODO(beng): figure out how not to expose
Fetcher at all at this layer.
Indeed. It would be nice if the callback was defined here. You can do that later
though.

https://codereview.chromium.org/1342503003/diff/60001/mojo/shell/application_...
mojo/shell/application_fetcher.h:40: 
nit: only one newline.

[Ben Goodger (Google), 2015-09-13 17:32:33]

The CQ bit was checked by ben@chromium.org

[Ben Goodger (Google), 2015-09-13 17:32:34]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1342503003/#ps220001
(title: ".")

[commit-bot: I haz the power, 2015-09-13 17:32:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1342503003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1342503003/220001

[commit-bot: I haz the power, 2015-09-13 23:34:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-13 23:34:13]

Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ben Goodger (Google), 2015-09-13 23:49:06]

The CQ bit was checked by ben@chromium.org

[commit-bot: I haz the power, 2015-09-13 23:49:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1342503003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1342503003/220001

[commit-bot: I haz the power, 2015-09-14 05:26:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-14 05:26:21]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ben Goodger (Google), 2015-09-14 17:42:52]

The CQ bit was checked by ben@chromium.org

[commit-bot: I haz the power, 2015-09-14 17:43:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1342503003/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1342503003/220001

[commit-bot: I haz the power, 2015-09-14 18:14:53]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-09-14 18:15:51]

Patchset 12 (id:??) landed as
https://crrev.com/7134773ceeef31531b84818158902e6d7c74f110
Cr-Commit-Position: refs/heads/master@{#348659}

[Devlin, 2015-09-14 18:54:32]

A revert of this CL (patchset #12 id:220001) has been created in
https://codereview.chromium.org/1343823002/ by rdevlin.cronin@chromium.org.

The reason for reverting is: Broke the gn build:
ERROR at //mojo/fetcher/about_fetcher.h:8:11: Include not allowed.
#include "mojo/shell/fetcher.h"
          ^-------------------
It is not in any dependency of
  //mojo/fetcher:fetcher
The include file is in the target(s):
  //mojo/shell:shell
which should somehow be reachable.
___________________
ERROR at //mojo/fetcher/base_application_fetcher.cc:12:11: Include not allowed.
#include "mojo/shell/application_manager.h"
          ^-------------------------------
It is not in any dependency of
  //mojo/fetcher:fetcher
The include file is in the target(s):
  //mojo/shell:shell
which should somehow be reachable.
...

https://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/31954/....

[yzshen1, 2015-09-14 19:06:14]

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

[yzshen1, 2015-09-14 19:06:15]

https://codereview.chromium.org/1342503003/diff/220001/mojo/shell/application...
File mojo/shell/application_manager.cc (right):

https://codereview.chromium.org/1342503003/diff/220001/mojo/shell/application...
mojo/shell/application_manager.cc:106: GURL resolved_url =
fetcher_->ResolveURL(original_url);
One thing that seems like an issue:
Currently for mojo: URLs, we use the pre-resolved URL in the identity (line
143).
That means we won't find a running instance on line 108 even if there is one,
because it uses the resolved URL to look up. So the code proceeds to fetch, and
finally uses the pre-resolved URL on line 208 to look up and find the running
instance.

What do you think? Thanks!

[Ben Goodger (Google), 2015-09-14 23:27:16]

https://codereview.chromium.org/1342503003/diff/220001/mojo/shell/application...
File mojo/shell/application_manager.cc (right):

https://codereview.chromium.org/1342503003/diff/220001/mojo/shell/application...
mojo/shell/application_manager.cc:106: GURL resolved_url =
fetcher_->ResolveURL(original_url);
Good catch! WDYT about uniquifying instances based on the resolved URL?

[yzshen1, 2015-09-14 23:35:36]

On 2015/09/14 23:27:16, Ben Goodger (Google) wrote:
>
https://codereview.chromium.org/1342503003/diff/220001/mojo/shell/application...
> File mojo/shell/application_manager.cc (right):
> 
>
https://codereview.chromium.org/1342503003/diff/220001/mojo/shell/application...
> mojo/shell/application_manager.cc:106: GURL resolved_url =
> fetcher_->ResolveURL(original_url);
> Good catch! WDYT about uniquifying instances based on the resolved URL?

Scott mentioned that the resource provider app gives special privileges to apps
whose URL starts with "mojo:".

If we use the resolved URL in the identity of apps, we will break that behavior.

Maybe we could do the opposite and use the pre-resolved URL for all apps? Will
that cause other problems?

Thanks!

[sky, 2015-09-14 23:37:06]

On Mon, Sep 14, 2015 at 4:35 PM,  <yzshen@chromium.org> wrote:
> On 2015/09/14 23:27:16, Ben Goodger (Google) wrote:
>
>
https://codereview.chromium.org/1342503003/diff/220001/mojo/shell/application...
>>
>> File mojo/shell/application_manager.cc (right):
>
>
>
>
https://codereview.chromium.org/1342503003/diff/220001/mojo/shell/application...
>>
>> mojo/shell/application_manager.cc:106: GURL resolved_url =
>> fetcher_->ResolveURL(original_url);
>> Good catch! WDYT about uniquifying instances based on the resolved URL?
>
>
> Scott mentioned that the resource provider app gives special privileges to
> apps
> whose URL starts with "mojo:".
>
> If we use the resolved URL in the identity of apps, we will break that
> behavior.
>
> Maybe we could do the opposite and use the pre-resolved URL for all apps?
> Will
> that cause other problems?

That would be problematic for sites with redirects. Say 'a' redirects
to 'b'. If you connect to 'a' or 'b' I would expect to get the same
app.

  -Scott

>
> Thanks!
>
> https://codereview.chromium.org/1342503003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[yzshen1, 2015-09-14 23:40:33]

On 2015/09/14 23:37:06, sky wrote:
> On Mon, Sep 14, 2015 at 4:35 PM,  <mailto:yzshen@chromium.org> wrote:
> > On 2015/09/14 23:27:16, Ben Goodger (Google) wrote:
> >
> >
>
https://codereview.chromium.org/1342503003/diff/220001/mojo/shell/application...
> >>
> >> File mojo/shell/application_manager.cc (right):
> >
> >
> >
> >
>
https://codereview.chromium.org/1342503003/diff/220001/mojo/shell/application...
> >>
> >> mojo/shell/application_manager.cc:106: GURL resolved_url =
> >> fetcher_->ResolveURL(original_url);
> >> Good catch! WDYT about uniquifying instances based on the resolved URL?
> >
> >
> > Scott mentioned that the resource provider app gives special privileges to
> > apps
> > whose URL starts with "mojo:".
> >
> > If we use the resolved URL in the identity of apps, we will break that
> > behavior.
> >
> > Maybe we could do the opposite and use the pre-resolved URL for all apps?
> > Will
> > that cause other problems?
> 
> That would be problematic for sites with redirects. Say 'a' redirects
> to 'b'. If you connect to 'a' or 'b' I would expect to get the same
> app.
> 
>   -Scott
> 

I think redirects should be okay. Application manager uses the redirected URL to
start a new ApplicationManager::ConnectToApplication(),
so the app is registered using the redirected URL.

[sky, 2015-09-15 00:00:14]

I spoke with Yuzhu and he set me straight. I was confused by
pre-resolved and thought that would cover redirects, but redirects and
resolving are separate.

Also, we have to be careful with how mandoline registers application
package aliases. This looks fine, but I mention it for completeness.

The only problem we could have with using the pre-resolved name is if
we wanted different mojo urls to resolve to the same thing, or say the
ability to use both mojo: and the file: path identify the same app. I
don't believe we currently do either of these.

  -Scott

On Mon, Sep 14, 2015 at 4:40 PM,  <yzshen@chromium.org> wrote:
> On 2015/09/14 23:37:06, sky wrote:
>>
>> On Mon, Sep 14, 2015 at 4:35 PM,  <mailto:yzshen@chromium.org> wrote:
>> > On 2015/09/14 23:27:16, Ben Goodger (Google) wrote:
>> >
>> >
>
>
>
https://codereview.chromium.org/1342503003/diff/220001/mojo/shell/application...
>>
>> >>
>> >> File mojo/shell/application_manager.cc (right):
>> >
>> >
>> >
>> >
>
>
>
https://codereview.chromium.org/1342503003/diff/220001/mojo/shell/application...
>>
>> >>
>> >> mojo/shell/application_manager.cc:106: GURL resolved_url =
>> >> fetcher_->ResolveURL(original_url);
>> >> Good catch! WDYT about uniquifying instances based on the resolved URL?
>> >
>> >
>> > Scott mentioned that the resource provider app gives special privileges
>> > to
>> > apps
>> > whose URL starts with "mojo:".
>> >
>> > If we use the resolved URL in the identity of apps, we will break that
>> > behavior.
>> >
>> > Maybe we could do the opposite and use the pre-resolved URL for all
>> > apps?
>> > Will
>> > that cause other problems?
>
>
>> That would be problematic for sites with redirects. Say 'a' redirects
>> to 'b'. If you connect to 'a' or 'b' I would expect to get the same
>> app.
>
>
>>    -Scott
>
>
>
> I think redirects should be okay. Application manager uses the redirected
> URL to
> start a new ApplicationManager::ConnectToApplication(),
> so the app is registered using the redirected URL.
>
>
> https://codereview.chromium.org/1342503003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ben Goodger (Google), 2015-09-15 17:52:40]

The CQ bit was checked by ben@chromium.org

[Ben Goodger (Google), 2015-09-15 17:52:40]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1342503003/#ps260001
(title: ".")

[commit-bot: I haz the power, 2015-09-15 17:53:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1342503003/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1342503003/260001

[commit-bot: I haz the power, 2015-09-15 19:20:57]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2015-09-15 19:21:37]

Patchset 14 (id:??) landed as
https://crrev.com/c7ef4d2131963307ea537fc269bb5acee9383de2
Cr-Commit-Position: refs/heads/master@{#348954}

[Devlin, 2015-09-15 20:28:29]

A revert of this CL (patchset #14 id:260001) has been created in
https://codereview.chromium.org/1338283003/ by rdevlin.cronin@chromium.org.

The reason for reverting is: Broke the gn builds:
ERROR at //mojo/fetcher/about_fetcher_unittest.cc:17:11: Can't include this
header from here.
#include "mojo/fetcher/about_fetcher.h"
          ^---------------------------
The target:
  //mojo/runner:mojo_runner_unittests
is including a file from the target:
  //mojo/fetcher:fetcher

It's usually best to depend directly on the destination target.
In some cases, the destination target is considered a subcomponent
of an intermediate target. In this case, the intermediate target
should depend publicly on the destination to forward the ability
to include headers.

Dependency chain (there may also be others):
  //mojo/runner:mojo_runner_unittests -->
  //mojo/runner:lib --[private]-->
  //mojo/fetcher:fetcher

https://build.chromium.org/p/chromium.linux/builders/Linux%20GN/builds/32017/....

[Ben Goodger (Google), 2015-09-15 22:34:39]

The CQ bit was checked by ben@chromium.org

[Ben Goodger (Google), 2015-09-15 22:34:40]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1342503003/#ps280001
(title: ".")

[commit-bot: I haz the power, 2015-09-15 22:35:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1342503003/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1342503003/280001

[commit-bot: I haz the power, 2015-09-16 00:10:23]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2015-09-16 00:11:31]

Patchset 15 (id:??) landed as
https://crrev.com/f4e3b26f16471576ed034e74d863ec6e347b0417
Cr-Commit-Position: refs/heads/master@{#349035}

[commit-bot: I haz the power, 2015-09-23 12:34:30]

Patchset 15 (id:??) landed as
https://crrev.com/7134773ceeef31531b84818158902e6d7c74f110
Cr-Commit-Position: refs/heads/master@{#348659}

[commit-bot: I haz the power, 2015-09-23 12:47:59]

Patchset 15 (id:??) landed as
https://crrev.com/c7ef4d2131963307ea537fc269bb5acee9383de2
Cr-Commit-Position: refs/heads/master@{#348954}

[commit-bot: I haz the power, 2015-09-23 12:51:18]

Patchset 15 (id:??) landed as
https://crrev.com/f4e3b26f16471576ed034e74d863ec6e347b0417
Cr-Commit-Position: refs/heads/master@{#349035}