[Extensions Bindings] Move request dispatch to APIRequestHandler

extensions/renderer/api_request_handler.cc

Index: extensions/renderer/api_request_handler.cc
diff --git a/extensions/renderer/api_request_handler.cc b/extensions/renderer/api_request_handler.cc
index a55d71429aad3e7bddbe2d776de5a1a50dca573d..1dd7ca21df1c8e92395443bfecbdc21e3614a6a6 100644
--- a/extensions/renderer/api_request_handler.cc
+++ b/extensions/renderer/api_request_handler.cc
@@ -9,11 +9,15 @@
 #include "base/memory/ptr_util.h"
 #include "base/values.h"
 #include "content/public/child/v8_value_converter.h"
+#include "gin/converter.h"
 #include "third_party/WebKit/public/web/WebScopedUserGesture.h"
 #include "third_party/WebKit/public/web/WebUserGestureIndicator.h"
 
 namespace extensions {
 
+APIRequestHandler::Request::Request() {}
+APIRequestHandler::Request::~Request() = default;

[lazyboy, 2017/02/17 19:47:13]

Is there any particular reason to choose "default" instead of {}?

[Devlin, 2017/02/17 21:57:41]

Good question.  I've noticed a trend towards using default for destructors, but
in looking through the style, I can't find any indication that one is preferred
over the other (and both are allowed).  Do you have a preference in either
direction?

(Or Jeremy, do you know if there's a reason to use one over the other?)

[lazyboy, 2017/02/17 22:06:55]

This is fine, I was just curious if this was was intentional for some reason and
I wanted to learn...
lgtm

[jbroman, 2017/02/21 00:55:55]

I don't have a preference one way or another, and the style guide is silent
about it. "= default;" is more helpful for copy/move constructors and assignment
operators, but for destructors it's fine either way.

+
 APIRequestHandler::PendingRequest::PendingRequest(
     v8::Isolate* isolate,
     v8::Local<v8::Function> callback,
@@ -36,24 +40,60 @@ APIRequestHandler::PendingRequest::PendingRequest(PendingRequest&&) = default;
 APIRequestHandler::PendingRequest& APIRequestHandler::PendingRequest::operator=(
     PendingRequest&&) = default;
 
-APIRequestHandler::APIRequestHandler(const CallJSFunction& call_js,
+APIRequestHandler::APIRequestHandler(const SendRequestMethod& send_request,
+                                     const CallJSFunction& call_js,
                                      APILastError last_error)
-    : call_js_(call_js), last_error_(std::move(last_error)) {}
+    : send_request_(send_request),
+      call_js_(call_js),
+      last_error_(std::move(last_error)) {}
 
 APIRequestHandler::~APIRequestHandler() {}
 
-int APIRequestHandler::AddPendingRequest(
-    v8::Isolate* isolate,
-    v8::Local<v8::Function> callback,
-    v8::Local<v8::Context> context,
-    const std::vector<v8::Local<v8::Value>>& callback_args) {
-  // TODO(devlin): We could *probably* get away with just using an integer here,
-  // but it's a little less foolproof. How slow is GenerateGUID? Should we use
-  // that instead? It means updating the IPC (ExtensionHostMsg_Request).
-  // base::UnguessableToken is another good option.
-  int id = next_request_id_++;
-  pending_requests_.insert(std::make_pair(
-      id, PendingRequest(isolate, callback, context, callback_args)));
+int APIRequestHandler::StartRequest(v8::Local<v8::Context> context,
+                                    const std::string& method,
+                                    std::unique_ptr<base::ListValue> arguments,
+                                    v8::Local<v8::Function> callback,
+                                    v8::Local<v8::Function> custom_callback) {
+  auto request = base::MakeUnique<Request>();
+
+  if (!custom_callback.IsEmpty() || !callback.IsEmpty()) {
+    v8::Isolate* isolate = context->GetIsolate();
+    // In the JS bindings, custom callbacks are called with the arguments of
+    // name, the full request object (see below), the original callback, and
+    // the responses from the API. The responses from the API are handled by the
+    // APIRequestHandler, but we need to curry in the other values.
+    std::vector<v8::Local<v8::Value>> callback_args;
+    if (!custom_callback.IsEmpty()) {
+      // TODO(devlin): The |request| object in the JS bindings includes
+      // properties for callback, callbackSchema, args, stack, id, and
+      // customCallback. Of those, it appears that we only use stack, args, and
+      // id (since callback is curried in separately). We may be able to update
+      // bindings to get away from some of those. For now, just pass in an empty
+      // object (most APIs don't rely on it).
+      v8::Local<v8::Object> request = v8::Object::New(isolate);
+      callback_args = {gin::StringToSymbol(isolate, method), request, callback};
+      callback = custom_callback;
+    }
+
+    // TODO(devlin): We could *probably* get away with just using an integer
+    // here, but it's a little less foolproof. How slow is GenerateGUID? Should
+    // we use that instead? It means updating the IPC
+    // (ExtensionHostMsg_Request).
+    // base::UnguessableToken is another good option.
+    request->request_id = next_request_id_++;
+    request->has_callback = true;
+    pending_requests_.insert(std::make_pair(
+        request->request_id,
+        PendingRequest(isolate, callback, context, callback_args)));
+  }
+
+  request->has_user_gesture =
+      blink::WebUserGestureIndicator::isProcessingUserGestureThreadSafe();
+  request->arguments = std::move(arguments);
+  request->method_name = method;
+
+  int id = request->request_id;
+  send_request_.Run(std::move(request), context);
   return id;
 }