[Extensions Binding] Allow for registering custom hooks

extensions/renderer/api_bindings_system.h

 // 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.
 
 #ifndef EXTENSIONS_RENDERER_API_BINDINGS_SYSTEM_H_
 #define EXTENSIONS_RENDERER_API_BINDINGS_SYSTEM_H_
 
 #include <map>
 #include <memory>
 #include <string>
@@ skipping 52 matching lines @@
   // Responds to the request with the given |request_id|, calling the callback
   // with |response|.
   void CompleteRequest(int request_id, const base::ListValue& response);
 
   // Notifies the APIEventHandler to fire the corresponding event, notifying
   // listeners.
   void FireEventInContext(const std::string& event_name,
                           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);
 
   // Handles a call into an API, adds a pending request to the
   // |request_handler_|, and calls |send_request_|.
   void OnAPICall(const std::string& name,
                  std::unique_ptr<base::ListValue> arguments,
                  v8::Isolate* isolate,
                  v8::Local<v8::Context> context,
                  v8::Local<v8::Function> callback);
 
   // The map of cached API reference types.
   ArgumentSpec::RefMap type_reference_map_;
 
   // The request handler associated with the system.
   APIRequestHandler request_handler_;
 
   // The event handler associated with the system.
   APIEventHandler event_handler_;
 
   // A map from api_name -> APIBinding for constructed APIs. APIBindings are
   // 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_;
 
   // The method to call when a new API call is triggered. Curried in for testing
   // purposes. Typically, this would send an IPC to the browser to begin the
   // function work.
   SendRequestMethod send_request_;
 
   DISALLOW_COPY_AND_ASSIGN(APIBindingsSystem);
 };
 
 }  // namespace
 
 #endif  // EXTENSIONS_RENDERER_API_BINDINGS_SYSTEM_H_