[Extensions] Clean up the handling of ExtensionHostMsg_Request

[Extensions] Clean up the handling of ExtensionHostMsg_Request

ExtensionHostMsg_Request is sent when an extension calls an API function. Before
this patch, this IPC would be sent to one of 11 different call sites, all of
which then routed it to the ExtensionFunctionDispatcher - and all of which
have to implement ExtensionFunctionDispatcher::Delegate.

Instead, have ExtensionWebContentsObserver handle the IPC, since it is created
(or should be) for all extension web contents. This also lets us eliminate many
(though not all) of the ExtensionFunctionDispatcher::Delegate implementations
(I will try to clean more up in a later patch).

The size of this patch is due to a number of yaks that needed shaving along the
way - in particular, around GuestView.

BUG=498017
BUG=405246
Committed: https://crrev.com/cb2ec659ab8741962f3391970a5fff512ebb6509
Cr-Commit-Position: refs/heads/master@{#333843}

[Devlin, 2015-06-09 21:56:31]

Patchset #1 (id:1) has been deleted

[Devlin, 2015-06-09 21:56:42]

Patchset #1 (id:20001) has been deleted

[Devlin, 2015-06-09 22:18:01]

rdevlin.cronin@chromium.org changed reviewers:
+ kalman@chromium.org

[Devlin, 2015-06-09 22:18:02]

Hey Ben - Sorry for the size.  On the upside, it really is a nice clean up, and
has a very negative line count!

[Devlin, 2015-06-09 22:26:43]

patch failure... sad day.  Patch set 1 has a green try, and not much has
functionally changed.

[Devlin, 2015-06-10 18:32:34]

rdevlin.cronin@chromium.org changed reviewers:
+ fsamuel@chromium.org, sky@chromium.org

[Devlin, 2015-06-10 18:32:35]

+Fady guest_view/
+Sky for ui/

[not at google - send to devlin, 2015-06-10 18:37:49]

Looks great to me, this is a huge cleanup. I doubt I'll have many more comments
than these, but I'm heading off to lunch. I went through all the main files and
about half the smaller ones.

https://codereview.chromium.org/1169223002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

https://codereview.chromium.org/1169223002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/downloads/downloads_api.cc:1248:
dispatcher()->GetVisibleWebContents();
I think this will fit on 1 line (and I would hope that clang-format realises
that).

https://codereview.chromium.org/1169223002/diff/80001/extensions/browser/exte...
File extensions/browser/extension_function_dispatcher.h (right):

https://codereview.chromium.org/1169223002/diff/80001/extensions/browser/exte...
extensions/browser/extension_function_dispatcher.h:95:
ExtensionFunctionDispatcher(content::BrowserContext* browser_context);
explicit

https://codereview.chromium.org/1169223002/diff/80001/extensions/browser/gues...
File extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc
(right):

https://codereview.chromium.org/1169223002/diff/80001/extensions/browser/gues...
extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc:150:
ExtensionsAPIClient::Get()->AttachWebContentsHelpers(web_contents());
It seems like this belongs in the GuestView base class, since guest view relies
on extension bindings to work. In fact many of these GuestView::DidInitialize
implementations *only* call AttachWebContentsHelpers(), so you could remove
those.

I'll let fsamuel@ comment on this, though.

[sky, 2015-06-10 19:43:01]

LGTM

[not at google - send to devlin, 2015-06-10 20:48:13]

Finished. 1 main comment, apologies if missing something obvious in the
SupportsUserData machinations.

https://codereview.chromium.org/1169223002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_extension_function_details.cc (right):

https://codereview.chromium.org/1169223002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/chrome_extension_function_details.cc:57:
function_->dispatcher()->GetExtensionWindowController();
It would be nice if this were just function_->GetWindowController() (likewise
GetVisibleWebContents), and the dispatcher() accessor on ExtensionFunction
entirely removed. But, not necessarily right now.

https://codereview.chromium.org/1169223002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/tab_helper.cc (right):

https://codereview.chromium.org/1169223002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:111: set_delegate(this);
This pattern appears a lot:

// maybe there's a comment, maybe not.
ChromeExtensionWebContentsObserver::CreateForWebContents(web_contents);
ExtensionWebContentsObserver::GetForWebContents(web_contents)->dispatcher()->set_delegate(this);

it would be nicer to do this in 1 step, plus moot the way I needed to go back
and forth between files figuring out why one is on
ChromeExtensionWebContentsObserver and the other is on
ExtensionWebContentsObserver. How about:

ExtensionsBrowserClient::GetOrCreateExtensionWebContentsObserver(web_contents);

which, like you've already done in extension_web_contents_observer, you could
alias to look like:

ExtensionWebContentsObserver::GetOrCreateWebContents(web_contents)
    ->dispatcher()
    ->set_delegate(this);

and in fact, would that let you remove
ExtensionAPIClient::AttachWebContentsHelpers, since this would do the same?

https://codereview.chromium.org/1169223002/diff/80001/extensions/browser/app_...
File extensions/browser/app_window/app_window.cc (right):

https://codereview.chromium.org/1169223002/diff/80001/extensions/browser/app_...
extensions/browser/app_window/app_window.cc:262:
ExtensionWebContentsObserver::GetForWebContents(web_contents())->
Lack of a CreateForWebContents here is a bit puzzling. I guess it's being
created somewhere up the stack...?

Also in ExtensionHost.

[Fady Samuel, 2015-06-10 20:51:18]

https://codereview.chromium.org/1169223002/diff/80001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc
(left):

https://codereview.chromium.org/1169223002/diff/80001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:89: }
Wow, have you moved all of this into extensions? I don't see them. Did I miss
something?

https://codereview.chromium.org/1169223002/diff/80001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/1169223002/diff/80001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:339:
ExtensionsAPIClient::Get()->AttachWebContentsHelpers(web_contents());
Does this mean all extensions will get routed to a function dispatcher? Does
that mean arbitrary extension APIs will work inside a webview? I worry this
breaks things?

[Devlin, 2015-06-10 20:59:20]

https://codereview.chromium.org/1169223002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/downloads/downloads_api.cc (right):

https://codereview.chromium.org/1169223002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/downloads/downloads_api.cc:1248:
dispatcher()->GetVisibleWebContents();
On 2015/06/10 18:37:49, kalman wrote:
> I think this will fit on 1 line (and I would hope that clang-format realises
> that).

Done.

https://codereview.chromium.org/1169223002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_extension_function_details.cc (right):

https://codereview.chromium.org/1169223002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/chrome_extension_function_details.cc:57:
function_->dispatcher()->GetExtensionWindowController();
On 2015/06/10 20:48:12, kalman wrote:
> It would be nice if this were just function_->GetWindowController() (likewise
> GetVisibleWebContents), and the dispatcher() accessor on ExtensionFunction
> entirely removed. But, not necessarily right now.

Agreed - I intend to try and clean up ExtensionFunction's dependence on
Dispatcher at some point, but that's a later patch.

https://codereview.chromium.org/1169223002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/tab_helper.cc (right):

https://codereview.chromium.org/1169223002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:111: set_delegate(this);
On 2015/06/10 20:48:12, kalman wrote:
> This pattern appears a lot:
> 
> // maybe there's a comment, maybe not.
> ChromeExtensionWebContentsObserver::CreateForWebContents(web_contents);
>
ExtensionWebContentsObserver::GetForWebContents(web_contents)->dispatcher()->set_delegate(this);
> 
> it would be nicer to do this in 1 step, plus moot the way I needed to go back
> and forth between files figuring out why one is on
> ChromeExtensionWebContentsObserver and the other is on
> ExtensionWebContentsObserver. How about:
> 
>
ExtensionsBrowserClient::GetOrCreateExtensionWebContentsObserver(web_contents);
> 
> which, like you've already done in extension_web_contents_observer, you could
> alias to look like:
> 
> ExtensionWebContentsObserver::GetOrCreateWebContents(web_contents)
>     ->dispatcher()
>     ->set_delegate(this);
> 
> and in fact, would that let you remove
> ExtensionAPIClient::AttachWebContentsHelpers, since this would do the same?

That would work.  My concern is that it makes it a little less deterministic as
to when ExtensionWebContentsObservers are created, which we don't want (this
particular case was a little bit of throwing in the towel on my part, since we
do the same thing above with SessionTabHelper).  I think I'd prefer to have the
normal case be a *failure* so that we know we didn't initialize the
ExtensionWebContentsObserver when we were supposed to, rather than silence,
which may result in missing important IPCs...

