Convert Web Store Inline Install IPCs to mojo

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 f85e854b63c31cfd75b15b6b227605ca4877b864..872b79fac98f7a889171b606c3ce443bbf9bcf9f 100644
--- a/chrome/browser/extensions/tab_helper.cc
+++ b/chrome/browser/extensions/tab_helper.cc
@@ -33,7 +33,6 @@
 #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"
@@ -64,6 +63,9 @@
 #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)
@@ -89,7 +91,6 @@ class TabHelper::InlineInstallObserver : public InstallObserver {
                         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),
@@ -112,8 +113,8 @@ class TabHelper::InlineInstallObserver : public InstallObserver {
   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 {
@@ -124,18 +125,13 @@ class TabHelper::InlineInstallObserver : public InstallObserver {
 
   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_;
 
@@ -148,6 +144,17 @@ class TabHelper::InlineInstallObserver : public InstallObserver {
   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())),
@@ -192,10 +199,14 @@ TabHelper::TabHelper(content::WebContents* web_contents)
                  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() {
@@ -366,8 +377,6 @@ 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,
@@ -388,59 +397,11 @@ void TabHelper::DidCloneToNewWebContents(WebContents* old_web_contents,
   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.
@@ -455,10 +416,9 @@ void TabHelper::OnInlineWebstoreInstall(content::RenderFrameHost* host,
   }
 
   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;
   }
 
@@ -507,6 +467,53 @@ void TabHelper::OnInlineWebstoreInstall(content::RenderFrameHost* host,
   }
 }
 
+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,
@@ -630,12 +637,8 @@ void TabHelper::OnInlineInstallComplete(int install_id,
   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 {