Fixes 4218 - setIcon for older, fussy versions of Chrome

Fixes 4218 - setIcon for older, fussy versions of Chrome

[kzar, 2016-08-15 20:18:22]

Patch Set 1

[Sebastian Noack, 2016-08-15 21:22:16]

https://codereview.adblockplus.org/29349820/diff/29349821/chrome/ext/backgrou...
File chrome/ext/background.js (right):

https://codereview.adblockplus.org/29349820/diff/29349821/chrome/ext/backgrou...
chrome/ext/background.js:172: var supportedIconSizes = ["19", "38", "16", "32",
"20", "40"];
Nit: Preserve the blank line.

https://codereview.adblockplus.org/29349820/diff/29349821/chrome/ext/backgrou...
chrome/ext/background.js:182: var iconDetails = function()
For consistency with the surrounding code, make it an auxiliary method rather
than an anonymous function.

[Sebastian Noack, 2016-08-15 22:13:05]

https://codereview.adblockplus.org/29349820/diff/29349821/chrome/ext/backgrou...
File chrome/ext/background.js (right):

https://codereview.adblockplus.org/29349820/diff/29349821/chrome/ext/backgrou...
chrome/ext/background.js:199: throw(e);
This branch is redundant. It's not that we would enter a recursive condition
otherwise.

[kzar, 2016-08-16 08:33:41]

Patch Set 2 : Addressed feedback

https://codereview.adblockplus.org/29349820/diff/29349821/chrome/ext/backgrou...
File chrome/ext/background.js (right):

https://codereview.adblockplus.org/29349820/diff/29349821/chrome/ext/backgrou...
chrome/ext/background.js:172: var supportedIconSizes = ["19", "38", "16", "32",
"20", "40"];
On 2016/08/15 21:22:16, Sebastian Noack wrote:
> Nit: Preserve the blank line.

Done.

https://codereview.adblockplus.org/29349820/diff/29349821/chrome/ext/backgrou...
chrome/ext/background.js:182: var iconDetails = function()
On 2016/08/15 21:22:16, Sebastian Noack wrote:
> For consistency with the surrounding code, make it an auxiliary method rather
> than an anonymous function.

Done.

https://codereview.adblockplus.org/29349820/diff/29349821/chrome/ext/backgrou...
chrome/ext/background.js:199: throw(e);
On 2016/08/15 22:13:05, Sebastian Noack wrote:
> This branch is redundant. It's not that we would enter a recursive condition
> otherwise.

Oh yea, it would just be called twice but who cares.

[Sebastian Noack, 2016-08-16 08:39:09]

LGTM

[Sebastian Noack, 2016-08-16 08:44:02]

Mh wait, what is about the icon animation? We call
chrome.browserAction.setIcon({imageData: ..}) directly there?

[kzar, 2016-08-16 08:46:53]

On 2016/08/16 08:44:02, Sebastian Noack wrote:
> Mh wait, what is about the icon animation? We call
> chrome.browserAction.setIcon({imageData: ..}) directly there?

God damnit, you're right... Sorry pushed just before I noticed your message.
Will fix now.

[kzar, 2016-08-16 09:47:43]

Patch Set 3 : Fix icon animations too!

Well I'm not too happy with this change, open to suggestions about how to make
it less ugly. It does work though.

[kzar, 2016-08-16 11:25:12]

Patch Set 4 : Removed stray semicolon

[Sebastian Noack, 2016-08-16 11:58:25]

I think I found a better way. All you need is following two helper methods:

    _legacySetIcon: function(details)
    {
      var newDetails = {};
      for (var key in details)
      {
        var value = details[key];
        if (typeof value == "object")
          value = {19: value[19], 38: value[38]};
        newDetails[key] = value;
      }
      chrome.browserAction.setIcon(newDetails);
    },
    _safeSetIcon: function(details)
    {
      try
      {
        chrome.browserAction.setIcon(details);
      }
      catch (e)
      {
        this._safeSetIcon = this._legacySetIcon;
        this._safeSetIcon(details);
      }
    }

And then just use _safeSetIcon() everywhere, where we used
chrome.browserAction.setIcon() previously, i.e. in _applyChanges() and in the
animation code.

[kzar, 2016-08-16 12:40:44]

Patch Set 5 : Go with Sebastian's approach

Yea, I like that idea much better. Done.

[Sebastian Noack, 2016-08-16 16:49:12]

https://codereview.adblockplus.org/29349820/diff/29349864/chrome/ext/backgrou...
File chrome/ext/background.js (right):

https://codereview.adblockplus.org/29349820/diff/29349864/chrome/ext/backgrou...
chrome/ext/background.js:184: if (details.hasOwnProperty(key))
Is this case worth to be considered? As far as I can tell we always pass a plain
object, and there is arguable no reason to pass any thing else, and in that case
each property we get here is an own property.

https://codereview.adblockplus.org/29349820/diff/29349864/chrome/ext/backgrou...
chrome/ext/background.js:213: var details = {tabId: this._tabId, path: {}};
This change here isn't even necessary, and while it avoids duplication, it makes
the code hardly any easier to understand. I think the original declarative
approach was nicer.

[kzar, 2016-08-16 16:59:17]

Patch Set 6 : Addressed feedback

https://codereview.adblockplus.org/29349820/diff/29349864/chrome/ext/backgrou...
File chrome/ext/background.js (right):

https://codereview.adblockplus.org/29349820/diff/29349864/chrome/ext/backgrou...
chrome/ext/background.js:184: if (details.hasOwnProperty(key))
On 2016/08/16 16:49:11, Sebastian Noack wrote:
> Is this case worth to be considered? As far as I can tell we always pass a
plain
> object, and there is arguable no reason to pass any thing else, and in that
case
> each property we get here is an own property.

Done.

https://codereview.adblockplus.org/29349820/diff/29349864/chrome/ext/backgrou...
chrome/ext/background.js:213: var details = {tabId: this._tabId, path: {}};
On 2016/08/16 16:49:11, Sebastian Noack wrote:
> This change here isn't even necessary, and while it avoids duplication, it
makes
> the code hardly any easier to understand. I think the original declarative
> approach was nicer.

Done.

[Sebastian Noack, 2016-08-16 17:11:04]

LGTM