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..972737ec6deee2e0f76d67d81b1ab02b4fa6e386 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,100 @@ 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,

[Devlin, 2017/04/27 23:16:14]

I don't think this should need to be an ObjectBackedNativeHandler.

[catmullings, 2017/04/28 03:07:30]

Done.

[catmullings, 2017/04/28 03:07:30]

Just for documentation: Making WebstoreBindingsHelper an
ObjectBackedNativeHandler was causing tests to fail. This is because nothing was
calling NativeHandler::Invalidate() after a WebstoreBindingsHelper was
destroyed. The reason why WebstoreBindings, also an ObjectBackedNativeHandler,
did not have this error is because the ModuleSystem that it's hooked up to
invalidates it
(https://cs.chromium.org/chromium/src/extensions/renderer/module_system.cc?l=202).

+      public mojom::InlineInstallProgressListener {
+ public:
+  WebstoreBindingsHelper(ScriptContext* context);
+  ~WebstoreBindingsHelper() override;
+  mojom::InlineInstallProgressListenerPtr Create();
+
+ private:
+  void InlineInstallStageChanged(api::webstore::InstallStage stage) override;
+  void InlineInstallDownloadProgress(int percent_downloaded) override;
+};
+
+WebstoreBindings::WebstoreBindingsHelper::WebstoreBindingsHelper(
+    ScriptContext* context)
+    : ObjectBackedNativeHandler(context) {}
+
+mojom::InlineInstallProgressListenerPtr
+WebstoreBindings::WebstoreBindingsHelper::Create() {

[Devlin, 2017/04/27 23:16:14]

This create method is... odd.  We call it on an instance of the class.  Instead,
we should probably just construct the class directly (as this method currently
does), or we should make the constructor private and make the Create method
static and take a ScriptContext.

[catmullings, 2017/04/28 03:07:30]

Done.

+  mojom::InlineInstallProgressListenerPtr install_progress_listener;
+  mojo::MakeStrongBinding(base::MakeUnique<WebstoreBindingsHelper>(context()),
+                          mojo::MakeRequest(&install_progress_listener));
+  return install_progress_listener;
+}
+
+WebstoreBindings::WebstoreBindingsHelper::~WebstoreBindingsHelper() = default;
+
+void WebstoreBindings::WebstoreBindingsHelper::InlineInstallStageChanged(

[Devlin, 2017/04/27 23:16:14]

nit: Can we instead leave these methods in WebstoreBindings, and just call the
method from here?  (This would involve taking a pointer [or weak ptr, depending
on lifetime concerns] to the parent WebstoreBindings, but I think that should be
okay).

e.g.

void WebstoreBindings::WebstoreBindingsHelper::InlineInstallStateChanged(
    api::webstore::InstallStage stage) {
  parent_->InlineInstallStateChanged(stage);
}

This has two main advantages:
- It centralizes the logic, and makes this helper class more clearly a "helper"
(rather than the source of implementation).
- It makes reviewing easier because the diff can be smaller. :D

[catmullings, 2017/04/28 03:07:30]

Done.

+    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);
+}
+
 WebstoreBindings::WebstoreBindings(ScriptContext* context)
-    : ObjectBackedNativeHandler(context) {
+    : ObjectBackedNativeHandler(context),
+      webstore_bindings_helper_(
+          base::MakeUnique<WebstoreBindingsHelper>(context)) {
   RouteFunction("Install", "webstore",
                 base::Bind(&WebstoreBindings::Install, base::Unretained(this)));
+  context->GetRenderFrame()->GetRemoteAssociatedInterfaces()->GetInterface(
+      &inline_install_);
+}
+
+void WebstoreBindings::InlineInstallResponse(int install_id,
+                                             bool success,
+                                             const std::string& error,
+                                             webstore_install::Result result) {
+  v8::Isolate* isolate = context()->isolate();
+  v8::HandleScope handle_scope(isolate);
+  v8::Context::Scope context_scope(context()->v8_context());
+  v8::Local<v8::Value> argv[] = {
+      v8::Integer::New(isolate, install_id), v8::Boolean::New(isolate, success),
+      v8::String::NewFromUtf8(isolate, error.c_str()),
+      v8::String::NewFromUtf8(
+          isolate,
+          api::webstore::kInstallResultCodes[static_cast<int>(result)])};
+  context()->module_system()->CallModuleMethodSafe(
+      "webstore", "onInstallResponse", arraysize(argv), argv);
 }
 
+WebstoreBindings::~WebstoreBindings() {}
+
 void WebstoreBindings::Install(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
   content::RenderFrame* render_frame = context()->GetRenderFrame();
@@ -91,9 +177,14 @@ 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));
+  mojom::InlineInstallProgressListenerPtr install_progress_listener =
+      webstore_bindings_helper_->Create();
+
+  inline_install_->DoInlineInstall(
+      install_id, webstore_item_id, listener_mask,
+      std::move(install_progress_listener),

[Devlin, 2017/04/27 23:16:14]

Does this need to be a strong (i.e., owning) pointer?  Or can ownership be kept
within the WebstoreBindings parent, allowing reuse between calls?

[catmullings, 2017/04/28 03:07:30]

If std::move() isn't used, then there's a compiler error:

call to deleted constructor of
'extensions::mojom::InlineInstallProgressListenerPtr'

+      base::Bind(&WebstoreBindings::InlineInstallResponse,
+                 base::Unretained(this)));
 
   args.GetReturnValue().Set(static_cast<int32_t>(install_id));
 }
@@ -187,66 +278,7 @@ 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();
-  v8::HandleScope handle_scope(isolate);
-  v8::Context::Scope context_scope(context()->v8_context());
-  v8::Local<v8::Value> argv[] = {
-    v8::Integer::New(isolate, install_id),
-    v8::Boolean::New(isolate, success),
-    v8::String::NewFromUtf8(isolate, error.c_str()),
-    v8::String::NewFromUtf8(
-        isolate, api::webstore::kInstallResultCodes[static_cast<int>(result)])
-  };
-  context()->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