Convert Web Store Inline Install IPCs to mojo

chrome/renderer/extensions/webstore_bindings.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/renderer/extensions/webstore_bindings.h"
 
 #include <stdint.h>
 
 #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"
 #include "third_party/WebKit/public/web/WebNode.h"
 #include "third_party/WebKit/public/web/WebUserGestureIndicator.h"
 #include "url/gurl.h"
 #include "v8/include/v8.h"
 
 using blink::WebDocument;
 using blink::WebElement;
@@ skipping 10 matching lines @@
     "Chrome Web Store installations can only be started by the top frame.";
 const char kNotUserGestureError[] =
     "Chrome Web Store installations can only be initated by a user gesture.";
 const char kNoWebstoreItemLinkFoundError[] =
     "No Chrome Web Store item link found.";
 const char kInvalidWebstoreItemUrlError[] =
     "Invalid Chrome Web Store item URL.";
 
 // 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();
   if (!render_frame)
     return;
 
   // The first two arguments indicate whether or not there are install stage
   // or download progress listeners.
   int listener_mask = 0;
   CHECK(args[0]->IsBoolean());
@@ skipping 15 matching lines @@
 
   if (!GetWebstoreItemIdFromFrame(
       frame, preferred_store_link_url, &webstore_item_id, &error)) {
     args.GetIsolate()->ThrowException(
         v8::String::NewFromUtf8(args.GetIsolate(), error.c_str()));
     return;
   }
 
   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));
 }
 
 // static
 bool WebstoreBindings::GetWebstoreItemIdFromFrame(
     blink::WebLocalFrame* frame,
     const std::string& preferred_store_link_url,
     std::string* webstore_item_id,
     std::string* error) {
@@ skipping 73 matching lines @@
 
     *webstore_item_id = candidate_webstore_item_id;
     return true;
   }
 
   *error = kNoWebstoreItemLinkFoundError;
   return false;
 }
 
 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