Convert Web Store Inline Install IPCs to mojo

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/memory/ptr_util.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 15 matching lines @@
 #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"
 #include "chrome/browser/ui/browser_dialogs.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/web_applications/web_app.h"
 #include "chrome/common/extensions/api/webstore/webstore_api_constants.h"
-#include "chrome/common/extensions/chrome_extension_messages.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "chrome/common/extensions/manifest_handlers/app_launch_info.h"
 #include "chrome/common/render_messages.h"
 #include "chrome/common/url_constants.h"
 #include "content/public/browser/invalidate_type.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
@@ skipping 10 matching lines @@
 #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"
+#include "mojo/public/cpp/bindings/strong_binding.h"
+#include "services/service_manager/public/cpp/interface_provider.h"
+#include "services/service_manager/public/cpp/interface_registry.h"
 #include "url/url_constants.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 {
 
 // A helper class to watch the progress of inline installation and update the
 // renderer. Owned by the TabHelper.
 class TabHelper::InlineInstallObserver : public InstallObserver {
  public:
   InlineInstallObserver(TabHelper* tab_helper,
                         content::BrowserContext* browser_context,
                         int routing_id,
                         const ExtensionId& extension_id,
                         bool observe_download_progress,
                         bool observe_install_stage)
       : tab_helper_(tab_helper),
-        routing_id_(routing_id),
         extension_id_(extension_id),
         observe_download_progress_(observe_download_progress),
         observe_install_stage_(observe_install_stage),
         install_observer_(this) {
     DCHECK(tab_helper);
     DCHECK(observe_download_progress || observe_install_stage);
     InstallTracker* install_tracker =
         InstallTrackerFactory::GetForBrowserContext(browser_context);
     if (install_tracker)
       install_observer_.Add(install_tracker);
   }
   ~InlineInstallObserver() override {}
 
  private:
   // InstallObserver:
   void OnBeginExtensionDownload(const ExtensionId& extension_id) override {
     SendInstallStageChangedMessage(extension_id,
                                    api::webstore::INSTALL_STAGE_DOWNLOADING);
   }
   void OnDownloadProgress(const ExtensionId& extension_id,
                           int percent_downloaded) override {
     if (observe_download_progress_ && extension_id == extension_id_) {
-      tab_helper_->Send(new ExtensionMsg_InlineInstallDownloadProgress(
-          routing_id_, percent_downloaded));
+      tab_helper_->inline_install_status_->InlineInstallDownloadProgress(
+          percent_downloaded);
     }
   }
   void OnBeginCrxInstall(const ExtensionId& extension_id) override {
     SendInstallStageChangedMessage(extension_id,
                                    api::webstore::INSTALL_STAGE_INSTALLING);
   }
   void OnShutdown() override { install_observer_.RemoveAll(); }
 
   void SendInstallStageChangedMessage(const ExtensionId& extension_id,
                                       api::webstore::InstallStage stage) {
-    if (observe_install_stage_ && extension_id == extension_id_) {
-      tab_helper_->Send(
-          new ExtensionMsg_InlineInstallStageChanged(routing_id_, stage));
-    }
+    if (observe_install_stage_ && extension_id == extension_id_)
+      tab_helper_->inline_install_status_->InlineInstallStageChanged(stage);
   }
 
   // The owning TabHelper (guaranteed to be valid).
   TabHelper* const tab_helper_;
 
-  // The routing id to use in sending IPC updates.
-  int routing_id_;
-
   // The id of the extension to observe.
   ExtensionId extension_id_;
 
   // Whether or not to observe download/install progress.
   const bool observe_download_progress_;
   const bool observe_install_stage_;
 
   ScopedObserver<InstallTracker, InstallObserver> install_observer_;
 
   DISALLOW_COPY_AND_ASSIGN(InlineInstallObserver);
 };
 
