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 d04d9053597e1feaaf0252d438928168d0f7bc32..81d3051ff0eb56e40119c6c5f581dd5ede7c8313 100644
--- a/chrome/browser/extensions/tab_helper.cc
+++ b/chrome/browser/extensions/tab_helper.cc
@@ -32,7 +32,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"
@@ -63,6 +62,7 @@
 #include "extensions/common/extension_urls.h"
 #include "extensions/common/feature_switch.h"
 #include "extensions/common/manifest_handlers/icons_handler.h"
+#include "services/service_manager/public/cpp/interface_provider.h"
 #include "url/url_constants.h"
 
 #if defined(OS_WIN)
@@ -83,12 +83,10 @@ 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),
@@ -111,8 +109,10 @@ 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));
+      auto iter =
+          tab_helper_->inline_install_progress_listeners_.find(extension_id);
+      DCHECK(iter != tab_helper_->inline_install_progress_listeners_.end());

[palmer, 2017/05/08 23:39:03]

DCHECK suggests to me that this would only happen in case of programmer error,
but I wonder if it is possible for |find| to return |end| legitimately (due to
the slings and arrows of outrageous fortune at runtime)? Should this be

  if (iter != tab_helper_->inline_install_progress_listeners_.end()) {
    iter->second->InlineInstallDownloadProgress(percent_downloaded);
  }

? But, if it really would only happen due to programmer error, then I'd agree
that a DCHECK is right.

[catmullings, 2017/05/23 02:38:37]

My hunch is that this case would never happen, hence the use of DCHECK. Devlin,
can you confirm?

[Devlin, 2017/05/23 15:08:47]

The way this is written, this should never happen.  The TabHelper owns the
inline install progress listeners and the inline install observers, and should
erase them at the same time.  If one exists, so should the other.  So DCHECK
here should be correct.

+      iter->second->InlineInstallDownloadProgress(percent_downloaded);
     }
   }
   void OnBeginCrxInstall(const ExtensionId& extension_id) override {
@@ -124,17 +124,16 @@ 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));
+      auto iter =
+          tab_helper_->inline_install_progress_listeners_.find(extension_id);
+      DCHECK(iter != tab_helper_->inline_install_progress_listeners_.end());

[palmer, 2017/05/08 23:39:03]

Same question here.

+      iter->second->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_;
 
@@ -147,6 +146,10 @@ class TabHelper::InlineInstallObserver : public InstallObserver {
   DISALLOW_COPY_AND_ASSIGN(InlineInstallObserver);
 };
 
+TabHelper::~TabHelper() {
+  RemoveScriptExecutionObserver(ActivityLog::GetInstance(profile_));
+}
+
 TabHelper::TabHelper(content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
       profile_(Profile::FromBrowserContext(web_contents->GetBrowserContext())),
@@ -159,6 +162,7 @@ TabHelper::TabHelper(content::WebContents* web_contents)
       extension_action_runner_(new ExtensionActionRunner(web_contents)),
       webstore_inline_installer_factory_(new WebstoreInlineInstallerFactory()),
       registry_observer_(this),
+      bindings_(web_contents, this),
       image_loader_ptr_factory_(this),
       weak_ptr_factory_(this) {
   // The ActiveTabPermissionManager requires a session ID; ensure this
@@ -192,10 +196,6 @@ TabHelper::TabHelper(content::WebContents* web_contents)
                      &web_contents->GetController()));
 }
 
