Convert Web Store Inline Install IPCs to mojo

chrome/renderer/extensions/webstore_bindings.cc

Index: chrome/renderer/extensions/webstore_bindings.cc
diff --git a/chrome/renderer/extensions/webstore_bindings.cc b/chrome/renderer/extensions/webstore_bindings.cc
index 2a3c2aa6dd7a01f5fcf61bc2b8d9958482aabe7a..70f4e7c814d96528509da4a8852d3511a364531e 100644
--- a/chrome/renderer/extensions/webstore_bindings.cc
+++ b/chrome/renderer/extensions/webstore_bindings.cc
@@ -9,12 +9,15 @@
 #include "base/macros.h"
 #include "base/strings/string_util.h"
 #include "chrome/common/extensions/api/webstore/webstore_api_constants.h"
-#include "chrome/common/extensions/chrome_extension_messages.h"
 #include "components/crx_file/id_util.h"
+#include "content/public/common/associated_interface_provider.h"
 #include "content/public/renderer/render_frame.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_urls.h"
 #include "extensions/renderer/script_context.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 "third_party/WebKit/public/web/WebDocument.h"
 #include "third_party/WebKit/public/web/WebElement.h"
 #include "third_party/WebKit/public/web/WebLocalFrame.h"
@@ -45,17 +48,91 @@ const char kInvalidWebstoreItemUrlError[] =
 
 // chrome.webstore.install() calls generate an install ID so that the install's
 // callbacks may be fired when the browser notifies us of install completion
-// (successful or not) via OnInlineWebstoreInstallResponse.
+// (successful or not) via InlineInstallResponse.
 int g_next_install_id = 0;
 
 } // anonymous namespace
 
+class WebstoreBindings::WebstoreBindingsHelper
+    : public ObjectBackedNativeHandler,
+      public mojom::InlineInstallStatus {
+ public:
+  static void Create(ScriptContext* context,
+                     mojom::InlineInstallStatusRequest request);
+
+ private:
+  WebstoreBindingsHelper(ScriptContext* context,
+                         mojom::InlineInstallStatusRequest request);
+  ~WebstoreBindingsHelper() override;
+
+  void InlineInstallStageChanged(api::webstore::InstallStage stage) override;
+  void InlineInstallDownloadProgress(int percent_downloaded) override;
+
+  mojo::Binding<mojom::InlineInstallStatus> binding_;
+};
+
+// static
+void WebstoreBindings::WebstoreBindingsHelper::Create(
+    ScriptContext* context,
+    mojom::InlineInstallStatusRequest request) {
+  new WebstoreBindingsHelper(context, std::move(request));
+}
+
+WebstoreBindings::WebstoreBindingsHelper::WebstoreBindingsHelper(
+    ScriptContext* context,
+    mojom::InlineInstallStatusRequest request)
+    : ObjectBackedNativeHandler(context), binding_(this, std::move(request)) {}
+
+WebstoreBindings::WebstoreBindingsHelper::~WebstoreBindingsHelper() = default;
+
+void WebstoreBindings::WebstoreBindingsHelper::InlineInstallStageChanged(
+    api::webstore::InstallStage stage) {
+  const char* stage_string = NULL;
+  api::webstore::InstallStage install_stage =
+      static_cast<api::webstore::InstallStage>(stage);
+  switch (install_stage) {
+    case api::webstore::INSTALL_STAGE_DOWNLOADING:
+      stage_string = api::webstore::kInstallStageDownloading;
+      break;
+    case api::webstore::INSTALL_STAGE_INSTALLING:
+      stage_string = api::webstore::kInstallStageInstalling;
+      break;
+  }
+  v8::Isolate* isolate = context()->isolate();
+  v8::HandleScope handle_scope(isolate);
+  v8::Context::Scope context_scope(context()->v8_context());
+  v8::Local<v8::Value> argv[] = {
+      v8::String::NewFromUtf8(isolate, stage_string)};
+  context()->module_system()->CallModuleMethodSafe(
+      "webstore", "onInstallStageChanged", arraysize(argv), argv);
+}
+
+void WebstoreBindings::WebstoreBindingsHelper::InlineInstallDownloadProgress(
+    int percent_downloaded) {
+  v8::Isolate* isolate = context()->isolate();
+  v8::HandleScope handle_scope(isolate);
+  v8::Context::Scope context_scope(context()->v8_context());
+  v8::Local<v8::Value> argv[] = {
+      v8::Number::New(isolate, percent_downloaded / 100.0)};
+  context()->module_system()->CallModuleMethodSafe(
+      "webstore", "onDownloadProgress", arraysize(argv), argv);
+}
+
+ScriptContext* WebstoreBindings::context_ = NULL;
+
 WebstoreBindings::WebstoreBindings(ScriptContext* context)
     : ObjectBackedNativeHandler(context) {
+  WebstoreBindings::SetContext(context);
   RouteFunction("Install", "webstore",
                 base::Bind(&WebstoreBindings::Install, base::Unretained(this)));
+  context->GetRenderFrame()->GetRemoteAssociatedInterfaces()->GetInterface(
+      &inline_install_);
+  context->GetRenderFrame()->GetInterfaceRegistry()->AddInterface(
+      base::Bind(&WebstoreBindingsHelper::Create, context));

[Devlin, 2017/04/24 20:08:32]

This isn't necessary what we want.  In particular, we don't necessarily want to
strictly bind all requests for a frame to a single access point.  Instead, we
want each context to be able to receive its own messages.

Previously, the way we accomplished this was by having WebstoreBindings inherit
ChromeV8ExtensionHandler - a poorly named class that let these receive IPC
messages directly.  Now that we're moving away from these IPCs, we need a
different approach.

The issue with this current way is that a) it relies too much on which contexts
may or may not be created/valid and the order in which they are, and b) it binds
all requests to a single context.