+TabHelper::~TabHelper() {
+  RemoveScriptExecutionObserver(ActivityLog::GetInstance(profile_));
+}
+
+// static
+void BindInlineInstallRequest(content::WebContents* web_contents,
+                              mojom::InlineInstallRequest request) {
+  mojo::MakeStrongBinding(base::MakeUnique<TabHelper>(web_contents),
+                          std::move(request));

[Devlin, 2017/03/31 14:18:39]

This won't be quite right, I think.  A TabHelper is designed to be a
one-instance-per-web-contents object, but this will create a new TabHelper for
each frame that makes a request.

Looking around, I found content::WebContentsBindingSet and
content::WebContentsFrameBindingSet - I think that's closer to what we want.

https://cs.chromium.org/chromium/src/content/public/browser/web_contents_bind...

[catmullings, 2017/04/06 20:50:15]

Done.

[catmullings, 2017/04/26 20:33:46]

Just an FYI, the WebContentsFrameBindingSet is meant for (channel) associated
interface bindings. Since ordering is not a concern among these IPCs, this
binding set is a bit of an overkill. That being said, the
WebContentsFrameBindingSet handles many things behind the scenes that are useful
in this case over a WebContents, so migrating to a vanilla binding set might be
involved. 

+}
+
 TabHelper::TabHelper(content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
       profile_(Profile::FromBrowserContext(web_contents->GetBrowserContext())),
       extension_app_(NULL),
       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)),
@@ skipping 24 matching lines @@
   ChromeExtensionWebContentsObserver::CreateForWebContents(web_contents);
   ExtensionWebContentsObserver::GetForWebContents(web_contents)->dispatcher()->
       set_delegate(this);
 
   BookmarkManagerPrivateDragEventRouter::CreateForWebContents(web_contents);
 
   registrar_.Add(this,
                  content::NOTIFICATION_LOAD_STOP,
                  content::Source<NavigationController>(
                      &web_contents->GetController()));
-}
 
-TabHelper::~TabHelper() {
-  RemoveScriptExecutionObserver(ActivityLog::GetInstance(profile_));
+  content::RenderFrameHost* host = web_contents->GetMainFrame();
+  if (host) {
+    host->GetRemoteInterfaces()->GetInterface(&inline_install_status_);
+
+    host->GetInterfaceRegistry()->AddInterface(
+        base::Bind(&TabHelper::BindInlineInstallRequest, web_contents));

[catmullings, 2017/03/31 00:08:21]

This line is causing a linker error:

error: undefined reference to
'extensions::TabHelper::BindInlineInstallRequest(content::WebContents*,
mojo::InterfaceRequest<extensions::mojom::InlineInstall>)'

Devlin, would you have any idea what might be the cause? Maybe an improper setup
of the inline_install.mojom dependancy in the BUILD.gn files?

[Devlin, 2017/03/31 14:18:39]

This error is because BindInlineInstallRequest is declared as a method of
TabHelper (in the .h file, and specified here by
TabHelper::BindInlineInstallRequest), but it's defined as only "void
BindInlineInstallRequest" rather than "void
TabHelper::BindInlineInstallRequest". The result is that there's a declared
method in TabHelper (so the compiler knows it should exist) and a defined method
in the global namespace (that ends up never being used), but during linking, the
compiler can't find the definition of the TabHelper version.

[catmullings, 2017/04/06 20:50:15]

That makes sense. Thanks!

I assume that WebContentsFrameBindingSet handles adding the bindings to the
registry, but it'll be worth confirming this with mojo folks once they are added
to the cl review.

+  }
 }
 
 void TabHelper::CreateApplicationShortcuts() {
   DCHECK(CanCreateApplicationShortcuts());
   if (pending_web_app_action_ != NONE)
     return;
 
   // Start fetching web app info for CreateApplicationShortcut dialog and show
   // the dialog when the data is available in OnDidGetApplicationInfo.
   GetApplicationInfo(CREATE_SHORTCUT);
@@ skipping 150 matching lines @@
                         OnDidGetWebApplicationInfo)
     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;
   IPC_BEGIN_MESSAGE_MAP_WITH_PARAM(TabHelper, message, render_frame_host)
-    IPC_MESSAGE_HANDLER(ExtensionHostMsg_InlineWebstoreInstall,
-                        OnInlineWebstoreInstall)
     IPC_MESSAGE_HANDLER(ExtensionHostMsg_GetAppInstallState,
                         OnGetAppInstallState)
     IPC_MESSAGE_HANDLER(ExtensionHostMsg_ContentScriptsExecuting,
                         OnContentScriptsExecuting)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
 }
 
 void TabHelper::DidCloneToNewWebContents(WebContents* old_web_contents,
                                          WebContents* new_web_contents) {
   // When the WebContents that this is attached to is cloned, give the new clone
   // a TabHelper and copy state over.
   CreateForWebContents(new_web_contents);
   TabHelper* new_helper = FromWebContents(new_web_contents);
 
   new_helper->SetExtensionApp(extension_app());
   new_helper->extension_app_icon_ = extension_app_icon_;
 }
 
