Move more of ContentHandler handling into PackageManager.

Move more of ContentHandler handling into PackageManager.

Dependency/#include rules required that I restructure the unit tests as part of this, moving all the content handler tests into package_manager also.

R=yzshen@chromium.org
http://crbug.com/533085

Committed: https://crrev.com/885517fdbc3a284501d457c9f0256ea731f54e07
Cr-Commit-Position: refs/heads/master@{#350572}

[adamandashley13, 2015-09-19 15:23:04]

adamandashley13@gmail.com changed reviewers:
+ adamandashley13@gmail.com

[adamandashley13, 2015-09-19 15:23:04]

The CQ bit was checked by adamandashley13@gmail.com

[adamandashley13, 2015-09-19 15:23:05]

lgtm

[adamandashley13, 2015-09-19 15:23:06]

lgtm

[commit-bot: I haz the power, 2015-09-19 15:23:07]

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

[commit-bot: I haz the power, 2015-09-19 15:29:38]

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

[commit-bot: I haz the power, 2015-09-19 15:29:39]

Try jobs failed on following builders:
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Ben Goodger (Google), 2015-09-19 16:54:22]

On 2015/09/19 15:23:06, adamandashley13 wrote:
> lgtm

1. This change is a work in progress and is not for review.
2. I'm not sure who you are but please don't randomly lgtm changes if you have
no standing in the chromium project.
3. Even more importantly, please don't randomly check the commit bit on other
people's changes.

I'm removing you as a reviewer from this change.

[Ben Goodger (Google), 2015-09-19 16:54:30]

ben@chromium.org changed reviewers:
- adamandashley13@gmail.com

[Ben Goodger (Google), 2015-09-22 19:07:25]

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

[Ben Goodger (Google), 2015-09-23 15:25:03]

OK I think this is ready for review.

[yzshen1, 2015-09-23 16:53:46]

https://codereview.chromium.org/1358533004/diff/160001/content/browser/mojo/m...
File content/browser/mojo/mojo_shell_context.cc (right):

https://codereview.chromium.org/1358533004/diff/160001/content/browser/mojo/m...
content/browser/mojo/mojo_shell_context.cc:163: new
mojo::package_manager::PackageManagerImpl(base::FilePath(), nullptr));
Passing null could crash in some cases (e.g., LocalFetcher is used in
HandleWithContentHandler). Although the code path might not be reached, I feel
that we need to either pass a valid task runner or document clearly under what
circumstances nullptr is acceptable.

https://codereview.chromium.org/1358533004/diff/160001/mojo/package_manager/c...
File mojo/package_manager/content_handler_unittest.cc (right):

https://codereview.chromium.org/1358533004/diff/160001/mojo/package_manager/c...
mojo/package_manager/content_handler_unittest.cc:32: const char kTestURLString[]
= "test:testService";
These three urls are not used.

https://codereview.chromium.org/1358533004/diff/160001/mojo/package_manager/c...
mojo/package_manager/content_handler_unittest.cc:101: test_app_.reset();
nit: this line seems unnecessary.

https://codereview.chromium.org/1358533004/diff/160001/mojo/package_manager/p...
File mojo/package_manager/package_manager_impl.cc (right):

https://codereview.chromium.org/1358533004/diff/160001/mojo/package_manager/p...
mojo/package_manager/package_manager_impl.cc:198: // Why can't we use this in
single process mode? Because of
Because this comment seems to apply to all the three if-blocks in this method,
please consider moving this comment to right above line 162 (and maybe change
|enable_multi_process| to |use_real_qualifier| so that the comment is relevant
to the variable definition).

https://codereview.chromium.org/1358533004/diff/160001/mojo/package_manager/p...
File mojo/package_manager/package_manager_impl.h (right):

https://codereview.chromium.org/1358533004/diff/160001/mojo/package_manager/p...
mojo/package_manager/package_manager_impl.h:34: base::TaskRunner* task_runner);
Please comment on what this task_runner is used for.

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

https://codereview.chromium.org/1358533004/diff/160001/mojo/shell/application...
mojo/shell/application_manager.cc:14: #include
"mojo/application/public/interfaces/content_handler.mojom.h"
This include is probably no longer needed.

https://codereview.chromium.org/1358533004/diff/160001/mojo/shell/application...
File mojo/shell/application_manager.h (right):

https://codereview.chromium.org/1358533004/diff/160001/mojo/shell/application...
mojo/shell/application_manager.h:60: base::TaskRunner* task_runner);
Please comment on what this task_runner is used for.

https://codereview.chromium.org/1358533004/diff/160001/mojo/shell/package_man...
File mojo/shell/package_manager.h (right):

https://codereview.chromium.org/1358533004/diff/160001/mojo/shell/package_man...
mojo/shell/package_manager.h:40: // |url| is the url requested initially by a
call to ConnectToApplication().
Please update the comments.

[Ben Goodger (Google), 2015-09-23 18:24:20]

Thanks for the great comments. Updated.

https://codereview.chromium.org/1358533004/diff/160001/content/browser/mojo/m...
File content/browser/mojo/mojo_shell_context.cc (right):

https://codereview.chromium.org/1358533004/diff/160001/content/browser/mojo/m...
content/browser/mojo/mojo_shell_context.cc:163: new
mojo::package_manager::PackageManagerImpl(base::FilePath(), nullptr));
On 2015/09/23 16:53:46, yzshen1 wrote:
> Passing null could crash in some cases (e.g., LocalFetcher is used in
> HandleWithContentHandler). Although the code path might not be reached, I feel
> that we need to either pass a valid task runner or document clearly under what
> circumstances nullptr is acceptable.

