[Extensions Binding] Allow for registering custom hooks

extensions/renderer/api_bindings_system.h

Index: extensions/renderer/api_bindings_system.h
diff --git a/extensions/renderer/api_bindings_system.h b/extensions/renderer/api_bindings_system.h
index e40b898bcee5c215150694c27dd9ba494f2477f1..e8f30cf9f4ff95ce8828b840db03c0304cae3454 100644
--- a/extensions/renderer/api_bindings_system.h
+++ b/extensions/renderer/api_bindings_system.h
@@ -70,6 +70,11 @@ class APIBindingsSystem {
                           v8::Local<v8::Context> context,
                           const base::ListValue& response);
 
+  // Returns the APIBindingHooks object for the given api to allow for
+  // registering custom hooks. These must be registered *before* the
+  // binding is instantiated.
+  APIBindingHooks* GetHooksForAPI(const std::string& api_name);

[jbroman, 2016/12/08 16:58:56]

The use of GetHooksForAPI to do the registration seems untested at present.

[Devlin, 2016/12/08 19:12:03]

I wasn't sure there was much useful to test here (since it's mostly just in
hooks/binding), but went ahead and added one.

+

[jbroman, 2016/12/08 16:58:56]

Is there a reason for the asymmetry, where API schemas are requested from the
client lazily via GetAPISchemaMethod, but hooks are registered in advance? I'd
have expected this class to either consistently require up-front registration,
or consistently do that work lazily.

[Devlin, 2016/12/08 19:12:03]

Let's say we have a custom hook (call it CustomHook()).  For testability (and
isolation of knowledge), we don't want APIBindingsSystem to know about
CustomHook().  That means that we can have this registered in advance of
creation, like this, or on-demand.  For the on-demand approach, we'd need to
introduce another callback of some kind for GetHookForAPI(), but that seems like
a needlessly messy function to implement.  It's doable, but it feels clunkier
from most aspects.  So, next question, why don't we require all schemas
up-front, which is performance.  If we never request a schema, there should be
no reason to fetch all the resources (particularly when we also need to parse
each of those schemas into a dictionary value!).

Looking ahead to when we have some sort of generated schema data structure, I
think it *does* make sense to have schema knowledge up front, but we're not
there yet.

WDYT?

  private:
   // Creates a new APIBinding for the given |api_name|.
   std::unique_ptr<APIBinding> CreateNewAPIBinding(const std::string& api_name);
@@ -95,6 +100,9 @@ class APIBindingsSystem {
   // created lazily.
   std::map<std::string, std::unique_ptr<APIBinding>> api_bindings_;
 
+  // A map from api_name -> APIBindingHooks for registering custom hooks.
+  std::map<std::string, std::unique_ptr<APIBindingHooks>> binding_hooks_;

[jbroman, 2016/12/08 16:58:56]

We might want to watch out for this later -- it is very pointer-y (map already
does separate allocations for its nodes, and we have two layers of them plus the
separate allocation for APIBindingHooks). Not sure whether the number of entries
is large enough for this to be a big deal for performance, though.

[Devlin, 2016/12/08 19:12:03]

Added a note in the code.

+
   // The method to retrieve the DictionaryValue describing a given extension
   // API. Curried in for testing purposes so we can use fake APIs.
   GetAPISchemaMethod get_api_schema_;