[Extensions] Clean up the handling of ExtensionHostMsg_Request

chrome/browser/extensions/tab_helper.cc

Index: chrome/browser/extensions/tab_helper.cc
diff --git a/chrome/browser/extensions/tab_helper.cc b/chrome/browser/extensions/tab_helper.cc
index 53c7127bca8d605c61649d00e8d0a554f89c95ca..84539a0a67a50e2601256a44b9a0c88b64b964c2 100644
--- a/chrome/browser/extensions/tab_helper.cc
+++ b/chrome/browser/extensions/tab_helper.cc
@@ -14,6 +14,7 @@
 #include "chrome/browser/extensions/api/extension_action/extension_action_api.h"
 #include "chrome/browser/extensions/api/webstore/webstore_api.h"
 #include "chrome/browser/extensions/bookmark_app_helper.h"
+#include "chrome/browser/extensions/chrome_extension_web_contents_observer.h"
 #include "chrome/browser/extensions/error_console/error_console.h"
 #include "chrome/browser/extensions/extension_tab_util.h"
 #include "chrome/browser/extensions/extension_util.h"
@@ -50,6 +51,7 @@
 #include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
+#include "extensions/browser/extension_web_contents_observer.h"
 #include "extensions/browser/image_loader.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/extension.h"
@@ -75,9 +77,6 @@ namespace extensions {
 TabHelper::TabHelper(content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
       extension_app_(NULL),
-      extension_function_dispatcher_(
-          Profile::FromBrowserContext(web_contents->GetBrowserContext()),
-          this),
       pending_web_app_action_(NONE),
       last_committed_nav_entry_unique_id_(0),
       update_shortcut_on_load_complete_(false),
@@ -105,6 +104,12 @@ TabHelper::TabHelper(content::WebContents* web_contents)
 
   AddScriptExecutionObserver(ActivityLog::GetInstance(profile_));
 
+  // We need an ExtensionWebContentsObserver, so make sure one exists (this is
+  // a no-op if one already does).
+  ChromeExtensionWebContentsObserver::CreateForWebContents(web_contents);
+  ExtensionWebContentsObserver::GetForWebContents(web_contents)->dispatcher()->
+      set_delegate(this);

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

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?

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

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?

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

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).

+
   registrar_.Add(this,
                  content::NOTIFICATION_LOAD_STOP,
                  content::Source<NavigationController>(
@@ -261,7 +266,6 @@ bool TabHelper::OnMessageReceived(const IPC::Message& message) {
                         OnInlineWebstoreInstall)
     IPC_MESSAGE_HANDLER(ExtensionHostMsg_GetAppInstallState,
                         OnGetAppInstallState);
-    IPC_MESSAGE_HANDLER(ExtensionHostMsg_Request, OnRequest)
     IPC_MESSAGE_HANDLER(ExtensionHostMsg_OnWatchedPageChange,
                         OnWatchedPageChange)
     IPC_MESSAGE_UNHANDLED(handled = false)
@@ -419,11 +423,6 @@ void TabHelper::OnGetAppInstallState(const GURL& requestor_url,
       return_route_id, state, callback_id));
 }
 
-void TabHelper::OnRequest(const ExtensionHostMsg_Request_Params& request) {
-  extension_function_dispatcher_.Dispatch(request,
-                                          web_contents()->GetRenderViewHost());
-}
-
 void TabHelper::OnContentScriptsExecuting(
     const ScriptExecutionObserver::ExecutingScriptsMap& executing_scripts_map,
     const GURL& on_url) {