[Extensions Binding] Allow for registering custom hooks

extensions/renderer/api_bindings_system_unittest.cc

 // Copyright 2016 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_bindings_system.h"
 
 #include "base/bind.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
 #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"
 
 namespace extensions {
 
 namespace {
 
 // Fake API for testing.
 const char kAlphaAPIName[] = "alpha";
 const char kAlphaAPISpec[] =
@@ skipping 255 matching lines @@
   {
     // Test a call -> response on the second API.
     const char kTestCall[] = "obj.simpleFunc(2)";
     CallFunctionOnObject(context, beta_api, kTestCall);
     ValidateLastRequest("beta.simpleFunc", "[2]");
     EXPECT_EQ(-1, last_request()->request_id);
     reset_last_request();
   }
 }
 
+// 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:
   APIBindingsSystemTestWithRealAPI() {}
 
   // Executes the given |script_source| in |context|, expecting no exceptions.
   void ExecuteScript(v8::Local<v8::Context> context,
                      const std::string& script_source);
 
   // Executes the given |script_source| in |context| and compares a caught
@@ skipping 132 matching lines @@
                                           *ListValueFromString("['active']"));
 
     std::unique_ptr<base::Value> result =
         GetBaseValuePropertyFromObject(context->Global(), context, "idleState");
     ASSERT_TRUE(result);
     EXPECT_EQ("\"active\"", ValueToString(*result));
   }
 }
 
 }  // namespace extensions