Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(459)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 5 months ago by kzar
Modified:
3 years, 5 months ago
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
3 years, 5 months ago (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"]; ...
3 years, 5 months ago (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 ...
3 years, 5 months ago (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 = ...
3 years, 5 months ago (2016-08-16 08:33:41 UTC) #4
Sebastian Noack
LGTM
3 years, 5 months ago (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?
3 years, 5 months ago (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? ...
3 years, 5 months ago (2016-08-16 08:46:53 UTC) #7
kzar
Patch Set 3 : Fix icon animations too! Well I'm not too happy with this ...
3 years, 5 months ago (2016-08-16 09:47:43 UTC) #8
kzar
Patch Set 4 : Removed stray semicolon
3 years, 5 months ago (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: ...
3 years, 5 months ago (2016-08-16 11:58:25 UTC) #10
kzar
Patch Set 5 : Go with Sebastian's approach Yea, I like that idea much better. ...
3 years, 5 months ago (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? ...
3 years, 5 months ago (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 ...
3 years, 5 months ago (2016-08-16 16:59:17 UTC) #13
Sebastian Noack
3 years, 5 months ago (2016-08-16 17:11:04 UTC) #14
LGTM
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5