Good point. Currently nullptr is equivalent to not calling set_blocking_pool()
on ApplicationManager. The only client of ApplicationManager that calls this
method is Context. The other cases are tests, and in this instance content. This
code will have to change if it ever wants to load mojo apps using Local or
NetworkFetcher.

I've updated the documentation in package_manager_impl.h and
application_manager.h to reflect this.

[Ben Goodger (Google), 2015-09-23 18:25:43]

On 2015/09/23 18:24:20, Ben Goodger (Google) wrote:
> Thanks for the great comments. Updated.
> 
>
https://codereview.chromium.org/1358533004/diff/160001/content/browser/mojo/m...
> File content/browser/mojo/mojo_shell_context.cc (right):
> 
>
https://codereview.chromium.org/1358533004/diff/160001/content/browser/mojo/m...
> content/browser/mojo/mojo_shell_context.cc:163: new
> mojo::package_manager::PackageManagerImpl(base::FilePath(), nullptr));
> On 2015/09/23 16:53:46, yzshen1 wrote:
> > Passing null could crash in some cases (e.g., LocalFetcher is used in
> > HandleWithContentHandler). Although the code path might not be reached, I
feel
> > that we need to either pass a valid task runner or document clearly under
what
> > circumstances nullptr is acceptable.
> 
> Good point. Currently nullptr is equivalent to not calling set_blocking_pool()
> on ApplicationManager. The only client of ApplicationManager that calls this
> method is Context. The other cases are tests, and in this instance content.
This
> code will have to change if it ever wants to load mojo apps using Local or
> NetworkFetcher.
> 
> I've updated the documentation in package_manager_impl.h and
> application_manager.h to reflect this.

btw ultimately I'd like to have ApplicationManager not know about fetchers at
all, so it wouldn't need a task runner. Right now it has to have one because it
uses LocalFetcher::AsPath() at the end of HandleFetchCallback. I'd like to find
a way to cleanly shift this logic into PackageManager, but this CL was already
too big :-)

[yzshen1, 2015-09-23 20:50:57]

On 2015/09/23 18:25:43, Ben Goodger (Google) wrote:
> On 2015/09/23 18:24:20, Ben Goodger (Google) wrote:
> > Thanks for the great comments. Updated.
> > 
> >
>
https://codereview.chromium.org/1358533004/diff/160001/content/browser/mojo/m...
> > File content/browser/mojo/mojo_shell_context.cc (right):
> > 
> >
>
https://codereview.chromium.org/1358533004/diff/160001/content/browser/mojo/m...
> > content/browser/mojo/mojo_shell_context.cc:163: new
> > mojo::package_manager::PackageManagerImpl(base::FilePath(), nullptr));
> > On 2015/09/23 16:53:46, yzshen1 wrote:
> > > Passing null could crash in some cases (e.g., LocalFetcher is used in
> > > HandleWithContentHandler). Although the code path might not be reached, I
> feel
> > > that we need to either pass a valid task runner or document clearly under
> what
> > > circumstances nullptr is acceptable.
> > 
> > Good point. Currently nullptr is equivalent to not calling
set_blocking_pool()
> > on ApplicationManager. The only client of ApplicationManager that calls this
> > method is Context. The other cases are tests, and in this instance content.
> This
> > code will have to change if it ever wants to load mojo apps using Local or
> > NetworkFetcher.
> > 
> > I've updated the documentation in package_manager_impl.h and
> > application_manager.h to reflect this.
> 
> btw ultimately I'd like to have ApplicationManager not know about fetchers at
> all, so it wouldn't need a task runner. Right now it has to have one because
it
> uses LocalFetcher::AsPath() at the end of HandleFetchCallback. I'd like to
find
> a way to cleanly shift this logic into PackageManager, but this CL was already
> too big :-)

LGTM Thanks for explaining!

[Ben Goodger (Google), 2015-09-23 20:54:58]

The CQ bit was checked by ben@chromium.org

[Ben Goodger (Google), 2015-09-23 20:54:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
adamandashley13@gmail.com
Link to the patchset: https://codereview.chromium.org/1358533004/#ps180001
(title: ".")

[commit-bot: I haz the power, 2015-09-23 20:56:08]

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

[commit-bot: I haz the power, 2015-09-23 21:25:56]

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

[commit-bot: I haz the power, 2015-09-23 21:25:56]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Ben Goodger (Google), 2015-09-23 22:02:59]

The CQ bit was checked by ben@chromium.org

[commit-bot: I haz the power, 2015-09-23 22:06:25]

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

[commit-bot: I haz the power, 2015-09-23 22:51:20]

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

[commit-bot: I haz the power, 2015-09-23 22:51:21]

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

[Ben Goodger (Google), 2015-09-24 03:57:04]

The CQ bit was checked by ben@chromium.org

[commit-bot: I haz the power, 2015-09-24 03:57:36]

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

[commit-bot: I haz the power, 2015-09-24 04:29:01]

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

[commit-bot: I haz the power, 2015-09-24 04:29:02]

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

[Ben Goodger (Google), 2015-09-24 15:10:22]

The CQ bit was checked by ben@chromium.org

[Ben Goodger (Google), 2015-09-24 15:10:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
adamandashley13@gmail.com, yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/1358533004/#ps220001
(title: ".")

[commit-bot: I haz the power, 2015-09-24 15:11:45]

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

[commit-bot: I haz the power, 2015-09-24 16:39:25]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-09-24 16:40:34]

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