[Extensions] Clean up the handling of ExtensionHostMsg_Request

chrome/browser/extensions/tab_helper.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/tab_helper.h"
 
 #include "base/logging.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/active_script_controller.h"
 #include "chrome/browser/extensions/activity_log/activity_log.h"
 #include "chrome/browser/extensions/api/declarative_content/chrome_content_rules_registry.h"
 #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"
 #include "chrome/browser/extensions/location_bar_controller.h"
 #include "chrome/browser/extensions/webstore_inline_installer.h"
 #include "chrome/browser/extensions/webstore_inline_installer_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sessions/session_tab_helper.h"
 #include "chrome/browser/shell_integration.h"
 #include "chrome/browser/ui/browser_commands.h"
@@ skipping 16 matching lines @@
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/frame_navigate_params.h"
 #include "extensions/browser/api/declarative/rules_registry_service.h"
 #include "extensions/browser/extension_error.h"
 #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"
 #include "extensions/common/extension_icon_set.h"
 #include "extensions/common/extension_messages.h"
 #include "extensions/common/extension_resource.h"
 #include "extensions/common/extension_urls.h"
 #include "extensions/common/feature_switch.h"
 #include "extensions/common/manifest_handlers/icons_handler.h"
 
 #if defined(OS_WIN)
 #include "chrome/browser/web_applications/web_app_win.h"
 #endif
 
 using content::NavigationController;
 using content::NavigationEntry;
 using content::WebContents;
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(extensions::TabHelper);
 
 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),
       script_executor_(
           new ScriptExecutor(web_contents, &script_execution_observers_)),
       location_bar_controller_(new LocationBarController(web_contents)),
       active_script_controller_(new ActiveScriptController(web_contents)),
       webstore_inline_installer_factory_(new WebstoreInlineInstallerFactory()),
       image_loader_ptr_factory_(this),
       weak_ptr_factory_(this) {
   // The ActiveTabPermissionManager requires a session ID; ensure this
   // WebContents has one.
   SessionTabHelper::CreateForWebContents(web_contents);
   // The Unretained() is safe because ForEachFrame is synchronous.
   web_contents->ForEachFrame(
       base::Bind(&TabHelper::SetTabId, base::Unretained(this)));
   active_tab_permission_granter_.reset(new ActiveTabPermissionGranter(
       web_contents,
       SessionTabHelper::IdForTab(web_contents),
       Profile::FromBrowserContext(web_contents->GetBrowserContext())));
 
   // If more classes need to listen to global content script activity, then
   // a separate routing class with an observer interface should be written.
   profile_ = Profile::FromBrowserContext(web_contents->GetBrowserContext());
 
   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>(
                      &web_contents->GetController()));
 }
 
 TabHelper::~TabHelper() {
   RemoveScriptExecutionObserver(ActivityLog::GetInstance(profile_));
 }
 
@@ skipping 136 matching lines @@
 
 bool TabHelper::OnMessageReceived(const IPC::Message& message) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(TabHelper, message)
     IPC_MESSAGE_HANDLER(ChromeViewHostMsg_DidGetWebApplicationInfo,
                         OnDidGetWebApplicationInfo)
     IPC_MESSAGE_HANDLER(ExtensionHostMsg_InlineWebstoreInstall,
                         OnInlineWebstoreInstall)
     IPC_MESSAGE_HANDLER(ExtensionHostMsg_GetAppInstallState,
                         OnGetAppInstallState);
-    IPC_MESSAGE_HANDLER(ExtensionHostMsg_Request, OnRequest)
     IPC_MESSAGE_HANDLER(ExtensionHostMsg_OnWatchedPageChange,
                         OnWatchedPageChange)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
 }
 
 bool TabHelper::OnMessageReceived(const IPC::Message& message,
                                   content::RenderFrameHost* render_frame_host) {
   bool handled = true;
@@ skipping 137 matching lines @@
     state = extension_misc::kAppStateInstalled;
   else if (disabled_extensions.GetHostedAppByURL(requestor_url))
     state = extension_misc::kAppStateDisabled;
   else
     state = extension_misc::kAppStateNotInstalled;
 
   Send(new ExtensionMsg_GetAppInstallStateResponse(
       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) {
   FOR_EACH_OBSERVER(
       ScriptExecutionObserver,
       script_execution_observers_,
       OnScriptsExecuted(web_contents(), executing_scripts_map, on_url));
 }
 
 void TabHelper::OnWatchedPageChange(
@@ skipping 158 matching lines @@
   }
 }
 
 void TabHelper::SetTabId(content::RenderFrameHost* render_frame_host) {
   render_frame_host->Send(
       new ExtensionMsg_SetTabId(render_frame_host->GetRoutingID(),
                                 SessionTabHelper::IdForTab(web_contents())));
 }
 
 }  // namespace extensions