[Extensions Bindings] Move custom handling to APIBindingHooksDelegate

Move custom request handling from registering a method in the
APIBindingHooks to a delegate call to match CreateCustomEvent.
Add a APIBindingHooksTestDelegate for use in unittests to avoid having
a bunch of one-off subclasses.

BUG=653596

Review-Url: https://codereview.chromium.org/2831453002
Cr-Commit-Position: refs/heads/master@{#465735}

extensions/renderer/BUILD.gn

@@ -297,6 +297,8 @@ source_set("unit_tests") {
   sources = [
     "activity_log_converter_strategy_unittest.cc",
     "api/mojo_private/mojo_private_unittest.cc",
+    "api_binding_hooks_test_delegate.cc",
+    "api_binding_hooks_test_delegate.h",
     "api_binding_js_util_unittest.cc",
     "api_binding_test.cc",
     "api_binding_test.h",

extensions/renderer/api_binding_hooks.cc

@@ -207,12 +207,6 @@ APIBindingHooks::APIBindingHooks(const std::string& api_name,
     : api_name_(api_name), run_js_(run_js) {}
 APIBindingHooks::~APIBindingHooks() {}
 
-void APIBindingHooks::RegisterHandleRequest(const std::string& method_name,
-                                            const HandleRequestHook& hook) {
-  DCHECK(!hooks_used_) << "Hooks must be registered before the first use!";
-  request_hooks_[method_name] = hook;
-}
-
 APIBindingHooks::RequestResult APIBindingHooks::RunHooks(
     const std::string& method_name,
     v8::Local<v8::Context> context,
@@ -220,15 +214,11 @@ APIBindingHooks::RequestResult APIBindingHooks::RunHooks(
     std::vector<v8::Local<v8::Value>>* arguments,
     const APITypeReferenceMap& type_refs) {
   // Easy case: a native custom hook.
-  auto request_hooks_iter = request_hooks_.find(method_name);
-  if (request_hooks_iter != request_hooks_.end()) {
-    RequestResult result =
-        request_hooks_iter->second.Run(
-            signature, context, arguments, type_refs);
-    // Right now, it doesn't make sense to register a request handler that
-    // doesn't handle the request.
-    DCHECK_NE(RequestResult::NOT_HANDLED, result.code);
-    return result;
+  if (delegate_) {
+    RequestResult result = delegate_->HandleRequest(
+        method_name, signature, context, arguments, type_refs);
+    if (result.code != RequestResult::NOT_HANDLED)
+      return result;
   }
 
   // Harder case: looking up a custom hook registered on the context (since

extensions/renderer/api_binding_hooks.h

@@ -5,22 +5,16 @@
 #ifndef EXTENSIONS_RENDERER_API_BINDING_HOOKS_H_
 #define EXTENSIONS_RENDERER_API_BINDING_HOOKS_H_
 
-#include <map>
 #include <memory>
 #include <string>
 
-#include "base/callback.h"
 #include "base/macros.h"
 #include "extensions/renderer/api_binding_types.h"
-#include "extensions/renderer/argument_spec.h"
 #include "v8/include/v8.h"
 
-namespace gin {
-class Arguments;
-}
-
 namespace extensions {
 class APIBindingHooksDelegate;
+class APITypeReferenceMap;
 class APISignature;
 
 // A class to register custom hooks for given API calls that need different
@@ -51,29 +45,10 @@ class APIBindingHooks {
     v8::Local<v8::Value> return_value;  // Only valid if code == HANDLED.
   };
 
-  // The callback to handle an API method. We pass in the expected signature
-  // (so the caller can verify arguments, optionally after modifying/"massaging"
-  // them) and the passed arguments. The handler is responsible for returning,
-  // which depending on the API could mean either returning synchronously
-  // through gin::Arguments::Return or asynchronously through a passed callback.
-  // TODO(devlin): As we continue expanding the hooks interface, we should allow
-  // handlers to register a request so that they don't have to maintain a
-  // reference to the callback themselves.
-  using HandleRequestHook =
-      base::Callback<RequestResult(const APISignature*,
-                                   v8::Local<v8::Context> context,
-                                   std::vector<v8::Local<v8::Value>>*,
-                                   const APITypeReferenceMap&)>;
-
   APIBindingHooks(const std::string& api_name,
                   const binding::RunJSFunctionSync& run_js);
   ~APIBindingHooks();
 
-  // Register a custom binding to handle requests.
-  // TODO(devlin): Remove this in favor of a method on APIBindingHooksDelegate.
-  void RegisterHandleRequest(const std::string& method_name,
-                             const HandleRequestHook& hook);
-
   // Looks for any custom hooks associated with the given request, and, if any
   // are found, runs them. Returns the result of running the hooks, if any.
   RequestResult RunHooks(const std::string& method_name,
@@ -104,12 +79,6 @@ class APIBindingHooks {
                        v8::Local<v8::Context> context,
                        std::vector<v8::Local<v8::Value>>* arguments);
 
-  // Whether we've tried to use any hooks associated with this object.
-  bool hooks_used_ = false;
-
-  // All registered request handlers.
-  std::map<std::string, HandleRequestHook> request_hooks_;
-
   // The name of the associated API.
   std::string api_name_;
 

extensions/renderer/api_binding_hooks_delegate.cc

@@ -16,4 +16,14 @@ bool APIBindingHooksDelegate::CreateCustomEvent(
   return false;
 }
 
+APIBindingHooks::RequestResult APIBindingHooksDelegate::HandleRequest(
+    const std::string& method_name,
+    const APISignature* signature,
+    v8::Local<v8::Context> context,
+    std::vector<v8::Local<v8::Value>>* arguments,
+    const APITypeReferenceMap& refs) {
+  return APIBindingHooks::RequestResult(
+      APIBindingHooks::RequestResult::NOT_HANDLED);
+}
+
 }  // namespace extensions

extensions/renderer/api_binding_hooks_delegate.h

@@ -5,6 +5,7 @@
 #ifndef EXTENSIONS_RENDERER_API_BINDING_HOOKS_DELEGATE_H_
 #define EXTENSIONS_RENDERER_API_BINDING_HOOKS_DELEGATE_H_
 
+#include "extensions/renderer/api_binding_hooks.h"
 #include "extensions/renderer/api_binding_types.h"
 #include "v8/include/v8.h"
 
@@ -23,7 +24,13 @@ class APIBindingHooksDelegate {
                                  const std::string& event_name,
                                  v8::Local<v8::Value>* event_out);
 
-  // TODO(devlin): Add a virtual HandleRequest() method.
+  // Allows custom implementations to handle a given request.
+  virtual APIBindingHooks::RequestResult HandleRequest(
+      const std::string& method_name,
+      const APISignature* signature,
+      v8::Local<v8::Context> context,
+      std::vector<v8::Local<v8::Value>>* arguments,
+      const APITypeReferenceMap& refs);
 };
 
 }  // namespace extensions

extensions/renderer/api_binding_hooks_test_delegate.cc

+// Copyright 2017 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 "extensions/renderer/api_binding_hooks_test_delegate.h"
+
+namespace extensions {
+
+APIBindingHooksTestDelegate::APIBindingHooksTestDelegate() {}
+APIBindingHooksTestDelegate::~APIBindingHooksTestDelegate() {}
+
+bool APIBindingHooksTestDelegate::CreateCustomEvent(
+    v8::Local<v8::Context> context,
+    const binding::RunJSFunctionSync& run_js_sync,
+    const std::string& event_name,
+    v8::Local<v8::Value>* event_out) {
+  if (!custom_event_.is_null()) {
+    *event_out = custom_event_.Run(context, run_js_sync, event_name);
+    return true;
+  }
+  return false;
+}
+
+void APIBindingHooksTestDelegate::AddHandler(base::StringPiece name,
+                                             const RequestHandler& handler) {
+  request_handlers_[name.as_string()] = handler;
+}
+
+void APIBindingHooksTestDelegate::SetCustomEvent(
+    const CustomEventFactory& custom_event) {
+  custom_event_ = custom_event;
+}
+
+APIBindingHooks::RequestResult APIBindingHooksTestDelegate::HandleRequest(
+    const std::string& method_name,
+    const APISignature* signature,
+    v8::Local<v8::Context> context,
+    std::vector<v8::Local<v8::Value>>* arguments,
+    const APITypeReferenceMap& refs) {
+  auto iter = request_handlers_.find(method_name);
+  if (iter == request_handlers_.end()) {
+    return APIBindingHooks::RequestResult(
+        APIBindingHooks::RequestResult::NOT_HANDLED);
+  }
+  return iter->second.Run(signature, context, arguments, refs);
+}
+
+}  // namespace extensions

extensions/renderer/api_binding_hooks_test_delegate.h

+// Copyright 2017 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.
+
+#ifndef EXTENSIONS_RENDERER_API_BINDING_HOOKS_TEST_DELEGATE_H_
+#define EXTENSIONS_RENDERER_API_BINDING_HOOKS_TEST_DELEGATE_H_
+
+#include <map>
+#include <string>
+
+#include "base/callback.h"
+#include "base/macros.h"
+#include "base/strings/string_piece.h"
+#include "extensions/renderer/api_binding_hooks_delegate.h"
+#include "extensions/renderer/api_binding_types.h"
+#include "v8/include/v8.h"
+
+namespace extensions {
+
+// A test class to simply adding custom events or handlers for API hooks.
+class APIBindingHooksTestDelegate : public APIBindingHooksDelegate {
+ public:
+  APIBindingHooksTestDelegate();
+  ~APIBindingHooksTestDelegate() override;
+
+  using CustomEventFactory = base::Callback<v8::Local<v8::Value>(
+      v8::Local<v8::Context>,
+      const binding::RunJSFunctionSync& run_js,
+      const std::string& event_name)>;
+
+  using RequestHandler = base::Callback<APIBindingHooks::RequestResult(
+      const APISignature*,
+      v8::Local<v8::Context> context,
+      std::vector<v8::Local<v8::Value>>*,
+      const APITypeReferenceMap&)>;
+
+  // Adds a custom |handler| for the method with the given |name|.
+  void AddHandler(base::StringPiece name, const RequestHandler& handler);
+
+  // Creates events with the given factory.
+  void SetCustomEvent(const CustomEventFactory& custom_event);
+
+  // APIBindingHooksDelegate:
+  bool CreateCustomEvent(v8::Local<v8::Context> context,
+                         const binding::RunJSFunctionSync& run_js_sync,
+                         const std::string& event_name,
+                         v8::Local<v8::Value>* event_out) override;
+  APIBindingHooks::RequestResult HandleRequest(
+      const std::string& method_name,
+      const APISignature* signature,
+      v8::Local<v8::Context> context,
+      std::vector<v8::Local<v8::Value>>* arguments,
+      const APITypeReferenceMap& refs) override;
+
+ private:
+  std::map<std::string, RequestHandler> request_handlers_;
+  CustomEventFactory custom_event_;
+
+  DISALLOW_COPY_AND_ASSIGN(APIBindingHooksTestDelegate);
+};
+
+}  // namespace extensions
+
+#endif  // EXTENSIONS_RENDERER_API_BINDING_HOOKS_TEST_DELEGATE_H_

extensions/renderer/api_binding_unittest.cc

@@ -9,6 +9,7 @@
 #include "base/values.h"
 #include "extensions/renderer/api_binding.h"
 #include "extensions/renderer/api_binding_hooks.h"
+#include "extensions/renderer/api_binding_hooks_test_delegate.h"
 #include "extensions/renderer/api_binding_test.h"
 #include "extensions/renderer/api_binding_test_util.h"
 #include "extensions/renderer/api_event_handler.h"
@@ -178,6 +179,12 @@ class APIBindingUnittest : public APIBindingTest {
     ASSERT_TRUE(binding_hooks_);
   }
 
+  void SetHooksDelegate(
+      std::unique_ptr<APIBindingHooksDelegate> hooks_delegate) {
+    binding_hooks_delegate_ = std::move(hooks_delegate);
+    ASSERT_TRUE(binding_hooks_delegate_);
+  }
+
   void SetCreateCustomType(const APIBinding::CreateCustomType& callback) {
     create_custom_type_ = callback;
   }
@@ -187,6 +194,8 @@ class APIBindingUnittest : public APIBindingTest {
       binding_hooks_ = base::MakeUnique<APIBindingHooks>(
           kBindingName, binding::RunJSFunctionSync());
     }
+    if (binding_hooks_delegate_)
+      binding_hooks_->SetDelegate(std::move(binding_hooks_delegate_));
     event_handler_ = base::MakeUnique<APIEventHandler>(
         base::Bind(&RunFunctionOnGlobalAndIgnoreResult),
         base::Bind(&OnEventListenersChanged));
@@ -253,6 +262,7 @@ class APIBindingUnittest : public APIBindingTest {
   std::unique_ptr<base::ListValue> binding_types_;
   std::unique_ptr<base::DictionaryValue> binding_properties_;
   std::unique_ptr<APIBindingHooks> binding_hooks_;
+  std::unique_ptr<APIBindingHooksDelegate> binding_hooks_delegate_;
   APIBinding::CreateCustomType create_custom_type_;
 
   DISALLOW_COPY_AND_ASSIGN(APIBindingUnittest);
@@ -691,8 +701,7 @@ TEST_F(APIBindingUnittest, TestCustomHooks) {
   SetFunctions(kFunctions);
 
   // Register a hook for the test.oneString method.
-  auto hooks = base::MakeUnique<APIBindingHooks>(
-      kBindingName, base::Bind(&RunFunctionOnGlobalAndReturnHandle));
+  auto hooks = base::MakeUnique<APIBindingHooksTestDelegate>();
   bool did_call = false;
   auto hook = [](bool* did_call, const APISignature* signature,
                  v8::Local<v8::Context> context,
@@ -708,8 +717,8 @@ TEST_F(APIBindingUnittest, TestCustomHooks) {
     EXPECT_EQ("foo", gin::V8ToString(arguments->at(0)));
     return result;
   };
-  hooks->RegisterHandleRequest("test.oneString", base::Bind(hook, &did_call));
-  SetHooks(std::move(hooks));
+  hooks->AddHandler("test.oneString", base::Bind(hook, &did_call));
+  SetHooksDelegate(std::move(hooks));
 
   InitializeBinding();
 
@@ -990,8 +999,7 @@ TEST_F(APIBindingUnittest,
   v8::Local<v8::Context> context = MainContext();
 
   // Register a hook for the test.oneString method.
-  auto hooks = base::MakeUnique<APIBindingHooks>(
-      kBindingName, base::Bind(&RunFunctionOnGlobalAndReturnHandle));
+  auto hooks = base::MakeUnique<APIBindingHooksTestDelegate>();
   bool did_call = false;
   auto hook = [](bool* did_call, const APISignature* signature,
                  v8::Local<v8::Context> context,
@@ -1015,9 +1023,9 @@ TEST_F(APIBindingUnittest,
         gin::StringToV8(context->GetIsolate(), arg_value + " pong");
     return result;
   };
-  hooks->RegisterHandleRequest("test.oneString", base::Bind(hook, &did_call));
+  hooks->AddHandler("test.oneString", base::Bind(hook, &did_call));
 
-  SetHooks(std::move(hooks));
+  SetHooksDelegate(std::move(hooks));
   SetFunctions(kFunctions);
   InitializeBinding();
 

extensions/renderer/api_bindings_system_unittest.cc

@@ -14,7 +14,7 @@
 #include "extensions/common/extension_api.h"
 #include "extensions/renderer/api_binding.h"
 #include "extensions/renderer/api_binding_hooks.h"
-#include "extensions/renderer/api_binding_hooks_delegate.h"
+#include "extensions/renderer/api_binding_hooks_test_delegate.h"
 #include "extensions/renderer/api_binding_test_util.h"
 #include "extensions/renderer/api_binding_types.h"
 #include "extensions/renderer/api_bindings_system_unittest.h"
@@ -94,50 +94,6 @@ bool AllowAllAPIs(const std::string& name) {
   return true;
 }
 
-class TestHooks : public APIBindingHooksDelegate {
- public:
-  TestHooks() {}
-  ~TestHooks() override {}
-
-  using CustomEventFactory = base::Callback<v8::Local<v8::Value>(
-      v8::Local<v8::Context>,
-      const binding::RunJSFunctionSync& run_js,
-      const std::string& event_name)>;
-
-  bool CreateCustomEvent(v8::Local<v8::Context> context,
-                         const binding::RunJSFunctionSync& run_js_sync,
-                         const std::string& event_name,
-                         v8::Local<v8::Value>* event_out) override {
-    if (!custom_event_.is_null()) {
-      *event_out = custom_event_.Run(context, run_js_sync, event_name);
-      return true;
-    }
-    return false;
-  }
-
-  void RegisterHooks(APIBindingHooks* hooks) {
-    for (const auto& request_handler : request_handlers_) {
-      hooks->RegisterHandleRequest(request_handler.first,
-                                   request_handler.second);
-    }
-  }
-
-  void AddHandler(base::StringPiece name,
-                  const APIBindingHooks::HandleRequestHook& hook) {
-    request_handlers_[name.as_string()] = hook;
-  }
-
-  void SetCustomEvent(const CustomEventFactory& custom_event) {
-    custom_event_ = custom_event;
-  }
-
- private:
-  std::map<std::string, APIBindingHooks::HandleRequestHook> request_handlers_;
-  CustomEventFactory custom_event_;
-
-  DISALLOW_COPY_AND_ASSIGN(TestHooks);
-};
-
 }  // namespace
 
 APIBindingsSystemTest::APIBindingsSystemTest() {}
@@ -369,12 +325,11 @@ TEST_F(APIBindingsSystemTest, TestCustomHooks) {
     return result;
   };
 
-  auto test_hooks = base::MakeUnique<TestHooks>();
+  auto test_hooks = base::MakeUnique<APIBindingHooksTestDelegate>();
   test_hooks->AddHandler("alpha.functionWithCallback",
                          base::Bind(hook, &did_call));
   APIBindingHooks* binding_hooks =
       bindings_system()->GetHooksForAPI(kAlphaAPIName);
-  test_hooks->RegisterHooks(binding_hooks);
   binding_hooks->SetDelegate(std::move(test_hooks));
 
   v8::Local<v8::Object> alpha_api = bindings_system()->CreateAPIInstance(
@@ -486,7 +441,7 @@ TEST_F(APIBindingsSystemTest, TestCustomEvent) {
     return ret.As<v8::Value>();
   };
 
-  auto test_hooks = base::MakeUnique<TestHooks>();
+  auto test_hooks = base::MakeUnique<APIBindingHooksTestDelegate>();
   test_hooks->SetCustomEvent(base::Bind(create_custom_event));
   APIBindingHooks* binding_hooks =
       bindings_system()->GetHooksForAPI(kAlphaAPIName);