[Extensions Binding] Allow for registering custom hooks

extensions/renderer/api_binding_hooks.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_BINDING_HOOKS_H_
+#define EXTENSIONS_RENDERER_API_BINDING_HOOKS_H_
+
+#include <map>
+#include <memory>
+#include <string>
+
+#include "base/callback.h"
+#include "base/macros.h"
+#include "extensions/renderer/api_binding_types.h"
+#include "v8/include/v8.h"
+
+namespace gin {
+class Arguments;
+}
+
+namespace extensions {
+
+// A class to register custom hooks for given API calls that need different
+// handling. An instance exists for a single API, but can be used across
+// multiple contexts (but only on the same thread).
+// TODO(devlin): We have both C++ and JS custom bindings, but this only allows
+// for registration of C++ handlers. Add JS support.
+class APIBindingHooks {

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

Is this class expected to become more complicated when JS support is added? At
the moment, it's just a map.

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

Yep, it will be more complicated.  I actually started implementing it, but the
CL started to get much larger, and I figured it best to split it up.  The
biggest difference is that with JS bindings, you have to have it associated with
a context as well (because the JS function is associated with the context), and
so there's a bit more work in the ability to get the proper request handler.

+ public:
+  using HandleRequestHook =
+      base::Callback<void(const binding::APISignature*, gin::Arguments*)>;

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

Is the APISignature* parameter useful here? I'd have expected each hook to
already know the signature of the function it's implementing.

Also, what sort of hooks are typical? This doesn't provide a lot of support to
help with the tricky things like managing callback lifetime. Do custom hooks not
need to do that, or will support for that be added later?

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

In JS, we have a few different hooks.  Specifically:
- updateArgumentsPreValidate (calls with a chance to massage the arguments
before we compare to the expected signature)
- updateArgumentsPostValidate (calls to massage after we match to the expected
signature)
- handleRequest (handles the implementation after verifying args)
- customCallback (allows the hook to manipulate the response)

In C++, I think we can achieve most of those with a single hook and exposing
different methods.  E.g., we can have a method to match args, and then if the
handler wants to adjust args, then match, then handle, the impl is just
MatchArgs(signature, args);
UpdateArgs(args);
Handle();
etc.

Sorry for the long answer.  So that's why APISignature is here. :)

I'm not quite sure I follow about things like managing callback lifetime. 
What's the concern there?

[jbroman, 2016/12/09 15:53:08]

Ah, I'd assumed the hook replaced the entire function implementation. At that
point, I'd be worried about something custom that took a v8::Function out of the
gin::Arguments, and then had to worry about managing its lifetime. If the
callback will still go through the current flow (and these just "massage" the
arguments and response), then maybe that's not such a worry.

[Devlin, 2016/12/09 16:35:06]

Yeah, I'm hoping we can have the APIBinding/APIRequestHandler deal with most of
the lifetime.  This hooks API will probably change over time as we allow it to
be used with more custom bindings, but hopefully the concept remains the same.

+
+  explicit APIBindingHooks(const std::string& api_name);
+  ~APIBindingHooks();
+
+  // Register a custom binding to handle requests.
+  void RegisterHandleRequest(const std::string& method_name,
+                             const HandleRequestHook& hook);
+
+  // Returns the custom hook for the given method, or a null callback if none
+  // exists.
+  HandleRequestHook GetHandleRequest(const std::string& method_name);
+
+ private:
+  // The name of the API.
+  std::string api_name_;

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

This variable seems unused.

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

Ah, it is indeed now (leftover from when the JS impl was here).  Removed.

+
+  // All registered request handlers.
+  std::map<std::string, HandleRequestHook> request_hooks_;
+
+  DISALLOW_COPY_AND_ASSIGN(APIBindingHooks);
+};
+
+}  // namespace extensions
+
+#endif  // EXTENSIONS_RENDERER_API_BINDING_HOOKS_H_