[Extension Bindings] Add JS custom hook support

extensions/renderer/api_binding_hooks.cc

Index: extensions/renderer/api_binding_hooks.cc
diff --git a/extensions/renderer/api_binding_hooks.cc b/extensions/renderer/api_binding_hooks.cc
index 69ade18a96fa5f9650b78f8068c12d6282a86990..f4ed635f3612d52b626dbc72c1ca2f6963ba3017 100644
--- a/extensions/renderer/api_binding_hooks.cc
+++ b/extensions/renderer/api_binding_hooks.cc
@@ -4,9 +4,140 @@
 
 #include "extensions/renderer/api_binding_hooks.h"
 
+#include "base/memory/ptr_util.h"
+#include "base/strings/stringprintf.h"
+#include "base/supports_user_data.h"
+#include "gin/arguments.h"
+#include "gin/handle.h"
+#include "gin/object_template_builder.h"
+#include "gin/per_context_data.h"
+#include "gin/wrappable.h"
+
 namespace extensions {
 
-APIBindingHooks::APIBindingHooks() {}
+namespace {
+
+// An interface to allow for registration of custom hooks from JavaScript.
+// Contains registered hooks for a single API.
+class JSHookInterface final : public gin::Wrappable<JSHookInterface> {
+ public:
+  using JSHooks = std::map<std::string, v8::Global<v8::Function>>;
+
+  explicit JSHookInterface(const std::string& api_name)
+      : api_name_(api_name) {}
+  ~JSHookInterface() override {}
+
+  static gin::WrapperInfo kWrapperInfo;
+
+  // gin::Wrappable:
+  gin::ObjectTemplateBuilder GetObjectTemplateBuilder(
+      v8::Isolate* isolate) override {
+    return Wrappable<JSHookInterface>::GetObjectTemplateBuilder(isolate)
+        .SetMethod("setHandleRequest", &JSHookInterface::SetHandleRequest);
+  }
+
+  JSHooks* js_hooks() { return &js_hooks_; }
+
+ private:
+  // Adds a custom hook.
+  void SetHandleRequest(gin::Arguments* arguments) {

[jbroman, 2016/12/13 22:12:40]

There's nothing unusual going on here, so you don't need to use gin::Arguments
here. (This is one nice thing about gin::Wrappable.)

void SetHandleRequest(v8::Isolate* isolate, const std::string& method_name,
v8::Local<v8::Function> handler) {
}

It will also handle throwing an error if this signature doesn't match.

[Devlin, 2016/12/14 02:02:13]

Nifty, done.

+    std::string method_name;
+    if (!arguments->GetNext(&method_name))
+      return;
+    v8::Local<v8::Function> handler;
+    if (!arguments->GetNext(&handler))
+      return;
+
+    std::string qualified_method_name =
+        base::StringPrintf("%s.%s", api_name_.c_str(), method_name.c_str());
+    v8::Global<v8::Function>& entry = js_hooks_[qualified_method_name];
+    if (!entry.IsEmpty()) {
+      NOTREACHED() << "Hooks can only be set once.";
+      return;
+    }
+    js_hooks_[qualified_method_name] =

[jbroman, 2016/12/13 22:12:41]

You already have a reference to this object a few minutes up. Wouldn't this
work:

entry.Reset(isolate, handler);

[Devlin, 2016/12/14 02:02:14]

I was definitely going to do that, and apparently got distracted halfway through
my thought.  Thanks for catching this.

+        v8::Global<v8::Function>(arguments->isolate(), handler);
+  }
+
+  std::string api_name_;
+
+  JSHooks js_hooks_;
+
+  DISALLOW_COPY_AND_ASSIGN(JSHookInterface);
+};
+
+const char kExtensionAPIHooksPerContextKey[] = "extension_api_hooks";
+
+struct APIHooksPerContextData : public base::SupportsUserData::Data {
+  APIHooksPerContextData(v8::Isolate* isolate) : isolate(isolate) {}
+  ~APIHooksPerContextData() override {
+    v8::HandleScope scope(isolate);
+    for (const auto& pair : hook_interfaces) {
+      // We explicitly clear the hook data map here to remove all references to
+      // v8 objects in order to avoid cycles.
+      JSHookInterface* hooks = nullptr;
+      gin::Converter<JSHookInterface*>::FromV8(
+          isolate, pair.second.Get(isolate), &hooks);
+      CHECK(hooks);
+      hooks->js_hooks()->clear();
+    }
+  }
+
+  v8::Isolate* isolate = nullptr;
+
+  std::map<std::string, v8::Global<v8::Object>> hook_interfaces;
+};
+
+gin::WrapperInfo JSHookInterface::kWrapperInfo =
+    {gin::kEmbedderNativeGin};
+
+// Creates and returns JS object for the hook interface to allow for
+// registering custom hooks from JS.
+v8::Local<v8::Object> CreateJSHookInterface(const std::string& api_name,
+                                            v8::Local<v8::Context> context) {
+  gin::PerContextData* per_context_data = gin::PerContextData::From(context);
+  DCHECK(per_context_data);
+  APIHooksPerContextData* data = static_cast<APIHooksPerContextData*>(
+      per_context_data->GetUserData(kExtensionAPIHooksPerContextKey));
+  if (!data) {
+    auto api_data =
+        base::MakeUnique<APIHooksPerContextData>(context->GetIsolate());
+    data = api_data.get();
+    per_context_data->SetUserData(kExtensionAPIHooksPerContextKey,
+                                  api_data.release());
+  }
+
+  DCHECK(data->hook_interfaces.find(api_name) == data->hook_interfaces.end());
+
+  gin::Handle<JSHookInterface> hooks =
+      gin::CreateHandle(context->GetIsolate(), new JSHookInterface(api_name));
+  CHECK(!hooks.IsEmpty());
+  v8::Local<v8::Value> hooks_value = hooks.ToV8();
+  CHECK(hooks_value->IsObject());

[jbroman, 2016/12/13 22:12:40]

It'd be pretty bad for this to not be a v8::Object; the implementation of
gin::CreateHandle is pretty simple and already assumes v8::Object internally. Do
we need this check? (V8 already puts a debug-only check in the cast below.)

[Devlin, 2016/12/14 02:02:13]

Removed.

+  v8::Local<v8::Object> hooks_object =
+      v8::Local<v8::Object>::Cast(hooks_value);
+  data->hook_interfaces[api_name] =

[jbroman, 2016/12/13 22:12:40]

Up to your preference, but you can use Reset() here to avoid spelling out
v8::Global<v8::Object> here.

data->hook_interfaces[api_name].Reset(context->GetIsolate(), hooks_object);

[Devlin, 2016/12/14 02:02:14]

Works for me, done.

+      v8::Global<v8::Object>(context->GetIsolate(), hooks_object);
+
+  return hooks_object;
+}
+
+// A wrapper to run the provided function with the passed-in arguments.
+void RunJsFunction(const binding::RunJSFunction& run_js,
+                   v8::Local<v8::Function> function,
+                   v8::Local<v8::Context> context,
+                   const binding::APISignature* signature,
+                   gin::Arguments* arguments) {
+  std::vector<v8::Local<v8::Value>> v8_args;
+  if (!arguments->GetRemaining(&v8_args))
+    return;
+  run_js.Run(function, context, v8_args.size(), v8_args.data());

[jbroman, 2016/12/13 22:12:40]

Do these functions ever need the ability to throw exceptions or return values?
Currently, this doesn't seem to permit either.

[Devlin, 2016/12/14 02:02:14]

Yes, it doesn't currently.  It will need to in the future.  I've added a TODO.

+}
+
+}  // namespace
+
+APIBindingHooks::APIBindingHooks(const binding::RunJSFunction& run_js)
+    : run_js_(run_js) {}
 APIBindingHooks::~APIBindingHooks() {}
 
 void APIBindingHooks::RegisterHandleRequest(const std::string& method_name,
@@ -14,13 +145,64 @@ void APIBindingHooks::RegisterHandleRequest(const std::string& method_name,
   request_hooks_[method_name] = hook;
 }
 
+void APIBindingHooks::RegisterJsSource(v8::Global<v8::String> source) {
+  js_hooks_source_ = std::move(source);
+}
+
 APIBindingHooks::HandleRequestHook APIBindingHooks::GetHandleRequest(
-    const std::string& method_name) {
-  auto iter = request_hooks_.find(method_name);
-  if (iter != request_hooks_.end())
-    return iter->second;
+    const std::string& api_name,
+    const std::string& method_name,
+    v8::Local<v8::Context> context) {
+  // Easy case: a native custom hook.
+  auto request_hooks_iter = request_hooks_.find(method_name);
+  if (request_hooks_iter != request_hooks_.end())
+    return request_hooks_iter->second;
+
+  // Harder case: looking up a custom hook registered on the context (since
+  // these are JS, each context has a separate instance).
+  gin::PerContextData* per_context_data = gin::PerContextData::From(context);
+  DCHECK(per_context_data);
+  APIHooksPerContextData* data = static_cast<APIHooksPerContextData*>(
+      per_context_data->GetUserData(kExtensionAPIHooksPerContextKey));
+  if (!data)
+    return HandleRequestHook();
+
+  auto hook_interface_iter = data->hook_interfaces.find(api_name);
+  if (hook_interface_iter == data->hook_interfaces.end())
+    return HandleRequestHook();
+
+  JSHookInterface* hook_interface = nullptr;
+  gin::Converter<JSHookInterface*>::FromV8(
+      context->GetIsolate(),
+      hook_interface_iter->second.Get(context->GetIsolate()), &hook_interface);
+  CHECK(hook_interface);
+
+  auto js_hook_iter = hook_interface->js_hooks()->find(method_name);
+  if (js_hook_iter == hook_interface->js_hooks()->end())
+    return HandleRequestHook();
+
+  return base::Bind(&RunJsFunction, run_js_,

[jbroman, 2016/12/13 22:12:41]

I don't see whether I commented on this previously, but does the caller always
immediately invoke the callback? From here it's harder to tell whether this is
safe or not. e.g. you pass a Local<Context>, which is safe as long as the
callback is only invoked within the HandleScope that created it.

Might it be clearer if the function just did the thing, rather than making a
callback?

[Devlin, 2016/12/14 02:02:13]

Much cleaner now; good idea!

+                    js_hook_iter->second.Get(context->GetIsolate()),
+                    context);
+}
+
+void APIBindingHooks::InitializeInContext(
+    v8::Local<v8::Context> context,
+    const std::string& api_name) {
+  if (js_hooks_source_.IsEmpty())
+    return;
 
-  return HandleRequestHook();
+  v8::Local<v8::String> source = js_hooks_source_.Get(context->GetIsolate());
+  v8::Local<v8::Script> script;
+  if (!v8::Script::Compile(context, source).ToLocal(&script))

[jbroman, 2016/12/13 22:12:40]

nit: Do you want to also pass in a ScriptOrigin with a resource name? It make
exception tracebacks easier to read.

[Devlin, 2016/12/14 02:02:13]

sg; done.

+    return;
+  v8::Local<v8::Value> func_as_value = script->Run();
+  v8::Local<v8::Function> function;
+  if (!gin::ConvertFromV8(context->GetIsolate(), func_as_value, &function))
+    return;
+  v8::Local<v8::Object> api_hooks = GetJSHookInterface(api_name, context);

[jbroman, 2016/12/13 22:12:40]

I can't find the definition of this function. Should it be
CreateJSHookInterface?

[Devlin, 2016/12/14 02:02:13]

Errm, yes.  Done.

+  v8::Local<v8::Value> args[] = {api_hooks};
+  run_js_.Run(function, context, 1, args);

[jbroman, 2016/12/13 22:12:40]

nit: I'd prefer arraysize(args) to 1, in case arguments are added in the future.

[Devlin, 2016/12/14 02:02:14]

Done.

 }
 
 }  // namespace extensions