[Extensions Binding] Allow for registering custom hooks

extensions/renderer/api_bindings_system_unittest.cc

Index: extensions/renderer/api_bindings_system_unittest.cc
diff --git a/extensions/renderer/api_bindings_system_unittest.cc b/extensions/renderer/api_bindings_system_unittest.cc
index b1fcfce546d19661ea463c6d627d0695bbffdc66..f59f4261f48319fade8fb210a422569e7229f3ba 100644
--- a/extensions/renderer/api_bindings_system_unittest.cc
+++ b/extensions/renderer/api_bindings_system_unittest.cc
@@ -12,8 +12,11 @@
 #include "base/values.h"
 #include "extensions/common/extension_api.h"
 #include "extensions/renderer/api_binding.h"
+#include "extensions/renderer/api_binding_hooks.h"
 #include "extensions/renderer/api_binding_test.h"
 #include "extensions/renderer/api_binding_test_util.h"
+#include "extensions/renderer/api_binding_types.h"
+#include "gin/arguments.h"
 #include "gin/converter.h"
 #include "gin/try_catch.h"
 
@@ -289,6 +292,54 @@ TEST_F(APIBindingsSystemTest, TestInitializationAndCallbacks) {
   }
 }
 
+// Tests adding a custom hook to an API.
+TEST_F(APIBindingsSystemTest, TestCustomHooks) {
+  v8::HandleScope handle_scope(isolate());
+  v8::Local<v8::Context> context = ContextLocal();
+
+  bool did_call = false;
+  auto hook = [](bool* did_call, const binding::APISignature* signature,
+                 gin::Arguments* arguments) {
+    *did_call = true;
+    std::string argument;
+    EXPECT_EQ(2, arguments->Length());
+    EXPECT_TRUE(arguments->GetNext(&argument));
+    EXPECT_EQ("foo", argument);
+    v8::Local<v8::Function> function;
+    ASSERT_TRUE(arguments->GetNext(&function));
+    v8::Local<v8::String> response =
+        gin::StringToV8(arguments->isolate(), "bar");
+    v8::Local<v8::Value> response_args[] = {response};
+    RunFunctionOnGlobal(function, arguments->isolate()->GetCurrentContext(),
+                        1, response_args);
+  };
+
+  APIBindingHooks* hooks = bindings_system()->GetHooksForAPI(kAlphaAPIName);

[lazyboy, 2016/12/13 01:52:32]

I'm finding this pattern to add a hook harder to read: you first call
GetHooksForAPI and then add hook(s) to the return value.

Instead if APIBindingSystem exposed some RegisterHook(api_name, method_name,
hook) and then that function called APIBindingsHooks::RegisterHandleRequest(),
it would have been a bit better imo.

Then there's also a possibility of code like:

hooks = bindings_system()->GetHooksForAPI(...)
binding_system()->CreateAPIInstance(...)
hooks->RegisterHandleRequest("alpha.functionWithCallback", ...)

You can't guarantee alpha.functionWithCallback will be dispatched properly (of
course that's a user error).  Line #78 won't complain in api_bindings_system.cc.
Is that an issue?

[Devlin, 2016/12/13 18:41:30]

Hmm... depending on how complex the "add hooks" methods are, I'm worried about
the implication of having APIBindingSystem need to expose all the bits and bobs.
 Additionally, it would mean that registering multiple hooks for the same API is
less efficient, since you'd have to look it up each time.

For the time being, I'd like to keep it like this so that we keep a more
flexible interface, but I've added a TODO to re-evaluate in the future.  Does
that SGTY, or would you prefer to change it now and possibly change back later?

[lazyboy, 2016/12/13 18:58:25]

Keeping this as is + TODO SG.
Calling APIBindingHooks::RegisterHandleRequest *after* creating API instance
still seems like a recipe for race condition to me though. Maybe you can add a
DCHECK in RegisterHandleRequest that makes sure it's not called after any
APIBindingHooks::GetHandleRequest() is called? Or do we imagine that this would
also be valid in some lazy initialization scenario?

[Devlin, 2016/12/13 19:05:39]

Yeah, that's reasonable.  Added a DCHECK.

+  ASSERT_TRUE(hooks);
+  hooks->RegisterHandleRequest(
+      "alpha.functionWithCallback",
+      base::Bind(hook, &did_call));
+
+  v8::Local<v8::Object> alpha_api = bindings_system()->CreateAPIInstance(
+      kAlphaAPIName, context, isolate(), base::Bind(&AllowAllAPIs));
+  ASSERT_FALSE(alpha_api.IsEmpty());
+
+  {
+    // Test a simple call -> response.
+    const char kTestCall[] =
+        "obj.functionWithCallback('foo', function() {\n"
+        "  this.callbackArguments = Array.from(arguments);\n"
+        "});";
+    CallFunctionOnObject(context, alpha_api, kTestCall);
+    EXPECT_TRUE(did_call);
+
+    std::unique_ptr<base::Value> result = GetBaseValuePropertyFromObject(
+        context->Global(), context, "callbackArguments");
+    ASSERT_TRUE(result);
+    EXPECT_EQ("[\"bar\"]", ValueToString(*result));
+  }
+}
+
 // An implementation using real API schemas.
 class APIBindingsSystemTestWithRealAPI : public APIBindingsSystemTestBase {
  protected: