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

Issue 29334223: Issue 3532 - Generate animation images at runtime (Closed)

Created:
Jan. 21, 2016, 5:35 p.m. by kzar
Modified:
Jan. 26, 2016, 11:17 p.m.
Reviewers:
Sebastian Noack
Visibility:
Public.

Description

Issue 3532 - Generate animation images at runtime I used Felix's earlier work as a reference https://github.com/adblockplus/adblockpluschrome/commit/df0d21168f12006e272332dbe3ca2f643bdb8dba

Patch Set 1 #

Total comments: 12

Patch Set 2 : Addressed initial feedback #

Patch Set 3 : Avoid warnings when closing tabs during animation #

Patch Set 4 : Don't bother generate Safari icons at build time #

Total comments: 12

Patch Set 5 : Addressed further feedback #

Total comments: 22

Patch Set 6 : Addressed more feedback #

Patch Set 7 : Rebased #

Total comments: 19

Patch Set 8 : Addressed some more feedback #

Patch Set 9 : Handle tab switching / closing during animations #

Patch Set 10 : Pre render frames, listen for onactivated #

Total comments: 2

Patch Set 11 : Avoid exceptions when active tab is closed #

Patch Set 12 : Fixed bug in animationStep counter inc #

Total comments: 29

Patch Set 13 : Addressed more feedback! #

Total comments: 9

Patch Set 14 : We don't need to convert icons to data URLs #

Patch Set 15 : Iterate over size in inner loop #

Total comments: 2

Patch Set 16 : Removed NOOP callback, renamed "greyed" variable #

Patch Set 17 : Fix onActivated dispatch for Safari #

Patch Set 18 : Remove stopAnimation closure and hard coded opacity range #

Total comments: 12

Patch Set 19 : Addressed final nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+165 lines, -222 lines) Patch
M chrome/ext/background.js View 1 2 3 4 5 6 7 8 9 2 chunks +7 lines, -1 line 0 comments Download
M lib/icon.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +130 lines, -63 lines 0 comments Download
M lib/notificationHelper.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +13 lines, -37 lines 0 comments Download
M metadata.chrome View 1 chunk +1 line, -75 lines 0 comments Download
M metadata.safari View 1 2 3 4 2 chunks +7 lines, -44 lines 0 comments Download
M safari/ext/background.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +7 lines, -2 lines 0 comments Download
A safari/icons/abp-16.png View 1 2 3 Binary file 0 comments Download
M safari/icons/abp-16-notification-critical.png View 1 2 3 Binary file 0 comments Download
M safari/icons/abp-16-notification-information.png View 1 2 3 Binary file 0 comments Download
A safari/icons/abp-32.png View 1 2 3 Binary file 0 comments Download
M safari/icons/abp-32-notification-critical.png View 1 2 3 Binary file 0 comments Download
M safari/icons/abp-32-notification-information.png View 1 2 3 Binary file 0 comments Download

Messages

