[Extensions Bindings] Include request id in a custom callback response

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 369cfcb1495bafd50dfb8f4d3e697e1e5a409205..60e759b173e77098f95713e33a56acc73636f6cf 100644
--- a/extensions/renderer/api_request_handler.cc
+++ b/extensions/renderer/api_request_handler.cc
@@ -10,6 +10,7 @@
 #include "base/values.h"
 #include "content/public/child/v8_value_converter.h"
 #include "gin/converter.h"
+#include "gin/data_object_builder.h"
 #include "third_party/WebKit/public/web/WebScopedUserGesture.h"
 #include "third_party/WebKit/public/web/WebUserGestureIndicator.h"
 
@@ -57,6 +58,8 @@ int APIRequestHandler::StartRequest(v8::Local<v8::Context> context,
                                     binding::RequestThread thread) {
   auto request = base::MakeUnique<Request>();
 
+  int request_id = next_request_id_++;

[jbroman, 2017/05/23 22:15:03]

Should this be inside the if condition? It's not used outside it, so this seems
to increment it without doing anything else. If it should be here, comment?

[Devlin, 2017/05/24 16:08:33]

The browser side uses these identifiers, even though it's not the best practice.
 I've added a comment and a TODO.

+
   if (!custom_callback.IsEmpty() || !callback.IsEmpty()) {
     v8::Isolate* isolate = context->GetIsolate();
     // In the JS bindings, custom callbacks are called with the arguments of
@@ -69,9 +72,10 @@ int APIRequestHandler::StartRequest(v8::Local<v8::Context> context,
       // 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);
+      // bindings to get away from some of those. For now, just pass in an
+      // object with the request id.
+      v8::Local<v8::Object> request =
+          gin::DataObjectBuilder(isolate).Set("id", request_id).Build();
       v8::Local<v8::Value> callback_to_pass = callback;
       if (callback_to_pass.IsEmpty())
         callback_to_pass = v8::Undefined(isolate);
@@ -85,7 +89,7 @@ int APIRequestHandler::StartRequest(v8::Local<v8::Context> context,
     // we use that instead? It means updating the IPC
     // (ExtensionHostMsg_Request).
     // base::UnguessableToken is another good option.
-    request->request_id = next_request_id_++;
+    request->request_id = request_id;
     request->has_callback = true;
     pending_requests_.insert(std::make_pair(
         request->request_id,