Use Value objects instead of JSON strings in extension API response IPCs.

When we're sending the result of an extension API method call, we currently
convert a Value object into a JSON string in the browser process, and then call
JSON.parse in javascript on the renderer side. This CL changes that to pass the
Value object in the IPC, and uses V8ValueConverter to turn the Value into the
appropriate v8 native object, avoiding the need to parse JSON.

This is a first step towards supporting binary data for API request
parameters and responses.

BUG=122675
TEST=No new functionality ; existing unit and browser tests should still pass.

Review URL: https://chromiumcodereview.appspot.com/10041047

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@133133 0039d316-1c4b-4281-b951-d872f2087c98

15 files changed, 48 insertions, 40 deletions

diff --git a/chrome/browser/extensions/extension_function.cc b/chrome/browser/extensions/extension_function.cc
index b278df8..bfd5a70 100644
--- a/chrome/browser/extensions/extension_function.cc
+++ b/chrome/browser/extensions/extension_function.cc
@@ -85,20 +85,12 @@ void ExtensionFunction::OnQuotaExceeded() {
   SendResponse(false);
 }
 
-void ExtensionFunction::SetArgs(const ListValue* args) {
+void ExtensionFunction::SetArgs(const base::ListValue* args) {
   DCHECK(!args_.get());  // Should only be called once.
   args_.reset(args->DeepCopy());
 }
 
-const std::string ExtensionFunction::GetResult() {
-  std::string json;
-  // Some functions might not need to return any results.
-  if (result_.get())
-    base::JSONWriter::Write(result_.get(), &json);
-  return json;
-}
-
-Value* ExtensionFunction::GetResultValue() {
+const Value* ExtensionFunction::GetResultValue() {
   return result_.get();
 }
 
@@ -130,8 +122,14 @@ void ExtensionFunction::SendResponseImpl(base::ProcessHandle process,
     return;
   }
 
+  // Value objects can't be directly serialized in our IPC code, so we wrap the
+  // result_ Value with a ListValue (also transferring ownership of result_).
+  base::ListValue result_wrapper;
+  if (result_.get())
+    result_wrapper.Append(result_.release());
+
   ipc_sender->Send(new ExtensionMsg_Response(
-      routing_id, request_id_, success, GetResult(), GetError()));
+      routing_id, request_id_, success, result_wrapper, GetError()));
 }
 
 void ExtensionFunction::HandleBadMessage(base::ProcessHandle process) {
diff --git a/chrome/browser/extensions/extension_function.h b/chrome/browser/extensions/extension_function.h
index 7d9b51a..b68fd09 100644
--- a/chrome/browser/extensions/extension_function.h
+++ b/chrome/browser/extensions/extension_function.h
@@ -78,7 +78,7 @@ class ExtensionFunction
 
   // Execute the API. Clients should initialize the ExtensionFunction using
   // SetArgs(), set_request_id(), and the other setters before calling this
-  // method. Derived classes should be ready to return GetResult() and
+  // method. Derived classes should be ready to return GetResultValue() and
   // GetError() before returning from this function.
   // Note that once Run() returns, dispatcher() can be NULL, so be sure to
   // NULL-check.
@@ -98,12 +98,8 @@ class ExtensionFunction
   // Specifies the raw arguments to the function, as a JSON value.
   virtual void SetArgs(const base::ListValue* args);
 
-  // Retrieves the results of the function as a JSON-encoded string (may
-  // be empty).
-  virtual const std::string GetResult();
-
   // Retrieves the results of the function as a Value.
-  base::Value* GetResultValue();
+  const base::Value* GetResultValue();
 
   // Retrieves any error string from the function.
   virtual const std::string GetError();
diff --git a/chrome/browser/extensions/extension_function_dispatcher.cc b/chrome/browser/extensions/extension_function_dispatcher.cc
index 588d088..78aa9d7 100644
--- a/chrome/browser/extensions/extension_function_dispatcher.cc
+++ b/chrome/browser/extensions/extension_function_dispatcher.cc
@@ -303,7 +303,8 @@ ExtensionFunction* ExtensionFunctionDispatcher::CreateExtensionFunction(
 // static
 void ExtensionFunctionDispatcher::SendAccessDenied(
     IPC::Message::Sender* ipc_sender, int routing_id, int request_id) {
+  ListValue empty_list;
   ipc_sender->Send(new ExtensionMsg_Response(
-      routing_id, request_id, false, std::string(),
+      routing_id, request_id, false, empty_list,
       "Access to extension API denied."));
 }
diff --git a/chrome/browser/extensions/extension_function_test_utils.cc b/chrome/browser/extensions/extension_function_test_utils.cc
index 35989dc..f5561f3 100644
--- a/chrome/browser/extensions/extension_function_test_utils.cc
+++ b/chrome/browser/extensions/extension_function_test_utils.cc
@@ -123,8 +123,7 @@ std::string RunFunctionAndReturnError(UIThreadExtensionFunction* function,
                                       RunFunctionFlags flags) {
   scoped_refptr<ExtensionFunction> function_owner(function);
   RunFunction(function, args, browser, flags);
-  EXPECT_FALSE(function->GetResultValue()) << "Unexpected function result " <<
-      function->GetResult();
+  EXPECT_FALSE(function->GetResultValue()) << "Did not expect a result";
   return function->GetError();
 }
 
diff --git a/chrome/browser/extensions/extensions_quota_service_unittest.cc b/chrome/browser/extensions/extensions_quota_service_unittest.cc
index 2bf5b20..c13b4f0 100644
--- a/chrome/browser/extensions/extensions_quota_service_unittest.cc
+++ b/chrome/browser/extensions/extensions_quota_service_unittest.cc
@@ -59,7 +59,6 @@ class MockFunction : public ExtensionFunction {
   virtual void SetArgs(const ListValue* args) OVERRIDE {}
   virtual const std::string GetError() OVERRIDE { return std::string(); }
   virtual void SetError(const std::string& error) OVERRIDE {}
-  virtual const std::string GetResult() OVERRIDE { return std::string(); }
   virtual void Run() OVERRIDE {}
   virtual void Destruct() const OVERRIDE { delete this; }
   virtual bool RunImpl() OVERRIDE { return true; }
diff --git a/chrome/common/extensions/extension_messages.h b/chrome/common/extensions/extension_messages.h
index ff35b1e..f02a952 100644
--- a/chrome/common/extensions/extension_messages.h
+++ b/chrome/common/extensions/extension_messages.h
@@ -170,11 +170,13 @@ struct ParamTraits<ExtensionMsg_Loaded_Params> {
 
 // Messages sent from the browser to the renderer.
 
-// The browser sends this message in response to all extension api calls.
+// The browser sends this message in response to all extension api calls. The
+// response data (if any) is one of the base::Value subclasses, wrapped as the
+// first element in a ListValue.
 IPC_MESSAGE_ROUTED4(ExtensionMsg_Response,
                     int /* request_id */,
                     bool /* success */,
-                    std::string /* response */,
+                    ListValue /* response wrapper (see comment above) */,
                     std::string /* error */)
 
 // This message is optionally routed.  If used as a control message, it
diff --git a/chrome/renderer/extensions/extension_dispatcher.cc b/chrome/renderer/extensions/extension_dispatcher.cc
index 9b2744c..0a28bf6 100644
--- a/chrome/renderer/extensions/extension_dispatcher.cc
+++ b/chrome/renderer/extensions/extension_dispatcher.cc
@@ -863,7 +863,7 @@ Feature::Context ExtensionDispatcher::ClassifyJavaScriptContext(
 
 void ExtensionDispatcher::OnExtensionResponse(int request_id,
                                               bool success,
-                                              const std::string& response,
+                                              const base::ListValue& response,
                                               const std::string& error) {
   request_sender_->HandleResponse(request_id, success, response, error);
 }
diff --git a/chrome/renderer/extensions/extension_dispatcher.h b/chrome/renderer/extensions/extension_dispatcher.h
index 4e87366..cb7f60a 100644
--- a/chrome/renderer/extensions/extension_dispatcher.h
+++ b/chrome/renderer/extensions/extension_dispatcher.h
@@ -102,7 +102,7 @@ class ExtensionDispatcher : public content::RenderProcessObserver {
 
   void OnExtensionResponse(int request_id,
                            bool success,
-                           const std::string& response,
+                           const base::ListValue& response,
                            const std::string& error);
 
   // Checks that the current context contains an extension that has permission
diff --git a/chrome/renderer/extensions/extension_helper.cc b/chrome/renderer/extensions/extension_helper.cc
index 41fd732..3aba7c4 100644
--- a/chrome/renderer/extensions/extension_helper.cc
+++ b/chrome/renderer/extensions/extension_helper.cc
@@ -277,7 +277,7 @@ void ExtensionHelper::DidCreateDataSource(WebFrame* frame, WebDataSource* ds) {
 
 void ExtensionHelper::OnExtensionResponse(int request_id,
                                           bool success,
-                                          const std::string& response,
+                                          const base::ListValue& response,
                                           const std::string& error) {
   extension_dispatcher_->OnExtensionResponse(request_id,
                                              success,
@@ -287,7 +287,7 @@ void ExtensionHelper::OnExtensionResponse(int request_id,
 
 void ExtensionHelper::OnExtensionMessageInvoke(const std::string& extension_id,
                                                const std::string& function_name,
-                                               const ListValue& args,
+                                               const base::ListValue& args,
                                                const GURL& event_url,
                                                bool user_gesture) {
   scoped_ptr<WebScopedUserGesture> web_user_gesture;
diff --git a/chrome/renderer/extensions/extension_helper.h b/chrome/renderer/extensions/extension_helper.h
index a1ed3ed..2376b94 100644
--- a/chrome/renderer/extensions/extension_helper.h
+++ b/chrome/renderer/extensions/extension_helper.h
@@ -74,7 +74,7 @@ class ExtensionHelper
                                    WebKit::WebDataSource* ds) OVERRIDE;
 
   void OnExtensionResponse(int request_id, bool success,
-                           const std::string& response,
+                           const base::ListValue& response,
                            const std::string& error);
   void OnExtensionMessageInvoke(const std::string& extension_id,
                                 const std::string& function_name,
diff --git a/chrome/renderer/extensions/extension_request_sender.cc b/chrome/renderer/extensions/extension_request_sender.cc
index 9b6ce3f..dd9d712 100644
--- a/chrome/renderer/extensions/extension_request_sender.cc
+++ b/chrome/renderer/extensions/extension_request_sender.cc
@@ -10,10 +10,13 @@
 #include "chrome/renderer/extensions/chrome_v8_context_set.h"
 #include "chrome/renderer/extensions/extension_dispatcher.h"
 #include "content/public/renderer/render_view.h"
+#include "content/public/renderer/v8_value_converter.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebDocument.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebFrame.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebSecurityOrigin.h"
 
+using content::V8ValueConverter;
+
 // Contains info relevant to a pending API request.
 struct PendingRequest {
  public :
@@ -122,7 +125,7 @@ void ExtensionRequestSender::StartRequest(
 
 void ExtensionRequestSender::HandleResponse(int request_id,
                                             bool success,
-                                            const std::string& response,
+                                            const base::ListValue& response,
                                             const std::string& error) {
   linked_ptr<PendingRequest> request = RemoveRequest(request_id);
 
@@ -138,11 +141,24 @@ void ExtensionRequestSender::HandleResponse(int request_id,
     return;  // The frame went away.
 
   v8::HandleScope handle_scope;
+  v8::Handle<v8::Value> response_value = v8::Undefined();
+  DCHECK(response.GetSize() <= 1);
+  if (response.GetSize() == 1) {
+    // We use a ListValue as a wrapper (our IPC system can't handle Value
+    // directly since it has a private constructor) so we need to extract the
+    // value at index 0.
+    Value* value = NULL;
+    CHECK(response.Get(0, &value));
+    CHECK(value);
+    scoped_ptr<V8ValueConverter> converter(V8ValueConverter::create());
+    response_value = converter->ToV8Value(value, v8_context->v8_context());
+  }
+
   v8::Handle<v8::Value> argv[5];
   argv[0] = v8::Integer::New(request_id);
   argv[1] = v8::String::New(request->name.c_str());
   argv[2] = v8::Boolean::New(success);
-  argv[3] = v8::String::New(response.c_str());
+  argv[3] = response_value;
   argv[4] = v8::String::New(error.c_str());
 
   v8::Handle<v8::Value> retval;
diff --git a/chrome/renderer/extensions/extension_request_sender.h b/chrome/renderer/extensions/extension_request_sender.h
index 3188e43..20f7baf 100644
--- a/chrome/renderer/extensions/extension_request_sender.h
+++ b/chrome/renderer/extensions/extension_request_sender.h
@@ -42,7 +42,7 @@ class ExtensionRequestSender {
   // Handles responses from the extension host to calls made by StartRequest().
   void HandleResponse(int request_id,
                       bool success,
-                      const std::string& response,
+                      const base::ListValue& response,
                       const std::string& error);
 
 
diff --git a/chrome/renderer/resources/extensions/file_browser_private_custom_bindings.js b/chrome/renderer/resources/extensions/file_browser_private_custom_bindings.js
index 2423cfb..3f702b7 100644
--- a/chrome/renderer/resources/extensions/file_browser_private_custom_bindings.js
+++ b/chrome/renderer/resources/extensions/file_browser_private_custom_bindings.js
@@ -14,10 +14,9 @@ chromeHidden.registerCustomHook('fileBrowserPrivate', function(bindingsAPI) {
 
   apiFunctions.setCustomCallback('requestLocalFileSystem',
                                  function(name, request, response) {
-    var resp = response ? [chromeHidden.JSON.parse(response)] : [];
     var fs = null;
-    if (!resp[0].error)
-      fs = GetLocalFileSystem(resp[0].name, resp[0].path);
+    if (response && !response.error)
+      fs = GetLocalFileSystem(response.name, response.path);
     if (request.callback)
       request.callback(fs);
     request.callback = null;
diff --git a/chrome/renderer/resources/extensions/page_capture_custom_bindings.js b/chrome/renderer/resources/extensions/page_capture_custom_bindings.js
index 0bcbb0b..15b82f9 100644
--- a/chrome/renderer/resources/extensions/page_capture_custom_bindings.js
+++ b/chrome/renderer/resources/extensions/page_capture_custom_bindings.js
@@ -15,9 +15,8 @@ chromeHidden.registerCustomHook('pageCapture', function(bindingsAPI) {
 
   apiFunctions.setCustomCallback('saveAsMHTML',
                                  function(name, request, response) {
-    var params = chromeHidden.JSON.parse(response);
-    var path = params.mhtmlFilePath;
-    var size = params.mhtmlFileLength;
+    var path = response.mhtmlFilePath;
+    var size = response.mhtmlFileLength;
 
     if (request.callback)
       request.callback(CreateBlob(path, size));
diff --git a/chrome/renderer/resources/extensions/send_request.js b/chrome/renderer/resources/extensions/send_request.js
index 3e47f40..045e270 100644
--- a/chrome/renderer/resources/extensions/send_request.js
+++ b/chrome/renderer/resources/extensions/send_request.js
@@ -29,7 +29,7 @@ chromeHidden.handleResponse = function(requestId, name,
 
     if (request.callback) {
       // Callbacks currently only support one callback argument.
-      var callbackArgs = response ? [chromeHidden.JSON.parse(response)] : [];
+      var callbackArgs = typeof(response) != "undefined" ? [response] : [];
 
       // Validate callback in debug only -- and only when the
       // caller has provided a callback. Implementations of api
@@ -44,7 +44,6 @@ chromeHidden.handleResponse = function(requestId, name,
           if (request.callbackSchema.parameters.length > 1) {
             throw new Error("Callbacks may only define one parameter");
           }
-
           chromeHidden.validate(callbackArgs,
               request.callbackSchema.parameters);
         } catch (exception) {
@@ -53,7 +52,7 @@ chromeHidden.handleResponse = function(requestId, name,
         }
       }
 
-      if (response) {
+      if (typeof(response) != "undefined") {
         request.callback(callbackArgs[0]);
       } else {
         request.callback();