WDYT?

https://codereview.chromium.org/1169223002/diff/80001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc
(left):

https://codereview.chromium.org/1169223002/diff/80001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.cc:89: }
On 2015/06/10 20:51:18, Fady Samuel wrote:
> Wow, have you moved all of this into extensions? I don't see them. Did I miss
> something?

This all moved into the ChromeExtensionAPIClient::AttachWebContentsHandlers()
method, which is called from each guest view type.

https://codereview.chromium.org/1169223002/diff/80001/extensions/browser/app_...
File extensions/browser/app_window/app_window.cc (right):

https://codereview.chromium.org/1169223002/diff/80001/extensions/browser/app_...
extensions/browser/app_window/app_window.cc:262:
ExtensionWebContentsObserver::GetForWebContents(web_contents())->
On 2015/06/10 20:48:12, kalman wrote:
> Lack of a CreateForWebContents here is a bit puzzling. I guess it's being
> created somewhere up the stack...?
> 
> Also in ExtensionHost.

Yep, they're created up the stack.  In this particular case, it's created in
app_delegate_->InitWebContents().

https://codereview.chromium.org/1169223002/diff/80001/extensions/browser/exte...
File extensions/browser/extension_function_dispatcher.h (right):

https://codereview.chromium.org/1169223002/diff/80001/extensions/browser/exte...
extensions/browser/extension_function_dispatcher.h:95:
ExtensionFunctionDispatcher(content::BrowserContext* browser_context);
On 2015/06/10 18:37:49, kalman wrote:
> explicit

