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

Issue 5426447338438656: Fix icon animation (Closed)

Created:
Feb. 27, 2014, 2:38 p.m. by Sebastian Noack
Modified:
Feb. 27, 2014, 7:32 p.m.
Visibility:
Public.

Patch Set 1 #

Total comments: 2

Patch Set 2 : #

Patch Set 3 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+4 lines, -8 lines) Patch
M background.js View 1 2 2 chunks +3 lines, -7 lines 0 comments Download
M iconAnimation.js View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 4
Sebastian Noack
http://codereview.adblockplus.org/5426447338438656/diff/5629499534213120/background.js File background.js (left): http://codereview.adblockplus.org/5426447338438656/diff/5629499534213120/background.js#oldcode107 background.js:107: return; This relic from the time were we used ...
Feb. 27, 2014, 2:41 p.m. (2014-02-27 14:41:47 UTC) #1
Felix Dahlke
LGTM!
Feb. 27, 2014, 2:45 p.m. (2014-02-27 14:45:27 UTC) #2
Sebastian Noack
As discussed, instead of dropping the /^https?:/ check, I used it to properly show a ...
Feb. 27, 2014, 4:23 p.m. (2014-02-27 16:23:54 UTC) #3
Sebastian Noack
Feb. 27, 2014, 7:18 p.m. (2014-02-27 19:18:15 UTC) #4
I changed my mind regarding showing a disabled icon on pages with file://,
chrome[-extension]:// and other special URLs. I had to admit that it is very
distracting to have the icon change every time you open a new tab. And I can
imagine that it might confuse users. Also this would be inconsistent with
Firefox. However I still hide the context menu entry on those URLs.

Powered by Google App Engine
This is Rietveld