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

extensions/renderer/resources/set_icon.js

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 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);
     canvas_context.drawImage(img, 0, 0, canvas.width, canvas.height);
     var imageData = canvas_context.getImageData(0, 0, canvas.width,
                                                 canvas.height);
     callback(imageData);
   };
   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.');
-  }
 }
 
 /**
  * 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) {
     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 (smellsLikeImageData(details.imageData)) {
+      var imageData = details.imageData;
+      details.imageData = {};
+      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);
     }
 
-    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]);
-    }
     callback(SetIconCommon(details));
     return;
   }
 
   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);
-          });
-      }
-      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;
+      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.

+      }
+    } else if (typeof details.path == 'string') {
+      details.imageData = {};
+      loadImagePath(details.path, function(imageData) {
+        details.imageData[imageData.width.toString()] = imageData;
+        delete details.path;
+        callback(SetIconCommon(details));
+        return;
       });
     }
     return;
   }
   throw new Error('Either the path or imageData property must be specified.');
 }
 
 exports.$set('setIcon', setIcon);