Done.

https://codereview.chromium.org/1169223002/diff/80001/extensions/browser/gues...
File extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc
(right):

https://codereview.chromium.org/1169223002/diff/80001/extensions/browser/gues...
extensions/browser/guest_view/mime_handler_view/mime_handler_view_guest.cc:150:
ExtensionsAPIClient::Get()->AttachWebContentsHelpers(web_contents());
On 2015/06/10 18:37:49, kalman wrote:
> It seems like this belongs in the GuestView base class, since guest view
relies
> on extension bindings to work. In fact many of these GuestView::DidInitialize
> implementations *only* call AttachWebContentsHelpers(), so you could remove
> those.
> 
> I'll let fsamuel@ comment on this, though.

The only concern is that it would introduce a dependency from
components/guest_view to extensions, or would require wiring up (another)
delegate to this.  If we decide to do it (which I'd be fine with - it would be a
nice little clean up), I'd probably want to push it to a separate patch, if
that's okay (as is, this will still clean up quite a bit of code and introduces
no new dependencies).

https://codereview.chromium.org/1169223002/diff/80001/extensions/browser/gues...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/1169223002/diff/80001/extensions/browser/gues...
extensions/browser/guest_view/web_view/web_view_guest.cc:339:
ExtensionsAPIClient::Get()->AttachWebContentsHelpers(web_contents());
On 2015/06/10 20:51:18, Fady Samuel wrote:
> Does this mean all extensions will get routed to a function dispatcher? Does
> that mean arbitrary extension APIs will work inside a webview? I worry this
> breaks things?

Not really.  ExtensionWebContentsObserver (and ExtensionFunctionDispatcher, etc)
don't inherently allow for a web contents to use extension APIs.  That's handled
primarily by the exposure of the bindings to different javascript contexts,
which is all handled on the renderer side.  All this should do is wire up
relevant web contents helpers - which could be useful if, for instance, we
happen to embed an extension page in a web view.

[not at google - send to devlin, 2015-06-10 21:37:07]

lgtm

https://codereview.chromium.org/1169223002/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/tab_helper.cc (right):

https://codereview.chromium.org/1169223002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:111: set_delegate(this);
On 2015/06/10 20:59:20, Devlin wrote:
> On 2015/06/10 20:48:12, kalman wrote:
> > This pattern appears a lot:
> > 
> > // maybe there's a comment, maybe not.
> > ChromeExtensionWebContentsObserver::CreateForWebContents(web_contents);
> >
>
ExtensionWebContentsObserver::GetForWebContents(web_contents)->dispatcher()->set_delegate(this);
> > 
> > it would be nicer to do this in 1 step, plus moot the way I needed to go
back
> > and forth between files figuring out why one is on
> > ChromeExtensionWebContentsObserver and the other is on
> > ExtensionWebContentsObserver. How about:
> > 
> >
>
ExtensionsBrowserClient::GetOrCreateExtensionWebContentsObserver(web_contents);
> > 
> > which, like you've already done in extension_web_contents_observer, you
could
> > alias to look like:
> > 
> > ExtensionWebContentsObserver::GetOrCreateWebContents(web_contents)
> >     ->dispatcher()
> >     ->set_delegate(this);
> > 
> > and in fact, would that let you remove
> > ExtensionAPIClient::AttachWebContentsHelpers, since this would do the same?
> 
> That would work.  My concern is that it makes it a little less deterministic
as
> to when ExtensionWebContentsObservers are created, which we don't want (this
> particular case was a little bit of throwing in the towel on my part, since we
> do the same thing above with SessionTabHelper).  I think I'd prefer to have
the
> normal case be a *failure* so that we know we didn't initialize the
> ExtensionWebContentsObserver when we were supposed to, rather than silence,
> which may result in missing important IPCs...
> 
> WDYT?

As discussed - since the end game here *is* to make it explicit about installing
bindings, we should deal with the temporary extra code and keep it safe.

You may want to comment here, and in ExtensionHosts, where the EWCO is being
created (and obviously, change it eventually).

[Fady Samuel, 2015-06-10 21:37:44]

Thanks for clarifying your change offline, Devlin! LGTM

[Devlin, 2015-06-10 22:05:18]

Patchset #5 (id:120001) has been deleted

[Devlin, 2015-06-10 22:05:34]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2015-06-10 22:05:35]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
fsamuel@chromium.org, kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1169223002/#ps140001
(title: "Latest master")

[commit-bot: I haz the power, 2015-06-10 22:06:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1169223002/140001

[commit-bot: I haz the power, 2015-06-10 23:32:49]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2015-06-10 23:34:02]

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