Adds a way to determine id of content handler that created app

Adds a way to determine id of content handler that created app

This is needed for html viewer so that it can know which html_viewer is servicing the request.

BUG=479172, 490221
TEST=covered by tests
R=ben@chromium.org

Committed: https://crrev.com/6e094e866fc853c369d2cf27fc947bd0708c6840
Cr-Commit-Position: refs/heads/master@{#346541}

[sky, 2015-08-29 17:40:47]

Updated as discussed (and updated description).

[Ben Goodger (Google), 2015-08-31 17:41:07]

https://codereview.chromium.org/1311353005/diff/60001/components/html_viewer/...
File components/html_viewer/media_factory.cc (right):

https://codereview.chromium.org/1311353005/diff/60001/components/html_viewer/...
components/html_viewer/media_factory.cc:126:
base::Bind(&OnGotContentHandlerID));
mojo::Shell::ConnectToApplicationCallback()?

https://codereview.chromium.org/1311353005/diff/60001/components/pdf_viewer/p...
File components/pdf_viewer/pdf_viewer.cc (right):

https://codereview.chromium.org/1311353005/diff/60001/components/pdf_viewer/p...
components/pdf_viewer/pdf_viewer.cc:84: nullptr,
base::Bind(&OnGotContentHandlerID));
etc..

https://codereview.chromium.org/1311353005/diff/60001/mandoline/ui/aura/surfa...
File mandoline/ui/aura/surface_binding.cc (right):

https://codereview.chromium.org/1311353005/diff/60001/mandoline/ui/aura/surfa...
mandoline/ui/aura/surface_binding.cc:129: base::Bind(&OnGotContentHandlerID));
etc..

https://codereview.chromium.org/1311353005/diff/60001/mojo/application/public...
File mojo/application/public/interfaces/shell.mojom (right):

https://codereview.chromium.org/1311353005/diff/60001/mojo/application/public...
mojo/application/public/interfaces/shell.mojom:63: // used |content_handler_id|
is 0.
since this looks a little arbitrary... add:

TODO(beng): determine if we need to expose the target application id also.

https://codereview.chromium.org/1311353005/diff/60001/mojo/shell/application_...
File mojo/shell/application_instance.cc (right):

https://codereview.chromium.org/1311353005/diff/60001/mojo/shell/application_...
mojo/shell/application_instance.cc:123:
callback.Run(ApplicationManager::kInvalidContentHandlerID);
if it's an expected part of the api that your callback might get run with this
special id, perhaps this id should be declared in the mojom?

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

https://codereview.chromium.org/1311353005/diff/60001/mojo/shell/application_...
mojo/shell/application_instance.h:92: Shell::ConnectToApplicationCallback
connect_callback;
don't think you need the Shell:: here?

https://codereview.chromium.org/1311353005/diff/60001/mojo/shell/application_...
File mojo/shell/application_manager.cc (right):

https://codereview.chromium.org/1311353005/diff/60001/mojo/shell/application_...
mojo/shell/application_manager.cc:205:
connect_callback.Run(instance->requesting_content_handler_id());
is ConnectToClient a more central place to run this callback?

https://codereview.chromium.org/1311353005/diff/60001/mojo/shell/application_...
File mojo/shell/application_manager_unittest.cc (right):

https://codereview.chromium.org/1311353005/diff/60001/mojo/shell/application_...
mojo/shell/application_manager_unittest.cc:886:
MultipleConnectionsToContentHandlerGetSameTargetId) {
remove "Target" since you renamed the concept.

https://codereview.chromium.org/1311353005/diff/60001/mojo/shell/content_hand...
File mojo/shell/content_handler_connection.h (right):

https://codereview.chromium.org/1311353005/diff/60001/mojo/shell/content_hand...
mojo/shell/content_handler_connection.h:51: void ScheduleTargetIdCallback(
rename?

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

https://codereview.chromium.org/1311353005/diff/140001/mojo/shell/application...
mojo/shell/application_manager.cc:265:
connect_callback.Run(kInvalidContentHandlerID);
Specifically I'm trying to understand the flow of this content handler id. Why
if a content handler id is passed do you call this callback with invalid id?

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

https://codereview.chromium.org/1311353005/diff/140001/mojo/shell/application...
mojo/shell/application_manager.h:88: // If this request is from a content
handler then
what does this mean?

https://codereview.chromium.org/1311353005/diff/140001/mojo/shell/content_han...
File mojo/shell/content_handler_connection.h (right):

https://codereview.chromium.org/1311353005/diff/140001/mojo/shell/content_han...
mojo/shell/content_handler_connection.h:58: void
OnGotNestedContentHandlerID(uint32_t content_handler_id);
why do you care about nested content handler ids?

[sky, 2015-08-31 19:46:07]

https://codereview.chromium.org/1311353005/diff/60001/components/html_viewer/...
File components/html_viewer/media_factory.cc (right):

https://codereview.chromium.org/1311353005/diff/60001/components/html_viewer/...
components/html_viewer/media_factory.cc:126:
base::Bind(&OnGotContentHandlerID));
On 2015/08/31 17:41:07, Ben Goodger (Google) wrote:
> mojo::Shell::ConnectToApplicationCallback()?

That would require this code linking with mojo_shell.

https://codereview.chromium.org/1311353005/diff/60001/components/pdf_viewer/p...
File components/pdf_viewer/pdf_viewer.cc (right):

https://codereview.chromium.org/1311353005/diff/60001/components/pdf_viewer/p...
components/pdf_viewer/pdf_viewer.cc:84: nullptr,
base::Bind(&OnGotContentHandlerID));
On 2015/08/31 17:41:07, Ben Goodger (Google) wrote:
> etc..

