Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29349820: Fixes 4218 - setIcon for older, fussy versions of Chrome (Closed)

Created:
Aug. 15, 2016, 8:17 p.m. by kzar
Modified:
Aug. 16, 2016, 5:20 p.m.
Reviewers:
Sebastian Noack
Visibility:
Public.

Description

Fixes 4218 - setIcon for older, fussy versions of Chrome

Patch Set 1 #

Total comments: 6

Patch Set 2 : Addressed feedback #

Patch Set 3 : Fix icon animations too! #

Patch Set 4 : Removed stray semicolon #

Patch Set 5 : Go with Sebastian's approach #

Total comments: 4

Patch Set 6 : Addressed feedback #

Unified diffs Side-by-side diffs Delta from patch set Stats (+29 lines, -2 lines) Patch
M chrome/ext/background.js View 1 2 3 4 5 1 chunk +28 lines, -1 line 0 comments Download
M lib/icon.js View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 14
kzar
Patch Set 1
Aug. 15, 2016, 8:18 p.m. (2016-08-15 20:18:22 UTC) #1
Sebastian Noack
https://codereview.adblockplus.org/29349820/diff/29349821/chrome/ext/background.js File chrome/ext/background.js (right): https://codereview.adblockplus.org/29349820/diff/29349821/chrome/ext/background.js#newcode172 chrome/ext/background.js:172: var supportedIconSizes = ["19", "38", "16", "32", "20", "40"]; ...
Aug. 15, 2016, 9:22 p.m. (2016-08-15 21:22:16 UTC) #2
Sebastian Noack
https://codereview.adblockplus.org/29349820/diff/29349821/chrome/ext/background.js File chrome/ext/background.js (right): https://codereview.adblockplus.org/29349820/diff/29349821/chrome/ext/background.js#newcode199 chrome/ext/background.js:199: throw(e); This branch is redundant. It's not that we ...
Aug. 15, 2016, 10:13 p.m. (2016-08-15 22:13:05 UTC) #3
kzar
Patch Set 2 : Addressed feedback https://codereview.adblockplus.org/29349820/diff/29349821/chrome/ext/background.js File chrome/ext/background.js (right): https://codereview.adblockplus.org/29349820/diff/29349821/chrome/ext/background.js#newcode172 chrome/ext/background.js:172: var supportedIconSizes = ...
Aug. 16, 2016, 8:33 a.m. (2016-08-16 08:33:41 UTC) #4
Sebastian Noack
LGTM
Aug. 16, 2016, 8:39 a.m. (2016-08-16 08:39:09 UTC) #5
Sebastian Noack
Mh wait, what is about the icon animation? We call chrome.browserAction.setIcon({imageData: ..}) directly there?
Aug. 16, 2016, 8:44 a.m. (2016-08-16 08:44:02 UTC) #6
kzar
On 2016/08/16 08:44:02, Sebastian Noack wrote: > Mh wait, what is about the icon animation? ...
Aug. 16, 2016, 8:46 a.m. (2016-08-16 08:46:53 UTC) #7
kzar
Patch Set 3 : Fix icon animations too! Well I'm not too happy with this ...
Aug. 16, 2016, 9:47 a.m. (2016-08-16 09:47:43 UTC) #8
kzar
Patch Set 4 : Removed stray semicolon
Aug. 16, 2016, 11:25 a.m. (2016-08-16 11:25:12 UTC) #9
Sebastian Noack
I think I found a better way. All you need is following two helper methods: ...
Aug. 16, 2016, 11:58 a.m. (2016-08-16 11:58:25 UTC) #10
kzar
Patch Set 5 : Go with Sebastian's approach Yea, I like that idea much better. ...
Aug. 16, 2016, 12:40 p.m. (2016-08-16 12:40:44 UTC) #11
Sebastian Noack
https://codereview.adblockplus.org/29349820/diff/29349864/chrome/ext/background.js File chrome/ext/background.js (right): https://codereview.adblockplus.org/29349820/diff/29349864/chrome/ext/background.js#newcode184 chrome/ext/background.js:184: if (details.hasOwnProperty(key)) Is this case worth to be considered? ...
Aug. 16, 2016, 4:49 p.m. (2016-08-16 16:49:12 UTC) #12
kzar
Patch Set 6 : Addressed feedback https://codereview.adblockplus.org/29349820/diff/29349864/chrome/ext/background.js File chrome/ext/background.js (right): https://codereview.adblockplus.org/29349820/diff/29349864/chrome/ext/background.js#newcode184 chrome/ext/background.js:184: if (details.hasOwnProperty(key)) On ...
Aug. 16, 2016, 4:59 p.m. (2016-08-16 16:59:17 UTC) #13
Sebastian Noack
Aug. 16, 2016, 5:11 p.m. (2016-08-16 17:11:04 UTC) #14
LGTM

Powered by Google App Engine
This is Rietveld