Total messages: 30
kzar
Patch Set 1
Jan. 21, 2016, 5:37 p.m. (2016-01-21 17:37:36 UTC) #1
Sebastian Noack
I didn't finish reviewing these changes yet. But some comments so far. https://codereview.adblockplus.org/29334223/diff/29334224/background.js File background.js ...
Jan. 21, 2016, 6:11 p.m. (2016-01-21 18:11:02 UTC) #2
kzar
Patch Set 2 : Addressed initial feedback https://codereview.adblockplus.org/29334223/diff/29334224/background.js File background.js (right): https://codereview.adblockplus.org/29334223/diff/29334224/background.js#newcode130 background.js:130: updateIcon(page, whitelisted ...
Jan. 21, 2016, 9:07 p.m. (2016-01-21 21:07:30 UTC) #3
kzar
Patch Set 3 : Avoid warnings when closing tabs during animation
Jan. 21, 2016, 10:29 p.m. (2016-01-21 22:29:28 UTC) #4
kzar
Patch Set 4 : Don't bother generate Safari icons at build time
Jan. 21, 2016, 10:47 p.m. (2016-01-21 22:47:42 UTC) #5
Sebastian Noack
https://codereview.adblockplus.org/29334223/diff/29334224/background.js File background.js (right): https://codereview.adblockplus.org/29334223/diff/29334224/background.js#newcode130 background.js:130: updateIcon(page, whitelisted && true || false); On 2016/01/21 21:07:30, ...
Jan. 22, 2016, 3:59 p.m. (2016-01-22 15:59:16 UTC) #6
kzar
Patch Set 5 : Addressed further feedback https://codereview.adblockplus.org/29334223/diff/29334224/background.js File background.js (right): https://codereview.adblockplus.org/29334223/diff/29334224/background.js#newcode130 background.js:130: updateIcon(page, whitelisted ...
Jan. 22, 2016, 4:49 p.m. (2016-01-22 16:49:36 UTC) #7
Sebastian Noack
https://codereview.adblockplus.org/29334223/diff/29334375/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29334223/diff/29334375/lib/icon.js#newcode33 lib/icon.js:33: * Loads an image and draws it onto a ...
Jan. 23, 2016, 2:04 p.m. (2016-01-23 14:04:11 UTC) #8
Sebastian Noack
https://codereview.adblockplus.org/29334223/diff/29334375/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29334223/diff/29334375/lib/icon.js#newcode58 lib/icon.js:58: image.addEventListener("error", function(error) { reject(error); }); Note that the value ...
Jan. 23, 2016, 2:17 p.m. (2016-01-23 14:17:42 UTC) #9
kzar
Patch Set 6 : Addressed more feedback https://codereview.adblockplus.org/29334223/diff/29334375/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29334223/diff/29334375/lib/icon.js#newcode33 lib/icon.js:33: * Loads ...
Jan. 23, 2016, 2:43 p.m. (2016-01-23 14:43:15 UTC) #10
kzar
Patch Set 7 : Rebased
Jan. 23, 2016, 3:11 p.m. (2016-01-23 15:11:07 UTC) #11
Sebastian Noack
https://codereview.adblockplus.org/29334223/diff/29334419/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29334223/diff/29334419/lib/icon.js#newcode23 lib/icon.js:23: 1, 1, 1, 1, 1, Since, we have 10 ...
Jan. 23, 2016, 8:38 p.m. (2016-01-23 20:38:30 UTC) #12
kzar
Patch Set 8 : Addressed some more feedback https://codereview.adblockplus.org/29334223/diff/29334419/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29334223/diff/29334419/lib/icon.js#newcode23 lib/icon.js:23: 1, ...
Jan. 23, 2016, 9:16 p.m. (2016-01-23 21:16:57 UTC) #13
Sebastian Noack
https://codereview.adblockplus.org/29334223/diff/29334419/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29334223/diff/29334419/lib/icon.js#newcode83 lib/icon.js:83: if (frameCache) On 2016/01/23 21:16:57, kzar wrote: > On ...
Jan. 23, 2016, 9:26 p.m. (2016-01-23 21:26:33 UTC) #14
kzar
Patch Set 9 : Handle tab switching / closing during animations https://codereview.adblockplus.org/29334223/diff/29334419/lib/icon.js File lib/icon.js (right): ...
Jan. 23, 2016, 10:01 p.m. (2016-01-23 22:01:06 UTC) #15
kzar
Patch Set 10 : Pre render frames, listen for onactivated Patch Set 11 : Avoid ...
Jan. 24, 2016, 3:43 p.m. (2016-01-24 15:43:02 UTC) #16
kzar
Patch Set 12 : Fixed bug in animationStep counter inc
Jan. 24, 2016, 4:34 p.m. (2016-01-24 16:34:43 UTC) #17
Sebastian Noack
https://codereview.adblockplus.org/29334223/diff/29334490/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29334223/diff/29334490/lib/icon.js#newcode48 lib/icon.js:48: canvas.height = image.height; With the new approach, we don't ...
Jan. 25, 2016, 3:44 p.m. (2016-01-25 15:44:46 UTC) #18
kzar
Patch Set 13 : Addressed more feedback! https://codereview.adblockplus.org/29334223/diff/29334490/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29334223/diff/29334490/lib/icon.js#newcode48 lib/icon.js:48: canvas.height = ...
Jan. 26, 2016, 3:01 p.m. (2016-01-26 15:01:43 UTC) #19
Sebastian Noack
https://codereview.adblockplus.org/29334223/diff/29334490/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29334223/diff/29334490/lib/icon.js#newcode62 lib/icon.js:62: let greyed = whitelistedState.get(page) && !safariPlatform && true || ...
Jan. 26, 2016, 4:12 p.m. (2016-01-26 16:12:39 UTC) #20
kzar
Patch Set 14 : We don't need to convert icons to data URLs Patch Set ...
Jan. 26, 2016, 5:10 p.m. (2016-01-26 17:10:55 UTC) #21
Sebastian Noack
https://codereview.adblockplus.org/29334223/diff/29334584/lib/notificationHelper.js File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29334223/diff/29334584/lib/notificationHelper.js#newcode203 lib/notificationHelper.js:203: }, function() {}); While changing this code, note that ...
Jan. 26, 2016, 5:23 p.m. (2016-01-26 17:23:15 UTC) #22
kzar
Patch Set 16 : Removed NOOP callback, renamed "greyed" variable https://codereview.adblockplus.org/29334223/diff/29334490/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29334223/diff/29334490/lib/icon.js#newcode62 ...
Jan. 26, 2016, 5:35 p.m. (2016-01-26 17:35:11 UTC) #23
kzar
Patch Set 17 : Fix onActivated dispatch for Safari
Jan. 26, 2016, 6:06 p.m. (2016-01-26 18:06:35 UTC) #24
Sebastian Noack
https://codereview.adblockplus.org/29334223/diff/29334490/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29334223/diff/29334490/lib/icon.js#newcode112 lib/icon.js:112: for (let opacity of [0, 0.1, 0.2, 0.3, 0.4, ...
Jan. 26, 2016, 6:10 p.m. (2016-01-26 18:10:36 UTC) #25
kzar
Patch Set 18 : Remove stopAnimation closure and hard coded opacity range https://codereview.adblockplus.org/29334223/diff/29334490/lib/icon.js File lib/icon.js ...
Jan. 26, 2016, 6:46 p.m. (2016-01-26 18:46:36 UTC) #26
Sebastian Noack
https://codereview.adblockplus.org/29334223/diff/29334632/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29334223/diff/29334632/lib/icon.js#newcode37 lib/icon.js:37: image.addEventListener("load", () => Nit: The indentation of the block ...
Jan. 26, 2016, 10:35 p.m. (2016-01-26 22:35:18 UTC) #27
Sebastian Noack
https://codereview.adblockplus.org/29334223/diff/29334548/safari/ext/background.js File safari/ext/background.js (right): https://codereview.adblockplus.org/29334223/diff/29334548/safari/ext/background.js#newcode178 safari/ext/background.js:178: this._frames = [{url: new URL(url || "about:blank"), parent: null}]; ...
Jan. 26, 2016, 10:44 p.m. (2016-01-26 22:44:51 UTC) #28
kzar
Patch Set 19 : Addressed final nits https://codereview.adblockplus.org/29334223/diff/29334548/safari/ext/background.js File safari/ext/background.js (right): https://codereview.adblockplus.org/29334223/diff/29334548/safari/ext/background.js#newcode158 safari/ext/background.js:158: var pageId ...
Jan. 26, 2016, 10:59 p.m. (2016-01-26 22:59:39 UTC) #29
Sebastian Noack
Jan. 26, 2016, 11:10 p.m. (2016-01-26 23:10:15 UTC) #30
LGTM

Powered by Google App Engine
This is Rietveld