[Extensions Bindings] Move request dispatch to APIRequestHandler

extensions/renderer/api_request_handler.cc

 // 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.
 
 #include "extensions/renderer/api_request_handler.h"
 
 #include "base/bind.h"
 #include "base/guid.h"
 #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,
     v8::Local<v8::Context> context,
     const std::vector<v8::Local<v8::Value>>& local_callback_args)
     : isolate(isolate),
       context(isolate, context),
       callback(isolate, callback),
       user_gesture_token(
           blink::WebUserGestureIndicator::currentUserGestureToken()) {
   if (!local_callback_args.empty()) {
     callback_arguments.reserve(local_callback_args.size());
     for (const auto& arg : local_callback_args)
       callback_arguments.push_back(v8::Global<v8::Value>(isolate, arg));
   }
 }
 
 APIRequestHandler::PendingRequest::~PendingRequest() {}
 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;
 }
 
 void APIRequestHandler::CompleteRequest(int request_id,
                                         const base::ListValue& response_args,
                                         const std::string& error) {
   auto iter = pending_requests_.find(request_id);
   // The request may have been removed if the context was invalidated before a
   // response is ready.
   if (iter == pending_requests_.end())
@@ skipping 40 matching lines @@
 }
 
 std::set<int> APIRequestHandler::GetPendingRequestIdsForTesting() const {
   std::set<int> result;
   for (const auto& pair : pending_requests_)
     result.insert(pair.first);
   return result;
 }
 
 }  // namespace extensions