Refactor setIcon to allow for more general use of imageData

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 883fd673997f849eaef0dc3514f6291131dadfb6..3b68ae6285975f0d4985cb7d6d2a788747792166 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,44 @@ 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.

[not at google - send to devlin, 2014/08/20 14:42:18]

There is another parameter here, |callback|. Document that.

Also mention that the callback maybe called reentrantly. This is actually a
shame, and if this were new code I'd say that we should ensure it's not
re-entrant, BUT it's not worth it given it's old code. It works just fine the
way it is.

Just document it.

[gpdavis, 2014/08/20 17:58:18]

Could you clarify what you mean by reentrantly, exactly?  Is this because the
callback has calls throughout the function, even though they're all followed by
returns (which makes it okay)?  Just want to make sure I understand this instead
of just saying okay and doing it :)

[not at google - send to devlin, 2014/08/20 18:20:29]

Reentrant is the difference between:

foo();
[1,2,3].forEach(bar);
baz();

and

foo();
setTimeout(bar);
baz();

In the first example foo() will be called, then bar() (3 times), then baz. This
is reentrant.
In the second example foo() will be called, then baz(), then at some point in
the future bar(). This is not reentrant. It's *never* possible to call bar()
before baz().

Mixing an API between reentrant callbacks and non-reentrant callbacks is
confusing and can lead to subtle bugs, e.g. what if instead of the above we had:

foo();
doSomething(bar);
baz();

and doSomething can be either reentrant or not, it depends on the input or state
of the world or whatever. That has undefined behaviour, since baz *may* be
called after bar, or *may* not be. Hard to program for. This isn't just
speculation, I've seen some really tricky bugs caused by it.

You can fix that without too much effort, but like I said, not worth it. But
worth commenting on.

[gpdavis, 2014/08/20 18:27:18]

Ahh, okay, I think I understand now.  So we're comfortable with the way things
are mostly because, like I said before, we always return immediately after the
callback, so the unpredictability of reentrancy is unlikely to be problematic?

+ */
+function setIcon(details, callback) {
   var iconSizes = [19, 38];
   if ('iconIndex' in details) {

[not at google - send to devlin, 2014/08/20 14:42:18]

Could you add a comment here:

// 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.

[gpdavis, 2014/08/20 17:58:18]

Done.

-    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 +94,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 +103,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;