See earlier comment. Basically any where that linked with shell I used the shell
callback. Everywhere else I duplicate it.

https://codereview.chromium.org/1311353005/diff/60001/mojo/shell/application_...
File mojo/shell/application_instance.cc (right):

https://codereview.chromium.org/1311353005/diff/60001/mojo/shell/application_...
mojo/shell/application_instance.cc:123:
callback.Run(ApplicationManager::kInvalidContentHandlerID);
On 2015/08/31 17:41:07, Ben Goodger (Google) wrote:
> if it's an expected part of the api that your callback might get run with this
> special id, perhaps this id should be declared in the mojom?

I didn't realize you could have constants in the mojom. Done.

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

https://codereview.chromium.org/1311353005/diff/60001/mojo/shell/application_...
mojo/shell/application_instance.h:92: Shell::ConnectToApplicationCallback
connect_callback;
On 2015/08/31 17:41:07, Ben Goodger (Google) wrote:
> don't think you need the Shell:: here?

Done.

https://codereview.chromium.org/1311353005/diff/60001/mojo/shell/application_...
File mojo/shell/application_manager.cc (right):

https://codereview.chromium.org/1311353005/diff/60001/mojo/shell/application_...
mojo/shell/application_manager.cc:205:
connect_callback.Run(instance->requesting_content_handler_id());
On 2015/08/31 17:41:07, Ben Goodger (Google) wrote:
> is ConnectToClient a more central place to run this callback?

Done.

https://codereview.chromium.org/1311353005/diff/60001/mojo/shell/application_...
File mojo/shell/application_manager_unittest.cc (right):

https://codereview.chromium.org/1311353005/diff/60001/mojo/shell/application_...
mojo/shell/application_manager_unittest.cc:886:
MultipleConnectionsToContentHandlerGetSameTargetId) {
On 2015/08/31 17:41:07, Ben Goodger (Google) wrote:
> remove "Target" since you renamed the concept.

Done.

https://codereview.chromium.org/1311353005/diff/60001/mojo/shell/content_hand...
File mojo/shell/content_handler_connection.h (right):

https://codereview.chromium.org/1311353005/diff/60001/mojo/shell/content_hand...
mojo/shell/content_handler_connection.h:51: void ScheduleTargetIdCallback(
On 2015/08/31 17:41:07, Ben Goodger (Google) wrote:
> rename?

This was all nuked.

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

https://codereview.chromium.org/1311353005/diff/140001/mojo/shell/application...
mojo/shell/application_manager.cc:265:
connect_callback.Run(kInvalidContentHandlerID);
On 2015/08/31 17:41:07, Ben Goodger (Google) wrote:
> Specifically I'm trying to understand the flow of this content handler id. Why
> if a content handler id is passed do you call this callback with invalid id?

Ya, this code is a bit cryptic. The key places that enabled the id are
ApplicationManager::LoadWithContentHandler, which is called to start a new
application loaded from a content handler. In that case the callback is run with
the id of the content handler. The other place is if the app already exists, in
which case we have to supply the id used at the time of creation of the app.
That code is ConnectToRunningApplication.

Much of the code for content handlers is done in HandleFetchCallback. As
AppInstance is actually created before we make the decision as to whether the
app is going to be loaded by a content handler HandleFetchCallback supplies a
callback to RegisterInstance() that does nothing. It's only once we know the app
is going to be loaded by a content handler that the real callback is called.
RegisterInstance() is called from a number of different code paths though, and
it simplifies things to have this run the callback too.

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

https://codereview.chromium.org/1311353005/diff/140001/mojo/shell/application...
mojo/shell/application_manager.h:88: // If this request is from a content
handler then
On 2015/08/31 17:41:07, Ben Goodger (Google) wrote:
> what does this mean?

I've nuked this. It isn't needed.

https://codereview.chromium.org/1311353005/diff/140001/mojo/shell/content_han...
File mojo/shell/content_handler_connection.h (right):

https://codereview.chromium.org/1311353005/diff/140001/mojo/shell/content_han...
mojo/shell/content_handler_connection.h:58: void
OnGotNestedContentHandlerID(uint32_t content_handler_id);
On 2015/08/31 17:41:07, Ben Goodger (Google) wrote:
> why do you care about nested content handler ids?

Indeed. I've change this code around quite a bit.

[Ben Goodger (Google), 2015-08-31 23:10:28]

lgtm

https://codereview.chromium.org/1311353005/diff/60001/components/html_viewer/...
File components/html_viewer/media_factory.cc (right):

https://codereview.chromium.org/1311353005/diff/60001/components/html_viewer/...
components/html_viewer/media_factory.cc:126:
base::Bind(&OnGotContentHandlerID));
On 2015/08/31 19:46:06, sky wrote:
> On 2015/08/31 17:41:07, Ben Goodger (Google) wrote:
> > mojo::Shell::ConnectToApplicationCallback()?
> 
> That would require this code linking with mojo_shell.

You could put it in mojo/application/public/cpp... turns out mojo/shell already
depends on that. Only for tests, but I don't have an issue with service impls
depending on client libs.

[sky, 2015-08-31 23:14:09]

The CQ bit was checked by sky@chromium.org

[sky, 2015-08-31 23:14:09]

The patchset sent to the CQ was uploaded after l-g-t-m from ben@chromium.org
Link to the patchset: https://codereview.chromium.org/1311353005/#ps180001
(title: "nuke comment")

[commit-bot: I haz the power, 2015-08-31 23:14:25]

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

[commit-bot: I haz the power, 2015-09-01 00:58:58]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2015-09-01 00:59:37]

Patchset 10 (id:??) landed as
https://crrev.com/6e094e866fc853c369d2cf27fc947bd0708c6840
Cr-Commit-Position: refs/heads/master@{#346541}