Fix the dynamic browser action setIcon path to work with any size icon.

BUG=564926,575256
TBR=benwells@chromium.org

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

Cr-Commit-Position: refs/heads/master@{#371133}

2 files changed, 45 insertions, 67 deletions

diff --git a/extensions/renderer/resources/set_icon.js b/extensions/renderer/resources/set_icon.js
index 5b66d6b..b0ed2b5 100644
--- a/extensions/renderer/resources/set_icon.js
+++ b/extensions/renderer/resources/set_icon.js
@@ -5,15 +5,15 @@
 var SetIconCommon = requireNative('setIcon').SetIconCommon;
 var sendRequest = require('sendRequest').sendRequest;
 
-function loadImagePath(path, iconSize, callback) {
+function loadImagePath(path, callback) {
   var img = new Image();
   img.onerror = function() {
     console.error('Could not load action icon \'' + path + '\'.');
   };
   img.onload = function() {
     var canvas = document.createElement('canvas');
-    canvas.width = img.width > iconSize ? iconSize : img.width;
-    canvas.height = img.height > iconSize ? iconSize : img.height;
+    canvas.width = img.width;
+    canvas.height = img.height;
 
     var canvas_context = canvas.getContext('2d');
     canvas_context.clearRect(0, 0, canvas.width, canvas.height);
@@ -25,27 +25,23 @@ function loadImagePath(path, iconSize, callback) {
   img.src = path;
 }
 
-function verifyImageData(imageData, iconSize) {
-  // Verify that this at least looks like an ImageData element.
+function smellsLikeImageData(imageData) {
+  // See if this object at least looks like an ImageData element.
   // Unfortunately, we cannot use instanceof because the ImageData
   // constructor is not public.
   //
   // We do this manually instead of using JSONSchema to avoid having these
   // properties show up in the doc.
-  if (!('width' in imageData) ||
-      !('height' in imageData) ||
-      !('data' in imageData)) {
+  return (typeof imageData == 'object') && ('width' in imageData) &&
+         ('height' in imageData) && ('data' in imageData);
+}
+
+function verifyImageData(imageData) {
+  if (!smellsLikeImageData(imageData)) {
     throw new Error(
         'The imageData property must contain an ImageData object or' +
         ' dictionary of ImageData objects.');
   }
-
-  if (imageData.width > iconSize ||
-      imageData.height > iconSize) {
-    throw new Error(
-        'The imageData property must contain an ImageData object that ' +
-        'is no larger than ' + iconSize + ' pixels square.');
-  }
 }
 
 /**
@@ -59,7 +55,6 @@ function verifyImageData(imageData, iconSize) {
  *   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
@@ -70,24 +65,19 @@ function setIcon(details, callback) {
   }
 
   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) {
-      // If details.imageData is not dictionary with keys in set {'19', '38'},
-      // it must be an ImageData object.
-      var sizeKey = iconSizes[0].toString();
+    if (smellsLikeImageData(details.imageData)) {
       var imageData = details.imageData;
       details.imageData = {};
-      details.imageData[sizeKey] = imageData;
-      verifyImageData(details.imageData[sizeKey], iconSizes[0]);
+      details.imageData[imageData.width.toString()] = imageData;
+    } else if (typeof details.imageData == 'object' &&
+               Object.getOwnPropertyNames(details.imageData).length !== 0) {
+      for (var sizeKey in details.imageData) {
+        verifyImageData(details.imageData[sizeKey]);
+      }
+    } else {
+      verifyImageData(false);
     }
+
     callback(SetIconCommon(details));
     return;
   }
@@ -95,37 +85,23 @@ function setIcon(details, callback) {
   if ('path' in details) {
     if (typeof details.path == 'object') {
       details.imageData = {};
-      var isEmpty = true;
-      var processIconSize = function(index) {
-        if (index == iconSizes.length) {
-          delete details.path;
-          if (isEmpty)
-            throw new Error('The path property must not be empty.');
-          callback(SetIconCommon(details));
-          return;
-        }
-        var sizeKey = iconSizes[index].toString();
-        if (!(sizeKey in details.path)) {
-          processIconSize(index + 1);
-          return;
-        }
-        isEmpty = false;
-        loadImagePath(details.path[sizeKey], iconSizes[index],
-          function(imageData) {
-            details.imageData[sizeKey] = imageData;
-            processIconSize(index + 1);
-          });
+      var detailKeyCount = 0;
+      for (var iconSize in details.path) {
+        ++detailKeyCount;
+        loadImagePath(details.path[iconSize], function(size, imageData) {
+          details.imageData[size] = imageData;
+          if (--detailKeyCount == 0)
+            callback(SetIconCommon(details));
+        }.bind(null, iconSize));
       }
-      processIconSize(0);
+      if (detailKeyCount == 0)
+        throw new Error('The path property must not be empty.');
     } else if (typeof details.path == 'string') {
-      var sizeKey = iconSizes[0].toString();
       details.imageData = {};
-      loadImagePath(details.path, iconSizes[0],
-          function(imageData) {
-            details.imageData[sizeKey] = imageData;
-            delete details.path;
-            callback(SetIconCommon(details));
-            return;
+      loadImagePath(details.path, function(imageData) {
+        details.imageData[imageData.width.toString()] = imageData;
+        delete details.path;
+        callback(SetIconCommon(details));
       });
     }
     return;
diff --git a/extensions/renderer/set_icon_natives.cc b/extensions/renderer/set_icon_natives.cc
index 426acfe..2b6b097 100644
--- a/extensions/renderer/set_icon_natives.cc
+++ b/extensions/renderer/set_icon_natives.cc
@@ -11,6 +11,7 @@
 
 #include "base/macros.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/strings/string_number_conversions.h"
 #include "content/public/common/common_param_traits.h"
 #include "extensions/renderer/request_sender.h"
 #include "extensions/renderer/script_context.h"
@@ -21,7 +22,6 @@
 
 namespace {
 
-const char* kImageSizeKeys[] = {"19", "38"};
 const char kInvalidDimensions[] = "ImageData has invalid dimensions.";
 const char kInvalidData[] = "ImageData data length does not match dimensions.";
 const char kNoMemory[] = "Chrome was unable to initialize icon.";
@@ -115,18 +115,20 @@ bool SetIconNatives::ConvertImageDataSetToBitmapValueSet(
           ->ToObject(isolate);
 
   DCHECK(bitmap_set_value);
-  for (size_t i = 0; i < arraysize(kImageSizeKeys); i++) {
-    if (!image_data_set->Has(
-            v8::String::NewFromUtf8(isolate, kImageSizeKeys[i])))
+
+  v8::Local<v8::Array> property_names(image_data_set->GetOwnPropertyNames());
+  for (size_t i = 0; i < property_names->Length(); ++i) {
+    v8::Local<v8::Value> key(property_names->Get(i));
+    v8::String::Utf8Value utf8_key(key);
+    int size;
+    if (!base::StringToInt(std::string(*utf8_key), &size))
       continue;
     v8::Local<v8::Object> image_data =
-        image_data_set->Get(v8::String::NewFromUtf8(isolate, kImageSizeKeys[i]))
-            ->ToObject(isolate);
+        image_data_set->Get(key)->ToObject(isolate);
     v8::Local<v8::Value> image_data_bitmap;
     if (!ConvertImageDataToBitmapValue(image_data, &image_data_bitmap))
       return false;
-    (*bitmap_set_value)->Set(
-        v8::String::NewFromUtf8(isolate, kImageSizeKeys[i]), image_data_bitmap);
+    (*bitmap_set_value)->Set(key, image_data_bitmap);
   }
   return true;
 }