My suggestion to fix this would be to have the DoInlineInstall() message take an
interface to an InlineInstallStatus:

DoInlineInstall(
    int32 install_id, string webstore_item_id,
    int32 listeners_mask, InlineInstallStatus install_status) =>
       (int32 install_id, bool success, string error,
        WebstoreInstallResult result);

(Ken pointed me to
https://chromium.googlesource.com/chromium/src/+/master/mojo/public/cpp/bindi...
for references on passing interfaces)

Then, the TabHelper on the browser side uses that passed interface to call these
update methods.

On the renderer side, we'd have the WebstoreBindingsHelper inherit from
InlineInstallStatus just as it does now, but be owned by the WebstoreBindings
and passed as an argument to the DoInlineInstall() method.

This allows us to have a separate InlineInstallStatus for each context in the
frame, similar to what we do now, and couples the lifetime of the
InlineInstallStatus to the WebstoreBindings (which is good from a deterministic
point of view).

Does that make sense?

(Separately - maybe we should rename InlineInstallStatus to
InlineInstallProgressListener or something?)

[catmullings, 2017/04/26 20:36:34]

Done.

 }
 
+WebstoreBindings::~WebstoreBindings() {}
+
 void WebstoreBindings::Install(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
   content::RenderFrame* render_frame = context()->GetRenderFrame();
@@ -91,9 +168,9 @@ void WebstoreBindings::Install(
 
   int install_id = g_next_install_id++;
 
-  Send(new ExtensionHostMsg_InlineWebstoreInstall(
-      render_frame->GetRoutingID(), install_id, GetRoutingID(),
-      webstore_item_id, listener_mask));
+  inline_install_->DoInlineInstall(
+      install_id, webstore_item_id, listener_mask,
+      base::Bind(&WebstoreBindings::InlineInstallResponse));
 
   args.GetReturnValue().Set(static_cast<int32_t>(install_id));
 }
@@ -187,26 +264,18 @@ bool WebstoreBindings::GetWebstoreItemIdFromFrame(
 }
 
 bool WebstoreBindings::OnMessageReceived(const IPC::Message& message) {
-  IPC_BEGIN_MESSAGE_MAP(WebstoreBindings, message)
-    IPC_MESSAGE_HANDLER(ExtensionMsg_InlineWebstoreInstallResponse,
-                        OnInlineWebstoreInstallResponse)
-    IPC_MESSAGE_HANDLER(ExtensionMsg_InlineInstallStageChanged,
-                        OnInlineInstallStageChanged)
-    IPC_MESSAGE_HANDLER(ExtensionMsg_InlineInstallDownloadProgress,
-                        OnInlineInstallDownloadProgress)
-    IPC_MESSAGE_UNHANDLED(CHECK(false) << "Unhandled IPC message")
-  IPC_END_MESSAGE_MAP()
   return true;
 }
 
-void WebstoreBindings::OnInlineWebstoreInstallResponse(
-    int install_id,
-    bool success,
-    const std::string& error,
-    webstore_install::Result result) {
-  v8::Isolate* isolate = context()->isolate();
+// static
+void WebstoreBindings::InlineInstallResponse(int install_id,
+                                             bool success,
+                                             const std::string& error,
+                                             webstore_install::Result result) {
+  v8::Isolate* isolate = WebstoreBindings::GetContext()->isolate();
   v8::HandleScope handle_scope(isolate);
-  v8::Context::Scope context_scope(context()->v8_context());
+  v8::Context::Scope context_scope(
+      WebstoreBindings::GetContext()->v8_context());
   v8::Local<v8::Value> argv[] = {
     v8::Integer::New(isolate, install_id),
     v8::Boolean::New(isolate, success),
@@ -214,39 +283,8 @@ void WebstoreBindings::OnInlineWebstoreInstallResponse(
     v8::String::NewFromUtf8(
         isolate, api::webstore::kInstallResultCodes[static_cast<int>(result)])
   };
-  context()->module_system()->CallModuleMethodSafe(
+  WebstoreBindings::GetContext()->module_system()->CallModuleMethodSafe(
       "webstore", "onInstallResponse", arraysize(argv), argv);
 }
 
-void WebstoreBindings::OnInlineInstallStageChanged(int stage) {
-  const char* stage_string = NULL;
-  api::webstore::InstallStage install_stage =
-      static_cast<api::webstore::InstallStage>(stage);
-  switch (install_stage) {
-    case api::webstore::INSTALL_STAGE_DOWNLOADING:
-      stage_string = api::webstore::kInstallStageDownloading;
-      break;
-    case api::webstore::INSTALL_STAGE_INSTALLING:
-      stage_string = api::webstore::kInstallStageInstalling;
-      break;
-  }
-  v8::Isolate* isolate = context()->isolate();
-  v8::HandleScope handle_scope(isolate);
-  v8::Context::Scope context_scope(context()->v8_context());
-  v8::Local<v8::Value> argv[] = {
-      v8::String::NewFromUtf8(isolate, stage_string)};
-  context()->module_system()->CallModuleMethodSafe(
-      "webstore", "onInstallStageChanged", arraysize(argv), argv);
-}
-
-void WebstoreBindings::OnInlineInstallDownloadProgress(int percent_downloaded) {
-  v8::Isolate* isolate = context()->isolate();
-  v8::HandleScope handle_scope(isolate);
-  v8::Context::Scope context_scope(context()->v8_context());
-  v8::Local<v8::Value> argv[] = {
-      v8::Number::New(isolate, percent_downloaded / 100.0)};
-  context()->module_system()->CallModuleMethodSafe(
-      "webstore", "onDownloadProgress", arraysize(argv), argv);
-}
-
 }  // namespace extensions