Refactor setIcon to allow for more general use of imageData.

Review URL: https://codereview.chromium.org/477193003

Cr-Commit-Position: refs/heads/master@{#291119}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@291119 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 92 insertions, 95 deletions

diff --git a/chrome/renderer/resources/extensions/browser_action_custom_bindings.js b/chrome/renderer/resources/extensions/browser_action_custom_bindings.js
index 60ac0c3..5704305 100644
--- a/chrome/renderer/resources/extensions/browser_action_custom_bindings.js
+++ b/chrome/renderer/resources/extensions/browser_action_custom_bindings.js
@@ -8,13 +8,15 @@ var binding = require('binding').Binding.create('browserAction');
 
 var setIcon = require('setIcon').setIcon;
 var getExtensionViews = requireNative('runtime').GetExtensionViews;
+var sendRequest = require('sendRequest').sendRequest;
 
 binding.registerCustomHook(function(bindingsAPI) {
   var apiFunctions = bindingsAPI.apiFunctions;
 
   apiFunctions.setHandleRequest('setIcon', function(details, callback) {
-    setIcon(details, callback, this.name, this.definition.parameters,
-        'browser action');
+    setIcon(details, function(args) {
+      sendRequest(this.name, [args, callback], this.definition.parameters);
+    }.bind(this));
   });
 
   apiFunctions.setCustomCallback('openPopup',
diff --git a/chrome/renderer/resources/extensions/page_action_custom_bindings.js b/chrome/renderer/resources/extensions/page_action_custom_bindings.js
index 97a308e..b4f92c6 100644
--- a/chrome/renderer/resources/extensions/page_action_custom_bindings.js
+++ b/chrome/renderer/resources/extensions/page_action_custom_bindings.js
@@ -7,13 +7,15 @@
 var binding = require('binding').Binding.create('pageAction');
 
 var setIcon = require('setIcon').setIcon;
+var sendRequest = require('sendRequest').sendRequest;
 
 binding.registerCustomHook(function(bindingsAPI) {
   var apiFunctions = bindingsAPI.apiFunctions;
 
   apiFunctions.setHandleRequest('setIcon', function(details, callback) {
-    setIcon(details, callback, this.name, this.definition.parameters,
-        'page action');
+    setIcon(details, function(args) {
+      sendRequest(this.name, [args, callback], this.definition.parameters);
+    }.bind(this));
   });
 });
 
diff --git a/chrome/renderer/resources/extensions/system_indicator_custom_bindings.js b/chrome/renderer/resources/extensions/system_indicator_custom_bindings.js
index 91d02a6..e07a9cd 100644
--- a/chrome/renderer/resources/extensions/system_indicator_custom_bindings.js
+++ b/chrome/renderer/resources/extensions/system_indicator_custom_bindings.js
@@ -9,13 +9,15 @@
 var binding = require('binding').Binding.create('systemIndicator');
 
 var setIcon = require('setIcon').setIcon;
+var sendRequest = require('sendRequest').sendRequest;
 
 binding.registerCustomHook(function(bindingsAPI) {
   var apiFunctions = bindingsAPI.apiFunctions;
 
   apiFunctions.setHandleRequest('setIcon', function(details, callback) {
-    setIcon(details, callback, this.name, this.definition.parameters,
-            'system indicator');
+    setIcon(details, function(args) {
+      sendRequest(this.name, [args, callback], this.definition.parameters);
+    }.bind(this));
   });
 });
 
diff --git a/extensions/renderer/resources/set_icon.js b/extensions/renderer/resources/set_icon.js
index 883fd67..83f5a80 100644
--- a/extensions/renderer/resources/set_icon.js
+++ b/extensions/renderer/resources/set_icon.js
@@ -5,11 +5,10 @@
 var SetIconCommon = requireNative('setIcon').SetIconCommon;
 var sendRequest = require('sendRequest').sendRequest;
 
-function loadImagePath(path, iconSize, actionType, callback) {
+function loadImagePath(path, iconSize, callback) {
   var img = new Image();
   img.onerror = function() {
-    console.error('Could not load ' + actionType + ' icon \'' +
-                  path + '\'.');
+    console.error('Could not load action icon \'' + path + '\'.');
   };
   img.onload = function() {
     var canvas = document.createElement('canvas');
@@ -49,39 +48,51 @@ function verifyImageData(imageData, iconSize) {
   }
 }
 
-function setIcon(details, callback, name, parameters, actionType) {
+/**
+ * Normalizes |details| to a format suitable for sending to the browser,
+ * for example converting ImageData to a binary representation.
+ *
+ * @param {ImageDetails} details
+ *   The ImageDetails passed into an extension action-style API.
+ * @param {Function} callback
+ *   The callback function to pass processed imageData back to. Note that this
+ *   callback may be called reentrantly.
+ */
+function setIcon(details, callback) {
   var iconSizes = [19, 38];
+  // Note that iconIndex is actually deprecated, and only available to the
+  // pageAction API.
+  // TODO(kalman): Investigate whether this is for the pageActions API, and if
+  // so, delete it.
   if ('iconIndex' in details) {
-    sendRequest(name, [details, callback], parameters);
-  } else if ('imageData' in details) {
-    if (typeof details.imageData == 'object') {
-      var isEmpty = true;
-      for (var i = 0; i < iconSizes.length; i++) {
-        var sizeKey = iconSizes[i].toString();
-        if (sizeKey in details.imageData) {
-          verifyImageData(details.imageData[sizeKey], iconSizes[i]);
-          isEmpty =false;
-        }
+    callback(details);
+    return;
+  }
+
+  if ('imageData' in details) {
+    var isEmpty = true;
+    for (var i = 0; i < iconSizes.length; i++) {
+      var sizeKey = iconSizes[i].toString();
+      if (sizeKey in details.imageData) {
+        verifyImageData(details.imageData[sizeKey], iconSizes[i]);
+        isEmpty =false;
       }
+    }
 
-      if (!isEmpty) {
-        sendRequest(name, [details, callback], parameters,
-                    {nativeFunction: SetIconCommon});
-      } else {
-        // If details.imageData is not dictionary with keys in set {'19', '38'},
-        // it must be an ImageData object.
-        var sizeKey = iconSizes[0].toString();
-        var imageData = details.imageData;
-        details.imageData = {};
-        details.imageData[sizeKey] = imageData;
-        verifyImageData(details.imageData[sizeKey], iconSizes[0]);
-        sendRequest(name, [details, callback], parameters,
-                    {nativeFunction: SetIconCommon});
-     }
-    } else {
-      throw new Error('imageData property has unexpected type.');
+    if (isEmpty) {
+      // If details.imageData is not dictionary with keys in set {'19', '38'},
+      // it must be an ImageData object.
+      var sizeKey = iconSizes[0].toString();
+      var imageData = details.imageData;
+      details.imageData = {};
+      details.imageData[sizeKey] = imageData;
+      verifyImageData(details.imageData[sizeKey], iconSizes[0]);
     }
-  } else if ('path' in details) {
+    callback(SetIconCommon(details));
+    return;
+  }
+
+  if ('path' in details) {
     if (typeof details.path == 'object') {
       details.imageData = {};
       var isEmpty = true;
@@ -90,8 +101,7 @@ function setIcon(details, callback, name, parameters, actionType) {
           delete details.path;
           if (isEmpty)
             throw new Error('The path property must not be empty.');
-          sendRequest(name, [details, callback], parameters,
-                      {nativeFunction: SetIconCommon});
+          callback(SetIconCommon(details));
           return;
         }
         var sizeKey = iconSizes[index].toString();
@@ -100,32 +110,27 @@ function setIcon(details, callback, name, parameters, actionType) {
           return;
         }
         isEmpty = false;
-        loadImagePath(details.path[sizeKey], iconSizes[index], actionType,
+        loadImagePath(details.path[sizeKey], iconSizes[index],
           function(imageData) {
             details.imageData[sizeKey] = imageData;
             processIconSize(index + 1);
           });
       }
-
       processIconSize(0);
     } else if (typeof details.path == 'string') {
       var sizeKey = iconSizes[0].toString();
       details.imageData = {};
-      loadImagePath(details.path, iconSizes[0], actionType,
+      loadImagePath(details.path, iconSizes[0],
           function(imageData) {
             details.imageData[sizeKey] = imageData;
             delete details.path;
-            sendRequest(name, [details, callback], parameters,
-                        {nativeFunction: SetIconCommon});
+            callback(SetIconCommon(details));
+            return;
       });
-    } else {
-      throw new Error('The path property should contain either string or ' +
-                      'dictionary of strings.');
     }
-  } else {
-    throw new Error(
-        'Either the path or imageData property must be specified.');
+    return;
   }
+  throw new Error('Either the path or imageData property must be specified.');
 }
 
 exports.setIcon = setIcon;
diff --git a/extensions/renderer/set_icon_natives.cc b/extensions/renderer/set_icon_natives.cc
index 192112f..4fcec19 100644
--- a/extensions/renderer/set_icon_natives.cc
+++ b/extensions/renderer/set_icon_natives.cc
@@ -11,6 +11,7 @@
 #include "extensions/renderer/request_sender.h"
 #include "extensions/renderer/script_context.h"
 #include "ipc/ipc_message_utils.h"
+#include "third_party/WebKit/public/web/WebArrayBufferConverter.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/gfx/ipc/gfx_param_traits.h"
 
@@ -35,7 +36,7 @@ SetIconNatives::SetIconNatives(RequestSender* request_sender,
 
 bool SetIconNatives::ConvertImageDataToBitmapValue(
     const v8::Local<v8::Object> image_data,
-    base::Value** bitmap_value) {
+    v8::Local<v8::Value>* image_data_bitmap) {
   v8::Isolate* isolate = context()->v8_context()->GetIsolate();
   v8::Local<v8::Object> data =
       image_data->Get(v8::String::NewFromUtf8(isolate, "data"))->ToObject();
@@ -92,72 +93,57 @@ bool SetIconNatives::ConvertImageDataToBitmapValue(
   // Construct the Value object.
   IPC::Message bitmap_pickle;
   IPC::WriteParam(&bitmap_pickle, bitmap);
-  *bitmap_value = base::BinaryValue::CreateWithCopiedBuffer(
-      static_cast<const char*>(bitmap_pickle.data()), bitmap_pickle.size());
+  blink::WebArrayBuffer buffer =
+      blink::WebArrayBuffer::create(bitmap_pickle.size(), 1);
+  memcpy(buffer.data(), bitmap_pickle.data(), bitmap_pickle.size());
+  *image_data_bitmap = blink::WebArrayBufferConverter::toV8Value(
+      &buffer, context()->v8_context()->Global(), isolate);
 
   return true;
 }
 
 bool SetIconNatives::ConvertImageDataSetToBitmapValueSet(
-    const v8::FunctionCallbackInfo<v8::Value>& args,
-    base::DictionaryValue* bitmap_set_value) {
-  v8::Local<v8::Object> extension_args = args[1]->ToObject();
-  v8::Local<v8::Object> details =
-      extension_args->Get(v8::String::NewFromUtf8(args.GetIsolate(), "0"))
-          ->ToObject();
+    v8::Local<v8::Object>& details,
+    v8::Local<v8::Object>* bitmap_set_value) {
+  v8::Isolate* isolate = context()->v8_context()->GetIsolate();
   v8::Local<v8::Object> image_data_set =
-      details->Get(v8::String::NewFromUtf8(args.GetIsolate(), "imageData"))
-          ->ToObject();
+      details->Get(v8::String::NewFromUtf8(isolate, "imageData"))->ToObject();
 
   DCHECK(bitmap_set_value);
   for (size_t i = 0; i < arraysize(kImageSizeKeys); i++) {
     if (!image_data_set->Has(
-            v8::String::NewFromUtf8(args.GetIsolate(), kImageSizeKeys[i])))
+            v8::String::NewFromUtf8(isolate, kImageSizeKeys[i])))
       continue;
     v8::Local<v8::Object> image_data =
-        image_data_set->Get(v8::String::NewFromUtf8(args.GetIsolate(),
-                                                    kImageSizeKeys[i]))
+        image_data_set->Get(v8::String::NewFromUtf8(isolate, kImageSizeKeys[i]))
             ->ToObject();
-    base::Value* image_data_bitmap = NULL;
+    v8::Local<v8::Value> image_data_bitmap;
     if (!ConvertImageDataToBitmapValue(image_data, &image_data_bitmap))
       return false;
-    bitmap_set_value->Set(kImageSizeKeys[i], image_data_bitmap);
+    (*bitmap_set_value)->Set(
+        v8::String::NewFromUtf8(isolate, kImageSizeKeys[i]), image_data_bitmap);
   }
   return true;
 }
 
 void SetIconNatives::SetIconCommon(
     const v8::FunctionCallbackInfo<v8::Value>& args) {
-  scoped_ptr<base::DictionaryValue> bitmap_set_value(
-      new base::DictionaryValue());
-  if (!ConvertImageDataSetToBitmapValueSet(args, bitmap_set_value.get()))
+  CHECK_EQ(1, args.Length());
+  CHECK(args[0]->IsObject());
+  v8::Local<v8::Object> details = args[0]->ToObject();
+  v8::Local<v8::Object> bitmap_set_value(v8::Object::New(args.GetIsolate()));
+  if (!ConvertImageDataSetToBitmapValueSet(details, &bitmap_set_value))
     return;
 
-  v8::Local<v8::Object> extension_args = args[1]->ToObject();
-  v8::Local<v8::Object> details =
-      extension_args->Get(v8::String::NewFromUtf8(args.GetIsolate(), "0"))
-          ->ToObject();
-
-  base::DictionaryValue* dict = new base::DictionaryValue();
-  dict->Set("imageData", bitmap_set_value.release());
-
+  v8::Local<v8::Object> dict(v8::Object::New(args.GetIsolate()));
+  dict->Set(v8::String::NewFromUtf8(args.GetIsolate(), "imageData"),
+            bitmap_set_value);
   if (details->Has(v8::String::NewFromUtf8(args.GetIsolate(), "tabId"))) {
-    dict->SetInteger(
-        "tabId",
-        details->Get(v8::String::NewFromUtf8(args.GetIsolate(), "tabId"))
-            ->Int32Value());
+    dict->Set(
+        v8::String::NewFromUtf8(args.GetIsolate(), "tabId"),
+        details->Get(v8::String::NewFromUtf8(args.GetIsolate(), "tabId")));
   }
-
-  base::ListValue list_value;
-  list_value.Append(dict);
-
-  std::string name = *v8::String::Utf8Value(args[0]);
-  int request_id = args[2]->Int32Value();
-  bool has_callback = args[3]->BooleanValue();
-  bool for_io_thread = args[4]->BooleanValue();
-
-  request_sender_->StartRequest(
-      context(), name, request_id, has_callback, for_io_thread, &list_value);
+  args.GetReturnValue().Set(dict);
 }
 
 }  // namespace extensions
diff --git a/extensions/renderer/set_icon_natives.h b/extensions/renderer/set_icon_natives.h
index e5129dd..0def0be 100644
--- a/extensions/renderer/set_icon_natives.h
+++ b/extensions/renderer/set_icon_natives.h
@@ -24,10 +24,10 @@ class SetIconNatives : public ObjectBackedNativeHandler {
 
  private:
   bool ConvertImageDataToBitmapValue(const v8::Local<v8::Object> image_data,
-                                     base::Value** bitmap_value);
+                                     v8::Local<v8::Value>* image_data_bitmap);
   bool ConvertImageDataSetToBitmapValueSet(
-      const v8::FunctionCallbackInfo<v8::Value>& args,
-      base::DictionaryValue* bitmap_value);
+      v8::Local<v8::Object>& details,
+      v8::Local<v8::Object>* bitmap_set_value);
   void SetIconCommon(const v8::FunctionCallbackInfo<v8::Value>& args);
 
   RequestSender* request_sender_;