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

extensions/renderer/resources/set_icon.js

Index: extensions/renderer/resources/set_icon.js
diff --git a/extensions/renderer/resources/set_icon.js b/extensions/renderer/resources/set_icon.js
index 5b66d6b35970140a4c57c2e39376385b1a3a93a5..c8fa4ac6ef12cca46a2dd3019a370afe7f4dd7e8 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 ' +

[Evan Stade, 2016/01/13 18:35:41]

I guess we could keep this check, I'm not sure what good it's doing though.

[Devlin, 2016/01/13 19:58:12]

Well, if we can scale, it's totally irrelevant, right?

[Evan Stade, 2016/01/14 01:59:19]

I'm assuming we'd change this to width/height == iconSize. All we would be
enforcing is that { "21": someImageData } is consistent (someImageData is
actually 21x21). Now we don't have to enforce this, but it's not totally
irrelevant: most likely if it trips it's catching a developer's mistake. There's
no reason I can think of to bother specifying a size that's not correct, as
opposed to omitting that size or correcting the key.

But if we tighten the verification to exact equality, that could break some
existing extensions I guess. :[

-        '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,25 @@ 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 = Object.getOwnPropertyNames(details.path).length;

[Devlin, 2016/01/13 19:58:12]

nit: getOwnPropertyNames wouldn't allow for inherited properties, whereas the
for-in loops do.  This probably isn't a big deal (I don't know how many devs, if
any, would use inheritance for setIcon arguments), but it might be nice to be
consistent (and lean towards just allowing all enumerable properties).

[Evan Stade, 2016/01/14 01:59:19]

Done.

+      if (detailKeyCount == 0)
+        throw new Error('The path property must not be empty.');
+      for (var iconSize in details.path) {
+        loadImagePath(details.path[iconSize], function(size, imageData) {
+          details.imageData[size] = imageData;
+          if (--detailKeyCount == 0) {

[Devlin, 2016/01/13 19:58:12]

This particular part would be more elegant with Promise.all(), but means
converting some stuff to promises.  I'll leave it up to you if you want to do
it.

[Evan Stade, 2016/01/14 01:59:19]

Tempting, but I'll pass for now. :)

+            callback(SetIconCommon(details));
+            return;
+          }
+        }.bind(null, iconSize));

[Devlin, 2016/01/13 19:58:12]

do we need this?

[Evan Stade, 2016/01/14 01:59:19]

Believe me, it didn't occur to me to add this and it caused much anguish until I
realized the problem. Without it, we depend on iconSize inside the inlined
function, but by the time the first load comes back, iconSize has already been
changed to a later key. js is fun.

[Devlin, 2016/01/19 22:44:40]

Wow.

       }
-      processIconSize(0);
     } 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;
       });
     }
     return;