-void TabHelper::OnDidGetWebApplicationInfo(const WebApplicationInfo& info) {
-  web_app_info_ = info;
-
-  NavigationEntry* entry =
-      web_contents()->GetController().GetLastCommittedEntry();
-  if (!entry || last_committed_nav_entry_unique_id_ != entry->GetUniqueID())
-    return;
-  last_committed_nav_entry_unique_id_ = 0;
-
-  switch (pending_web_app_action_) {
-#if !defined(OS_MACOSX)
-    case CREATE_SHORTCUT: {
-      chrome::ShowCreateWebAppShortcutsDialog(
-          web_contents()->GetTopLevelNativeWindow(),
-          web_contents());
-      break;
-    }
-#endif
-    case CREATE_HOSTED_APP: {
-      if (web_app_info_.app_url.is_empty())
-        web_app_info_.app_url = web_contents()->GetURL();
-
-      if (web_app_info_.title.empty())
-        web_app_info_.title = web_contents()->GetTitle();
-      if (web_app_info_.title.empty())
-        web_app_info_.title = base::UTF8ToUTF16(web_app_info_.app_url.spec());
-
-      bookmark_app_helper_.reset(
-          new BookmarkAppHelper(profile_, web_app_info_, web_contents()));
-      bookmark_app_helper_->Create(base::Bind(
-          &TabHelper::FinishCreateBookmarkApp, weak_ptr_factory_.GetWeakPtr()));
-      break;
-    }
-    case UPDATE_SHORTCUT: {
-      web_app::UpdateShortcutForTabContents(web_contents());
-      break;
-    }
-    default:
-      NOTREACHED();
-      break;
-  }
-
-  // The hosted app action will be cleared once the installation completes or
-  // fails.
-  if (pending_web_app_action_ != CREATE_HOSTED_APP)
-    pending_web_app_action_ = NONE;
-}
-
-void TabHelper::OnInlineWebstoreInstall(content::RenderFrameHost* host,

[catmullings, 2017/03/31 00:08:21]

Note that the original definition of OnInlineWebstoreInstall had a parameter for
the RenderFrameHost which is handled and tracked behind-the-scenes by the IPC
mechanisms. Because handling/tracking the RenderFrameHost needs to be more
explicit in mojo, note how this has been done on line 404 and if this SGTY. 

-                                        int install_id,
-                                        int return_route_id,
-                                        const std::string& webstore_item_id,
-                                        int listeners_mask) {
+void TabHelper::InlineWebstoreInstall(int install_id,
+                                      int return_route_id,
+                                      const std::string& webstore_item_id,
+                                      int listeners_mask) {
+  content::RenderFrameHost* host = web_contents()->GetMainFrame();
   GURL requestor_url(host->GetLastCommittedURL());
   // Check that the listener is reasonable. We should never get anything other
   // than an install stage listener, a download listener, or both.
   // The requestor_url should also be valid, and the renderer should disallow
   // child frames from sending the IPC.
   if ((listeners_mask & ~(api::webstore::INSTALL_STAGE_LISTENER |
                           api::webstore::DOWNLOAD_PROGRESS_LISTENER)) != 0 ||
       !requestor_url.is_valid() || requestor_url == url::kAboutBlankURL ||
       host->GetParent()) {
     NOTREACHED();
     return;
   }
 
   if (pending_inline_installations_.count(webstore_item_id) != 0) {
-    Send(new ExtensionMsg_InlineWebstoreInstallResponse(
-        return_route_id, install_id, false,
-        webstore_install::kInstallInProgressError,
-        webstore_install::INSTALL_IN_PROGRESS));
+    inline_install_status_->InlineWebstoreInstallResponse(

[Devlin, 2017/03/31 14:18:39]

note: see also comment in mojom file.

[catmullings, 2017/04/06 20:50:15]

Done.

+        install_id, false, webstore_install::kInstallInProgressError,
+        webstore_install::INSTALL_IN_PROGRESS);
     return;
   }
 
   pending_inline_installations_.insert(webstore_item_id);
   // Inform the Webstore API that an inline install is happening, in case the
   // page requested status updates.
   ExtensionRegistry* registry = ExtensionRegistry::Get(profile_);
   if (registry->disabled_extensions().Contains(webstore_item_id) &&
       (ExtensionPrefs::Get(profile_)->GetDisableReasons(webstore_item_id) &
            Extension::DISABLE_PERMISSIONS_INCREASE) != 0) {
@@ skipping 28 matching lines @@
     WebstoreStandaloneInstaller::Callback callback = base::Bind(
         &TabHelper::OnInlineInstallComplete, weak_ptr_factory_.GetWeakPtr(),
         install_id, return_route_id, webstore_item_id);
     scoped_refptr<WebstoreInlineInstaller> installer(
         webstore_inline_installer_factory_->CreateInstaller(
             web_contents(), host, webstore_item_id, requestor_url, callback));
     installer->BeginInstall();
   }
 }
 
+void TabHelper::OnDidGetWebApplicationInfo(const WebApplicationInfo& info) {

[Devlin, 2017/03/31 14:18:39]

Is there a reason to move this method?  If not, we should avoid it in this CL
because it makes the diff harder to process (it's not obvious if anything in the
method changed, or if it's just copy-paste).  If it's just for stylistic reasons
(e.g. matching the ordering of declaration to the order of definition), let's go
ahead and do that in a separate CL.

[catmullings, 2017/04/06 20:50:15]

Done.

+  web_app_info_ = info;
+
+  NavigationEntry* entry =
+      web_contents()->GetController().GetLastCommittedEntry();
+  if (!entry || last_committed_nav_entry_unique_id_ != entry->GetUniqueID())
+    return;
+  last_committed_nav_entry_unique_id_ = 0;
+
+  switch (pending_web_app_action_) {
+#if !defined(OS_MACOSX)
+    case CREATE_SHORTCUT: {
+      chrome::ShowCreateWebAppShortcutsDialog(
+          web_contents()->GetTopLevelNativeWindow(), web_contents());
+      break;
+    }
+#endif
+    case CREATE_HOSTED_APP: {
+      if (web_app_info_.app_url.is_empty())
+        web_app_info_.app_url = web_contents()->GetURL();
+
+      if (web_app_info_.title.empty())
+        web_app_info_.title = web_contents()->GetTitle();
+      if (web_app_info_.title.empty())
+        web_app_info_.title = base::UTF8ToUTF16(web_app_info_.app_url.spec());
+
+      bookmark_app_helper_.reset(
+          new BookmarkAppHelper(profile_, web_app_info_, web_contents()));
+      bookmark_app_helper_->Create(base::Bind(
+          &TabHelper::FinishCreateBookmarkApp, weak_ptr_factory_.GetWeakPtr()));
+      break;
+    }
+    case UPDATE_SHORTCUT: {
+      web_app::UpdateShortcutForTabContents(web_contents());
+      break;
+    }
+    default:
+      NOTREACHED();
+      break;
+  }
+
+  // The hosted app action will be cleared once the installation completes or
+  // fails.
+  if (pending_web_app_action_ != CREATE_HOSTED_APP)
+    pending_web_app_action_ = NONE;
+}
+
 void TabHelper::OnGetAppInstallState(content::RenderFrameHost* host,
                                      const GURL& requestor_url,
                                      int return_route_id,
                                      int callback_id) {
   ExtensionRegistry* registry =
       ExtensionRegistry::Get(web_contents()->GetBrowserContext());
   const ExtensionSet& extensions = registry->enabled_extensions();
   const ExtensionSet& disabled_extensions = registry->disabled_extensions();
 
   std::string state;
@@ skipping 103 matching lines @@
 
 void TabHelper::OnInlineInstallComplete(int install_id,
                                         int return_route_id,
                                         const ExtensionId& extension_id,
                                         bool success,
                                         const std::string& error,
                                         webstore_install::Result result) {
   DCHECK_EQ(1u, pending_inline_installations_.count(extension_id));
   pending_inline_installations_.erase(extension_id);
   install_observers_.erase(extension_id);
-  Send(new ExtensionMsg_InlineWebstoreInstallResponse(
-      return_route_id,
-      install_id,
-      success,
-      success ? std::string() : error,
-      result));
+  inline_install_status_->InlineWebstoreInstallResponse(
+      install_id, success, success ? std::string() : error, result);
 }
 
 WebContents* TabHelper::GetAssociatedWebContents() const {
   return web_contents();
 }
 
 void TabHelper::OnExtensionUnloaded(
     content::BrowserContext* browser_context,
     const Extension* extension,
     UnloadedExtensionInfo::Reason reason) {
@@ skipping 33 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