-TabHelper::~TabHelper() {
-  RemoveScriptExecutionObserver(ActivityLog::GetInstance(profile_));
-}
-
 void TabHelper::CreateHostedAppFromWebContents() {
   DCHECK(CanCreateBookmarkApp());
   if (pending_web_app_action_ != NONE)
@@ -346,8 +346,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,
@@ -408,11 +406,17 @@ void TabHelper::OnDidGetWebApplicationInfo(const WebApplicationInfo& info) {
     pending_web_app_action_ = NONE;
 }
 
-void TabHelper::OnInlineWebstoreInstall(content::RenderFrameHost* host,
-                                        int install_id,
-                                        int return_route_id,
-                                        const std::string& webstore_item_id,
-                                        int listeners_mask) {
+void TabHelper::DoInlineInstall(
+    int install_id,
+    const std::string& webstore_item_id,
+    int listeners_mask,
+    mojom::InlineInstallProgressListenerPtr install_progress_listener) {
+  content::RenderFrameHost* host = web_contents()->GetMainFrame();
+  if (bindings_.GetCurrentTargetFrame() != host) {
+    NOTREACHED();
+    return;
+  }
+
   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.
@@ -420,39 +424,38 @@ void TabHelper::OnInlineWebstoreInstall(content::RenderFrameHost* host,
   // 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()) {
+      !requestor_url.is_valid() || requestor_url == url::kAboutBlankURL) {
     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));
+  if (base::ContainsKey(inline_install_progress_listeners_, webstore_item_id)) {
+    install_progress_listener->InlineInstallResponse(
+        install_id, false, webstore_install::kInstallInProgressError,
+        webstore_install::INSTALL_IN_PROGRESS);
     return;
   }
 
-  pending_inline_installations_.insert(webstore_item_id);
+  inline_install_progress_listeners_[webstore_item_id] =
+      std::move(install_progress_listener);
   // 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) {
-      // The extension was disabled due to permissions increase. Prompt for
-      // re-enable.
-      // TODO(devlin): We should also prompt for re-enable for other reasons,
-      // like user-disabled.
-      // For clarity, explicitly end any prior reenable process.
-      extension_reenabler_.reset();
-      extension_reenabler_ = ExtensionReenabler::PromptForReenable(
-          registry->disabled_extensions().GetByID(webstore_item_id), profile_,
-          web_contents(), requestor_url,
-          base::Bind(&TabHelper::OnReenableComplete,
-                     weak_ptr_factory_.GetWeakPtr(), install_id,
-                     return_route_id, webstore_item_id));
+       Extension::DISABLE_PERMISSIONS_INCREASE) != 0) {
+    // The extension was disabled due to permissions increase. Prompt for
+    // re-enable.
+    // TODO(devlin): We should also prompt for re-enable for other reasons,
+    // like user-disabled.
+    // For clarity, explicitly end any prior reenable process.
+    extension_reenabler_.reset();
+    extension_reenabler_ = ExtensionReenabler::PromptForReenable(
+        registry->disabled_extensions().GetByID(webstore_item_id), profile_,
+        web_contents(), requestor_url,
+        base::Bind(&TabHelper::OnReenableComplete,
+                   weak_ptr_factory_.GetWeakPtr(), install_id,
+                   webstore_item_id));
   } else {
     // TODO(devlin): We should adddress the case of the extension already
     // being installed and enabled.
@@ -464,14 +467,13 @@ void TabHelper::OnInlineWebstoreInstall(content::RenderFrameHost* host,
       DCHECK_EQ(0u, install_observers_.count(webstore_item_id));
       install_observers_[webstore_item_id] =
           base::MakeUnique<InlineInstallObserver>(
-              this, web_contents()->GetBrowserContext(), return_route_id,
-              webstore_item_id, observe_download_progress,
-              observe_install_stage);
+              this, web_contents()->GetBrowserContext(), webstore_item_id,
+              observe_download_progress, observe_install_stage);
     }
 
     WebstoreStandaloneInstaller::Callback callback = base::Bind(
         &TabHelper::OnInlineInstallComplete, weak_ptr_factory_.GetWeakPtr(),
-        install_id, return_route_id, webstore_item_id);
+        install_id, webstore_item_id);
     scoped_refptr<WebstoreInlineInstaller> installer(
         webstore_inline_installer_factory_->CreateInstaller(
             web_contents(), host, webstore_item_id, requestor_url, callback));
@@ -562,7 +564,6 @@ WindowController* TabHelper::GetExtensionWindowController() const  {
 }
 
 void TabHelper::OnReenableComplete(int install_id,
-                                   int return_route_id,
                                    const ExtensionId& extension_id,
                                    ExtensionReenabler::ReenableResult result) {
   // Map the re-enable results to webstore-install results.
@@ -585,7 +586,7 @@ void TabHelper::OnReenableComplete(int install_id,
       break;
   }
 
-  OnInlineInstallComplete(install_id, return_route_id, extension_id,
+  OnInlineInstallComplete(install_id, extension_id,
                           result == ExtensionReenabler::REENABLE_SUCCESS, error,
                           webstore_result);
   // Note: ExtensionReenabler contained the callback with the curried-in
@@ -594,20 +595,16 @@ void TabHelper::OnReenableComplete(int install_id,
 }
 
 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));
+  auto iter = inline_install_progress_listeners_.find(extension_id);
+  DCHECK(iter != inline_install_progress_listeners_.end());

[palmer, 2017/05/08 23:39:03]

Same question here.

+  iter->second->InlineInstallResponse(install_id, success,
+                                      success ? std::string() : error, result);
+  inline_install_progress_listeners_.erase(iter);
 }
 
 WebContents* TabHelper::GetAssociatedWebContents() const {