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. |
DescriptionIssue 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 #
MessagesTotal messages: 30
Patch Set 1
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 (right): https://codereview.adblockplus.org/29334223/diff/29334224/background.js#newco... background.js:130: updateIcon(page, whitelisted && true || false); Why mind casting to bool here? https://codereview.adblockplus.org/29334223/diff/29334224/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29334223/diff/29334224/lib/icon.js#newcode20 lib/icon.js:20: const frameOpacities = [0, 0.2, 0.4, 0.6, 0.8, This has been copied from the original implementation but note that we increased the number of steps from 5 to 10 when we used pre-rendered images. Since e cache the generated animation frames now, 10 steps should still be fine. https://codereview.adblockplus.org/29334223/diff/29334224/lib/icon.js#newcode23 lib/icon.js:23: const numberOfFrames = frameOpacities.length; While rewriting most of this file, this is a good oppertunity to turn on strict mode. However, that means that you cannot use const here. https://codereview.adblockplus.org/29334223/diff/29334224/lib/icon.js#newcode29 lib/icon.js:29: function loadImage(path, size) Awesome, using a Promise here. But note that we load icons in a similar way in notificationHelper.js. Ideally, we unify that code in a common helper function.
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#newco... background.js:130: updateIcon(page, whitelisted && true || false); On 2016/01/21 18:11:01, Sebastian Noack wrote: > Why mind casting to bool here? Because the documentation for updateIcon says it expects a bool, but I realised we were actually passing either a filter or null. In a previous iteration of my work it mattered, now it doesn't so much but I still figured I should correct the mistake. https://codereview.adblockplus.org/29334223/diff/29334224/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29334223/diff/29334224/lib/icon.js#newcode20 lib/icon.js:20: const frameOpacities = [0, 0.2, 0.4, 0.6, 0.8, On 2016/01/21 18:11:01, Sebastian Noack wrote: > This has been copied from the original implementation but note that we increased > the number of steps from 5 to 10 when we used pre-rendered images. Since e cache > the generated animation frames now, 10 steps should still be fine. Haha, I did actually change it to 10 steps before as it looked a little smoother, but then I thought I better change it back! I should have trusted myself, Done. https://codereview.adblockplus.org/29334223/diff/29334224/lib/icon.js#newcode23 lib/icon.js:23: const numberOfFrames = frameOpacities.length; On 2016/01/21 18:11:01, Sebastian Noack wrote: > While rewriting most of this file, this is a good oppertunity to turn on strict > mode. However, that means that you cannot use const here. Done. https://codereview.adblockplus.org/29334223/diff/29334224/lib/icon.js#newcode29 lib/icon.js:29: function loadImage(path, size) On 2016/01/21 18:11:01, Sebastian Noack wrote: > Awesome, using a Promise here. But note that we load icons in a similar way in > notificationHelper.js. Ideally, we unify that code in a common helper function. Done. (Kinda... I put loadImage across, but couldn't see how to remove the duplication of the canvas stuff.)
Patch Set 3 : Avoid warnings when closing tabs during animation
Patch Set 4 : Don't bother generate Safari icons at build time
https://codereview.adblockplus.org/29334223/diff/29334224/background.js File background.js (right): https://codereview.adblockplus.org/29334223/diff/29334224/background.js#newco... background.js:130: updateIcon(page, whitelisted && true || false); On 2016/01/21 21:07:30, kzar wrote: > On 2016/01/21 18:11:01, Sebastian Noack wrote: > > Why mind casting to bool here? > > Because the documentation for updateIcon says it expects a bool, but I realised > we were actually passing either a filter or null. In a previous iteration of my > work it mattered, now it doesn't so much but I still figured I should correct > the mistake. Well, documented type annotations or not, JavaScript is still a lazy typed language. And adding the extra boilerplate to coerce arguments to a particular type seems a little backwards. https://codereview.adblockplus.org/29334223/diff/29334224/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29334223/diff/29334224/lib/icon.js#newcode23 lib/icon.js:23: const numberOfFrames = frameOpacities.length; On 2016/01/21 21:07:30, kzar wrote: > On 2016/01/21 18:11:01, Sebastian Noack wrote: > > While rewriting most of this file, this is a good oppertunity to turn on > strict > > mode. However, that means that you cannot use const here. > > Done. For reference, jsHydra converts const (like let) to var. So using const in strict mode is actually fine here. It doesn't have any runtime effects though. https://codereview.adblockplus.org/29334223/diff/29334247/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29334223/diff/29334247/lib/icon.js#newcode19 lib/icon.js:19: "use strict"; Nit: Please add a newline above. That's more consistent with how we enable strict mode in other scripts (in the adblockplus repo). Also I think it looks better. https://codereview.adblockplus.org/29334223/diff/29334247/lib/utils.js File lib/utils.js (right): https://codereview.adblockplus.org/29334223/diff/29334247/lib/utils.js#newcod... lib/utils.js:124: loadImage: function(path, size) I think I'd rather keep that function in icon.js but merely make it public and add documentation. https://codereview.adblockplus.org/29334223/diff/29334247/lib/utils.js#newcod... lib/utils.js:126: if (!path) Is that actually a scenario we need to handle? https://codereview.adblockplus.org/29334223/diff/29334247/lib/utils.js#newcod... lib/utils.js:134: image.addEventListener("load", function() { resolve(image); }); Since it's a general purpose function now, I'd also handle onerror. https://codereview.adblockplus.org/29334223/diff/29334247/lib/utils.js#newcod... lib/utils.js:134: image.addEventListener("load", function() { resolve(image); }); Since both, the code in icon.js and notificationHelper.js need a canvas how fulfilling the promise with a canvas object? Note that drawImage() can also take a canvas element when blending the images in icon.js. https://codereview.adblockplus.org/29334223/diff/29334247/metadata.safari File metadata.safari (right): https://codereview.adblockplus.org/29334223/diff/29334247/metadata.safari#new... metadata.safari:22: icons/abp-16-notification-critical@2x.png = safari/icons/abp-32-notification-critical.png Nit: For consistency this should be above the line above.
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#newco... background.js:130: updateIcon(page, whitelisted && true || false); On 2016/01/22 15:59:15, Sebastian Noack wrote: > On 2016/01/21 21:07:30, kzar wrote: > > On 2016/01/21 18:11:01, Sebastian Noack wrote: > > > Why mind casting to bool here? > > > > Because the documentation for updateIcon says it expects a bool, but I > realised > > we were actually passing either a filter or null. In a previous iteration of > my > > work it mattered, now it doesn't so much but I still figured I should correct > > the mistake. > > Well, documented type annotations or not, JavaScript is still a lazy typed > language. And adding the extra boilerplate to coerce arguments to a particular > type seems a little backwards. Ok, I'll just remove this change. https://codereview.adblockplus.org/29334223/diff/29334224/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29334223/diff/29334224/lib/icon.js#newcode23 lib/icon.js:23: const numberOfFrames = frameOpacities.length; On 2016/01/22 15:59:15, Sebastian Noack wrote: > On 2016/01/21 21:07:30, kzar wrote: > > On 2016/01/21 18:11:01, Sebastian Noack wrote: > > > While rewriting most of this file, this is a good oppertunity to turn on > > strict > > > mode. However, that means that you cannot use const here. > > > > Done. > > For reference, jsHydra converts const (like let) to var. So using const in > strict mode is actually fine here. It doesn't have any runtime effects though. Acknowledged. https://codereview.adblockplus.org/29334223/diff/29334247/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29334223/diff/29334247/lib/icon.js#newcode19 lib/icon.js:19: "use strict"; On 2016/01/22 15:59:15, Sebastian Noack wrote: > Nit: Please add a newline above. That's more consistent with how we enable > strict mode in other scripts (in the adblockplus repo). Also I think it looks > better. Done. https://codereview.adblockplus.org/29334223/diff/29334247/lib/utils.js File lib/utils.js (right): https://codereview.adblockplus.org/29334223/diff/29334247/lib/utils.js#newcod... lib/utils.js:124: loadImage: function(path, size) On 2016/01/22 15:59:15, Sebastian Noack wrote: > I think I'd rather keep that function in icon.js but merely make it public and > add documentation. Done. https://codereview.adblockplus.org/29334223/diff/29334247/lib/utils.js#newcod... lib/utils.js:126: if (!path) On 2016/01/22 15:59:15, Sebastian Noack wrote: > Is that actually a scenario we need to handle? Yes, it meant I could simplify the logic to load the images in icon.js quite a bit by just using Promise.all. https://codereview.adblockplus.org/29334223/diff/29334247/lib/utils.js#newcod... lib/utils.js:134: image.addEventListener("load", function() { resolve(image); }); On 2016/01/22 15:59:15, Sebastian Noack wrote: > Since both, the code in icon.js and notificationHelper.js need a canvas how > fulfilling the promise with a canvas object? Note that drawImage() can also take > a canvas element when blending the images in icon.js. Ah cool, didn't know that drawImage could take a canvas and it didn't occur to me I could just create re-create the context if required. Seems to work fine :) https://codereview.adblockplus.org/29334223/diff/29334247/lib/utils.js#newcod... lib/utils.js:134: image.addEventListener("load", function() { resolve(image); }); On 2016/01/22 15:59:15, Sebastian Noack wrote: > Since it's a general purpose function now, I'd also handle onerror. Done. https://codereview.adblockplus.org/29334223/diff/29334247/metadata.safari File metadata.safari (right): https://codereview.adblockplus.org/29334223/diff/29334247/metadata.safari#new... metadata.safari:22: icons/abp-16-notification-critical@2x.png = safari/icons/abp-32-notification-critical.png On 2016/01/22 15:59:15, Sebastian Noack wrote: > Nit: For consistency this should be above the line above. Done.
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 canvas Nit: Missing full stop? https://codereview.adblockplus.org/29334223/diff/29334375/lib/icon.js#newcode37 lib/icon.js:37: * @return {promise} Nit: It's "Promise" (capitalized) since it's a class. https://codereview.adblockplus.org/29334223/diff/29334375/lib/icon.js#newcode39 lib/icon.js:39: exports.loadImage = function loadImage(path, size) Nit: Named function assigned to a variable? https://codereview.adblockplus.org/29334223/diff/29334375/lib/icon.js#newcode39 lib/icon.js:39: exports.loadImage = function loadImage(path, size) Perhaps we should call the function loadCanvas or loadImageAsCanvas? https://codereview.adblockplus.org/29334223/diff/29334375/lib/icon.js#newcode40 lib/icon.js:40: { Nit: It seems that the indentation is off here. https://codereview.adblockplus.org/29334223/diff/29334375/lib/icon.js#newcode41 lib/icon.js:41: if (!path) Ah, now I see why you check for the path being empty. If for whatever reason the path should be empty neither onload nor onerror would fire and the promise will never be resolved. However, I'd rather throw an exception in that case. https://codereview.adblockplus.org/29334223/diff/29334375/lib/icon.js#newcode43 lib/icon.js:43: if (size) I'm not sure whether this logic should be handled here or rather in the calling code, in particular as it seems to be specific to the code below. https://codereview.adblockplus.org/29334223/diff/29334375/lib/icon.js#newcode49 lib/icon.js:49: image.addEventListener("load", function() Nit: How about using arrow functions? https://codereview.adblockplus.org/29334223/diff/29334375/lib/notificationHel... File lib/notificationHelper.js (left): https://codereview.adblockplus.org/29334223/diff/29334375/lib/notificationHel... lib/notificationHelper.js:155: function imgToBase64(url, callback) Since you return a promise now, the callback parameter is unused. https://codereview.adblockplus.org/29334223/diff/29334375/lib/notificationHel... File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29334223/diff/29334375/lib/notificationHel... lib/notificationHelper.js:159: return canvas.toDataURL("image/png"); Perhaps, we can simply get rid of this helper function now, just calling canvas.toDataURL("image/png") below?
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 passed to the listener here isn't an Error but an Event. So we should create our own Error to reject the promise with.
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 an image and draws it onto a canvas On 2016/01/23 14:04:09, Sebastian Noack wrote: > Nit: Missing full stop? Done. https://codereview.adblockplus.org/29334223/diff/29334375/lib/icon.js#newcode37 lib/icon.js:37: * @return {promise} On 2016/01/23 14:04:09, Sebastian Noack wrote: > Nit: It's "Promise" (capitalized) since it's a class. Done. https://codereview.adblockplus.org/29334223/diff/29334375/lib/icon.js#newcode39 lib/icon.js:39: exports.loadImage = function loadImage(path, size) On 2016/01/23 14:04:08, Sebastian Noack wrote: > Perhaps we should call the function loadCanvas or loadImageAsCanvas? Done. https://codereview.adblockplus.org/29334223/diff/29334375/lib/icon.js#newcode39 lib/icon.js:39: exports.loadImage = function loadImage(path, size) On 2016/01/23 14:04:09, Sebastian Noack wrote: > Nit: Named function assigned to a variable? Done. https://codereview.adblockplus.org/29334223/diff/29334375/lib/icon.js#newcode40 lib/icon.js:40: { On 2016/01/23 14:04:08, Sebastian Noack wrote: > Nit: It seems that the indentation is off here. Done. https://codereview.adblockplus.org/29334223/diff/29334375/lib/icon.js#newcode41 lib/icon.js:41: if (!path) On 2016/01/23 14:04:09, Sebastian Noack wrote: > Ah, now I see why you check for the path being empty. If for whatever reason the > path should be empty neither onload nor onerror would fire and the promise will > never be resolved. However, I'd rather throw an exception in that case. Done. https://codereview.adblockplus.org/29334223/diff/29334375/lib/icon.js#newcode43 lib/icon.js:43: if (size) On 2016/01/23 14:04:09, Sebastian Noack wrote: > I'm not sure whether this logic should be handled here or rather in the calling > code, in particular as it seems to be specific to the code below. Done. https://codereview.adblockplus.org/29334223/diff/29334375/lib/icon.js#newcode49 lib/icon.js:49: image.addEventListener("load", function() On 2016/01/23 14:04:09, Sebastian Noack wrote: > Nit: How about using arrow functions? Done. https://codereview.adblockplus.org/29334223/diff/29334375/lib/icon.js#newcode58 lib/icon.js:58: image.addEventListener("error", function(error) { reject(error); }); On 2016/01/23 14:17:41, Sebastian Noack wrote: > Note that the value passed to the listener here isn't an Error but an Event. So > we should create our own Error to reject the promise with. Done. https://codereview.adblockplus.org/29334223/diff/29334375/lib/notificationHel... File lib/notificationHelper.js (left): https://codereview.adblockplus.org/29334223/diff/29334375/lib/notificationHel... lib/notificationHelper.js:155: function imgToBase64(url, callback) On 2016/01/23 14:04:10, Sebastian Noack wrote: > Since you return a promise now, the callback parameter is unused. Acknowledged. https://codereview.adblockplus.org/29334223/diff/29334375/lib/notificationHel... File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29334223/diff/29334375/lib/notificationHel... lib/notificationHelper.js:159: return canvas.toDataURL("image/png"); On 2016/01/23 14:04:10, Sebastian Noack wrote: > Perhaps, we can simply get rid of this helper function now, just calling > canvas.toDataURL("image/png") below? Done.
Patch Set 7 : Rebased
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 frames per second now, you also need to repeat 1 (full opacity) twice as often to keep showing the icon for a full second. Nit: If you use 0.0 (instead 0) and 1.0 instead (1) that array definition looks a little nicer as the values align. https://codereview.adblockplus.org/29334223/diff/29334419/lib/icon.js#newcode35 lib/icon.js:35: * @param {string} path Path of the image to load Nit: "url" would be more accurrate than "path" as it infect can be any URL not only a relative path in the extension bundle. https://codereview.adblockplus.org/29334223/diff/29334419/lib/icon.js#newcode38 lib/icon.js:38: exports.loadImageAsCanvas = function (path, size) Nit: No space before arguments list. https://codereview.adblockplus.org/29334223/diff/29334419/lib/icon.js#newcode38 lib/icon.js:38: exports.loadImageAsCanvas = function (path, size) The size argument is unused. https://codereview.adblockplus.org/29334223/diff/29334419/lib/icon.js#newcode83 lib/icon.js:83: if (frameCache) Instead of caching we could probably simplify the logic by pre-rendering all frames in advance. Another advantage of that approach would be that the first time the icon is animated there is no overhead that might slow down the animation on slower computers. And we wouldn't load the source images again for each frame. I experimented, with that approach and got as far as following (untested) function. Feel free to use that code as-is or just for inspiration: function renderAnimationFrames() { return Promise.all([ loadImageAsCanvas(getIconPath(19, "")), loadImageAsCanvas(getIconPath(19, "-whitelisted")), loadImageAsCanvas(getIconPath(19, "-notification-" + notificationType)), loadImageAsCanvas(getIconPath(38, "")), loadImageAsCanvas(getIconPath(38, "-whitelisted")), loadImageAsCanvas(getIconPath(38, "-notification-" + notificationType)) ]).then(images => { return [false, true].map((whitelisted, i) => { var frames = []; for (let opacity = 0; opacity += 0.1; opacity <= 1) { let imageData = {}; [19, 38].forEach((size, j) => { let baseImage = images[j * 3 + i]; let overlayImage = images[j * 3 + 2]; let canvas = document.createElement("canvas"); let context = canvas.getContext("2d"); canvas.width = size; canvas.height = size; context.drawImage(baseImage, 0, 0); context.globalAlpha = opacity; context.drawImage(overlayImage, 0, 0); imageData[size] = context.getImageData(0, 0, size, size); }); frames.push(imageData); } return frames; }); }); } https://codereview.adblockplus.org/29334223/diff/29334419/lib/icon.js#newcode121 lib/icon.js:121: imageData: {19: imageData[0], How about generating the object as it is used in the first place rather than creating an array and turning it into an object here. https://codereview.adblockplus.org/29334223/diff/29334419/lib/icon.js#newcode137 lib/icon.js:137: ext.pages.query({active: true}, pages => Querying every 100ms for active tabs will impair the performance. We have a hotspot here. Note that the old logic merely checked every 10 seconds for active tabs running the animation on them. Though this caused the icon animation to be interrupted when tabs switch. If you don't want to live with that limitation you should listen on the onActivated event.
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, 1, 1, 1, 1, On 2016/01/23 20:38:30, Sebastian Noack wrote: > Since, we have 10 frames per second now, you also need to repeat 1 (full > opacity) twice as often to keep showing the icon for a full second. > > Nit: If you use 0.0 (instead 0) and 1.0 instead (1) that array definition looks > a little nicer as the values align. Done. https://codereview.adblockplus.org/29334223/diff/29334419/lib/icon.js#newcode35 lib/icon.js:35: * @param {string} path Path of the image to load On 2016/01/23 20:38:29, Sebastian Noack wrote: > Nit: "url" would be more accurrate than "path" as it infect can be any URL not > only a relative path in the extension bundle. Done. https://codereview.adblockplus.org/29334223/diff/29334419/lib/icon.js#newcode38 lib/icon.js:38: exports.loadImageAsCanvas = function (path, size) On 2016/01/23 20:38:29, Sebastian Noack wrote: > Nit: No space before arguments list. Done. https://codereview.adblockplus.org/29334223/diff/29334419/lib/icon.js#newcode38 lib/icon.js:38: exports.loadImageAsCanvas = function (path, size) On 2016/01/23 20:38:29, Sebastian Noack wrote: > The size argument is unused. Done. https://codereview.adblockplus.org/29334223/diff/29334419/lib/icon.js#newcode83 lib/icon.js:83: if (frameCache) On 2016/01/23 20:38:29, Sebastian Noack wrote: > Instead of caching we could probably simplify the logic by pre-rendering all > frames in advance. Another advantage of that approach would be that the first > time the icon is animated there is no overhead that might slow down the > animation on slower computers. And we wouldn't load the source images again for > each frame. > > I experimented, with that approach and got as far as following (untested) > function. Feel free to use that code as-is or just for inspiration: > > function renderAnimationFrames() > { > return Promise.all([ > loadImageAsCanvas(getIconPath(19, "")), > loadImageAsCanvas(getIconPath(19, "-whitelisted")), > loadImageAsCanvas(getIconPath(19, "-notification-" + notificationType)), > loadImageAsCanvas(getIconPath(38, "")), > loadImageAsCanvas(getIconPath(38, "-whitelisted")), > loadImageAsCanvas(getIconPath(38, "-notification-" + notificationType)) > ]).then(images => { > return [false, true].map((whitelisted, i) => > { > var frames = []; > > for (let opacity = 0; opacity += 0.1; opacity <= 1) > { > let imageData = {}; > > [19, 38].forEach((size, j) => > { > let baseImage = images[j * 3 + i]; > let overlayImage = images[j * 3 + 2]; > > let canvas = document.createElement("canvas"); > let context = canvas.getContext("2d"); > > canvas.width = size; > canvas.height = size; > > context.drawImage(baseImage, 0, 0); > context.globalAlpha = opacity; > context.drawImage(overlayImage, 0, 0); > > imageData[size] = context.getImageData(0, 0, size, size); > }); > > frames.push(imageData); > } > > return frames; > }); > }); > } I did consider pre-rendering them all, or caching everything but I was worried that it might consume a bunch of memory. I decided that caching the current animation only was a good compromise. That way when no animation is playing we're not wasting any memory. Having said that I'm flexible, I don't mind pre-rendering them all if you think the memory tradeoff is worth it. https://codereview.adblockplus.org/29334223/diff/29334419/lib/icon.js#newcode121 lib/icon.js:121: imageData: {19: imageData[0], On 2016/01/23 20:38:29, Sebastian Noack wrote: > How about generating the object as it is used in the first place rather than > creating an array and turning it into an object here. That would be nice but I'm not sure how to do it cleanly as I don't think chrome.browserAction.setIcon can take promises for the image data. https://codereview.adblockplus.org/29334223/diff/29334419/lib/icon.js#newcode137 lib/icon.js:137: ext.pages.query({active: true}, pages => On 2016/01/23 20:38:29, Sebastian Noack wrote: > Querying every 100ms for active tabs will impair the performance. We have a > hotspot here. Note that the old logic merely checked every 10 seconds for active > tabs running the animation on them. Though this caused the icon animation to be > interrupted when tabs switch. If you don't want to live with that limitation you > should listen on the onActivated event. Ah, whoops. I changed this around because I noticed that when you closed a tab during an animation a lot of exceptions would be thrown. I've changed it back, I'm not sure that avoiding those exceptions is worth setting up a `chrome.tabs.onRemoved` listener, what do you think?
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 2016/01/23 20:38:29, Sebastian Noack wrote: > > Instead of caching we could probably simplify the logic by pre-rendering all > > frames in advance. Another advantage of that approach would be that the first > > time the icon is animated there is no overhead that might slow down the > > animation on slower computers. And we wouldn't load the source images again > for > > each frame. > > > > I experimented, with that approach and got as far as following (untested) > > function. Feel free to use that code as-is or just for inspiration: > > > > function renderAnimationFrames() > > { > > return Promise.all([ > > loadImageAsCanvas(getIconPath(19, "")), > > loadImageAsCanvas(getIconPath(19, "-whitelisted")), > > loadImageAsCanvas(getIconPath(19, "-notification-" + notificationType)), > > loadImageAsCanvas(getIconPath(38, "")), > > loadImageAsCanvas(getIconPath(38, "-whitelisted")), > > loadImageAsCanvas(getIconPath(38, "-notification-" + notificationType)) > > ]).then(images => { > > return [false, true].map((whitelisted, i) => > > { > > var frames = []; > > > > for (let opacity = 0; opacity += 0.1; opacity <= 1) > > { > > let imageData = {}; > > > > [19, 38].forEach((size, j) => > > { > > let baseImage = images[j * 3 + i]; > > let overlayImage = images[j * 3 + 2]; > > > > let canvas = document.createElement("canvas"); > > let context = canvas.getContext("2d"); > > > > canvas.width = size; > > canvas.height = size; > > > > context.drawImage(baseImage, 0, 0); > > context.globalAlpha = opacity; > > context.drawImage(overlayImage, 0, 0); > > > > imageData[size] = context.getImageData(0, 0, size, size); > > }); > > > > frames.push(imageData); > > } > > > > return frames; > > }); > > }); > > } > > I did consider pre-rendering them all, or caching everything but I was worried > that it might consume a bunch of memory. I decided that caching the current > animation only was a good compromise. That way when no animation is playing > we're not wasting any memory. > > Having said that I'm flexible, I don't mind pre-rendering them all if you think > the memory tradeoff is worth it. The idea is NOT to prerender them on extension intilization. But prerender the whole animation prior to performing it. https://codereview.adblockplus.org/29334223/diff/29334419/lib/icon.js#newcode121 lib/icon.js:121: imageData: {19: imageData[0], On 2016/01/23 21:16:55, kzar wrote: > On 2016/01/23 20:38:29, Sebastian Noack wrote: > > How about generating the object as it is used in the first place rather than > > creating an array and turning it into an object here. > > That would be nice but I'm not sure how to do it cleanly as I don't think > chrome.browserAction.setIcon can take promises for the image data. See my other comment, and the code snippet I provided there. https://codereview.adblockplus.org/29334223/diff/29334419/lib/icon.js#newcode137 lib/icon.js:137: ext.pages.query({active: true}, pages => On 2016/01/23 21:16:56, kzar wrote: > On 2016/01/23 20:38:29, Sebastian Noack wrote: > > Querying every 100ms for active tabs will impair the performance. We have a > > hotspot here. Note that the old logic merely checked every 10 seconds for > active > > tabs running the animation on them. Though this caused the icon animation to > be > > interrupted when tabs switch. If you don't want to live with that limitation > you > > should listen on the onActivated event. > > Ah, whoops. I changed this around because I noticed that when you closed a tab > during an animation a lot of exceptions would be thrown. > > I've changed it back, I'm not sure that avoiding those exceptions is worth > setting up a `chrome.tabs.onRemoved` listener, what do you think? Well, exceptions shouldn't occur. The easiest way, that would probably work, to avoid them, would be not calling chrome.browserAction.setIcon if there is no entry in the PageMap we use to determine whether the icon is in whitelisted/disabled state.
Patch Set 9 : Handle tab switching / closing during animations 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#newcode137 lib/icon.js:137: ext.pages.query({active: true}, pages => On 2016/01/23 21:26:33, Sebastian Noack wrote: > On 2016/01/23 21:16:56, kzar wrote: > > On 2016/01/23 20:38:29, Sebastian Noack wrote: > > > Querying every 100ms for active tabs will impair the performance. We have a > > > hotspot here. Note that the old logic merely checked every 10 seconds for > > active > > > tabs running the animation on them. Though this caused the icon animation to > > be > > > interrupted when tabs switch. If you don't want to live with that limitation > > you > > > should listen on the onActivated event. > > > > Ah, whoops. I changed this around because I noticed that when you closed a tab > > during an animation a lot of exceptions would be thrown. > > > > I've changed it back, I'm not sure that avoiding those exceptions is worth > > setting up a `chrome.tabs.onRemoved` listener, what do you think? > > Well, exceptions shouldn't occur. The easiest way, that would probably work, to > avoid them, would be not calling chrome.browserAction.setIcon if there is no > entry in the PageMap we use to determine whether the icon is in > whitelisted/disabled state. On second thoughts I think it is worth adding the event listeners. Otherwise when you switch tab mid animation the animation continues to play for the wrong tab. (FWIW I tried your idea of checking `whitelistedState.has(page)` and it worked to avoid the exceptions.)
Patch Set 10 : Pre render frames, listen for onactivated Patch Set 11 : Avoid exceptions when active tab is closed 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:26:32, Sebastian Noack wrote: > On 2016/01/23 21:16:57, kzar wrote: > > On 2016/01/23 20:38:29, Sebastian Noack wrote: > > > Instead of caching we could probably simplify the logic by pre-rendering all > > > frames in advance. Another advantage of that approach would be that the > first > > > time the icon is animated there is no overhead that might slow down the > > > animation on slower computers. And we wouldn't load the source images again > > for > > > each frame. > > > > > > I experimented, with that approach and got as far as following (untested) > > > function. Feel free to use that code as-is or just for inspiration: > > > > > > function renderAnimationFrames() > > > { > > > return Promise.all([ > > > loadImageAsCanvas(getIconPath(19, "")), > > > loadImageAsCanvas(getIconPath(19, "-whitelisted")), > > > loadImageAsCanvas(getIconPath(19, "-notification-" + notificationType)), > > > loadImageAsCanvas(getIconPath(38, "")), > > > loadImageAsCanvas(getIconPath(38, "-whitelisted")), > > > loadImageAsCanvas(getIconPath(38, "-notification-" + notificationType)) > > > ]).then(images => { > > > return [false, true].map((whitelisted, i) => > > > { > > > var frames = []; > > > > > > for (let opacity = 0; opacity += 0.1; opacity <= 1) > > > { > > > let imageData = {}; > > > > > > [19, 38].forEach((size, j) => > > > { > > > let baseImage = images[j * 3 + i]; > > > let overlayImage = images[j * 3 + 2]; > > > > > > let canvas = document.createElement("canvas"); > > > let context = canvas.getContext("2d"); > > > > > > canvas.width = size; > > > canvas.height = size; > > > > > > context.drawImage(baseImage, 0, 0); > > > context.globalAlpha = opacity; > > > context.drawImage(overlayImage, 0, 0); > > > > > > imageData[size] = context.getImageData(0, 0, size, size); > > > }); > > > > > > frames.push(imageData); > > > } > > > > > > return frames; > > > }); > > > }); > > > } > > > > I did consider pre-rendering them all, or caching everything but I was worried > > that it might consume a bunch of memory. I decided that caching the current > > animation only was a good compromise. That way when no animation is playing > > we're not wasting any memory. > > > > Having said that I'm flexible, I don't mind pre-rendering them all if you > think > > the memory tradeoff is worth it. > > The idea is NOT to prerender them on extension intilization. But prerender the > whole animation prior to performing it. Ah I see, I think you're right. Done. https://codereview.adblockplus.org/29334223/diff/29334464/safari/ext/backgrou... File safari/ext/background.js (right): https://codereview.adblockplus.org/29334223/diff/29334464/safari/ext/backgrou... safari/ext/background.js:157: var activeTab = event.target.browserWindow.activeTab; I'm not sure I've got this dispatcher right, does it look OK to you? https://codereview.adblockplus.org/29334223/diff/29334464/safari/ext/backgrou... safari/ext/background.js:178: this._frames = [{url: new URL(url || "about:blank"), parent: null}]; Required as apparently Safari throws an exception for `new URL("")` unlike Chrome.
Patch Set 12 : Fixed bug in animationStep counter inc
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 necessarily need a canvas element anymore, but just something that we be passed to drawImage(). So converting the image to a canvas is redundant for the logic we have in this script. As for notificationHelper.js, I wonder whether we actually need a data URL or there or whether we could simply pass the URL of the icon instead? The assumption there was that the icon would have to be a web accessible resource otherwise. But I never doubled checked. https://codereview.adblockplus.org/29334223/diff/29334490/lib/icon.js#newcode62 lib/icon.js:62: let greyed = whitelistedState.get(page) && !safariPlatform && true || false; Nit: Please don't add redundant boilerplate to cast values too bool. Not that it would be any other type here anyway. https://codereview.adblockplus.org/29334223/diff/29334490/lib/icon.js#newcode98 lib/icon.js:98: 19: { base: [images[0], images[1]], overlay: images[2] }, While using a structured object here, how about using a separate name for the whitelisted image, rather than nesting an array? That way all values are documented, and you keep the data structures flat/simple. { default: images[0], whitelisted: images[1], overlay: images[2] } 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, 0.5, 0.6, 0.7, 0.8, 0.9, 1]) I'd rather generate the opacity values rather than hard-coding them: for (let opacity = 0; opacity <= 1; opacity += 0.1;) https://codereview.adblockplus.org/29334223/diff/29334490/lib/icon.js#newcode118 lib/icon.js:118: frames["" + opacity + Following data structure would be preferable: frames[keyBasedOnOpacityAndWhitelisted] = {19: [..], 38: [..]} That way we have objects that can be passed as-is to chrome.browserAction.setIcon(), keeping the code performed during the animation to a minimum. https://codereview.adblockplus.org/29334223/diff/29334490/lib/icon.js#newcode128 lib/icon.js:128: function runAnimation(animationType) Any reason you changed the semantic from notificationType to animationType? The icon indicates whether there is an important or normal notification, not whether the animation is important or not. https://codereview.adblockplus.org/29334223/diff/29334490/lib/icon.js#newcode130 lib/icon.js:130: let frames = !safariPlatform && renderFrames(animationType); I'd rather wait for the promise to be fulfilled before starting the animation, instead adding the overhead of adding a callback on every frame. https://codereview.adblockplus.org/29334223/diff/29334490/lib/icon.js#newcode143 lib/icon.js:143: ext.pages.onActivated.addListener(appendActivePage); It seems that you only implemented ext.pages.onActivated for Chrome. Any chance this code i reached on Safari? And if not, why not simply calling chrome.tabs.onActivated directly? Also what happened with the case when a tab is removed while the animation runs, causing errors to be logged? https://codereview.adblockplus.org/29334223/diff/29334490/lib/icon.js#newcode168 lib/icon.js:168: clearInterval(frameInterval); I think the previous logic, using global variables rather than passing around functions was either to understand.
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 = image.height; On 2016/01/25 15:44:45, Sebastian Noack wrote: > With the new approach, we don't necessarily need a canvas element anymore, but > just something that we be passed to drawImage(). So converting the image to a > canvas is redundant for the logic we have in this script. > > As for notificationHelper.js, I wonder whether we actually need a data URL or > there or whether we could simply pass the URL of the icon instead? The > assumption there was that the icon would have to be a web accessible resource > otherwise. But I never doubled checked. Good point about the canvas, Done. Not sure about just passing the URL, I've asked Ross and others for an example notification, if I get one I'll try and confirm. https://codereview.adblockplus.org/29334223/diff/29334490/lib/icon.js#newcode62 lib/icon.js:62: let greyed = whitelistedState.get(page) && !safariPlatform && true || false; On 2016/01/25 15:44:43, Sebastian Noack wrote: > Nit: Please don't add redundant boilerplate to cast values too bool. Not that it > would be any other type here anyway. Actually the value here is either null or the whitelisting filter. Also that does matter, we're converting the bool to a string as part of the frame key. https://codereview.adblockplus.org/29334223/diff/29334490/lib/icon.js#newcode98 lib/icon.js:98: 19: { base: [images[0], images[1]], overlay: images[2] }, On 2016/01/25 15:44:44, Sebastian Noack wrote: > While using a structured object here, how about using a separate name for the > whitelisted image, rather than nesting an array? That way all values are > documented, and you keep the data structures flat/simple. > > { default: images[0], whitelisted: images[1], overlay: images[2] } Could do but then the logic later wouldn't be as easy I suppose. `images[size]["base"][whitelisted | 0]` would instead need a conditional. I don't feel too strongly either way, up to you. 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, 0.5, 0.6, 0.7, 0.8, 0.9, 1]) On 2016/01/25 15:44:44, Sebastian Noack wrote: > I'd rather generate the opacity values rather than hard-coding them: > > for (let opacity = 0; opacity <= 1; opacity += 0.1;) As would I but unfortunately that doesn't work. (Try 0.2 + 0.1 in the console!) https://codereview.adblockplus.org/29334223/diff/29334490/lib/icon.js#newcode118 lib/icon.js:118: frames["" + opacity + On 2016/01/25 15:44:44, Sebastian Noack wrote: > Following data structure would be preferable: > > frames[keyBasedOnOpacityAndWhitelisted] = {19: [..], 38: [..]} > > That way we have objects that can be passed as-is to > chrome.browserAction.setIcon(), keeping the code performed during the animation > to a minimum. Good point, Done. https://codereview.adblockplus.org/29334223/diff/29334490/lib/icon.js#newcode128 lib/icon.js:128: function runAnimation(animationType) On 2016/01/25 15:44:45, Sebastian Noack wrote: > Any reason you changed the semantic from notificationType to animationType? The > icon indicates whether there is an important or normal notification, not whether > the animation is important or not. Done. https://codereview.adblockplus.org/29334223/diff/29334490/lib/icon.js#newcode130 lib/icon.js:130: let frames = !safariPlatform && renderFrames(animationType); On 2016/01/25 15:44:44, Sebastian Noack wrote: > I'd rather wait for the promise to be fulfilled before starting the animation, > instead adding the overhead of adding a callback on every frame. Good point, Done. https://codereview.adblockplus.org/29334223/diff/29334490/lib/icon.js#newcode143 lib/icon.js:143: ext.pages.onActivated.addListener(appendActivePage); On 2016/01/25 15:44:43, Sebastian Noack wrote: > It seems that you only implemented ext.pages.onActivated for Chrome. Any chance > this code i reached on Safari? And if not, why not simply calling > chrome.tabs.onActivated directly? > > Also what happened with the case when a tab is removed while the animation runs, > causing errors to be logged? I have implemented it for Safari too, check safari/ext/background.js . (Please could you to check the Safari implementation as I'm not sure I've got it right.) As for the logged errors when a tab is removed, I fixed that in Patch Set 11. https://codereview.adblockplus.org/29334223/diff/29334490/lib/icon.js#newcode168 lib/icon.js:168: clearInterval(frameInterval); On 2016/01/25 15:44:43, Sebastian Noack wrote: > I think the previous logic, using global variables rather than passing around > functions was either to understand. I disagree there.
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 || false; On 2016/01/26 15:01:41, kzar wrote: > On 2016/01/25 15:44:43, Sebastian Noack wrote: > > Nit: Please don't add redundant boilerplate to cast values too bool. Not that > it > > would be any other type here anyway. > > Actually the value here is either null or the whitelisting filter. Also that > does matter, we're converting the bool to a string as part of the frame key. I see. But how about: let whitelisted = !!whitelistedState.get(page) && !safariPlatform Also note that I called the variable whitelisted for consistence with the terminology everywhere else here. https://codereview.adblockplus.org/29334223/diff/29334490/lib/icon.js#newcode98 lib/icon.js:98: 19: { base: [images[0], images[1]], overlay: images[2] }, On 2016/01/26 15:01:40, kzar wrote: > On 2016/01/25 15:44:44, Sebastian Noack wrote: > > While using a structured object here, how about using a separate name for the > > whitelisted image, rather than nesting an array? That way all values are > > documented, and you keep the data structures flat/simple. > > > > { default: images[0], whitelisted: images[1], overlay: images[2] } > > Could do but then the logic later wouldn't be as easy I suppose. > `images[size]["base"][whitelisted | 0]` would instead need a conditional. I > don't feel too strongly either way, up to you. Fair enough. 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, 0.5, 0.6, 0.7, 0.8, 0.9, 1]) On 2016/01/26 15:01:40, kzar wrote: > On 2016/01/25 15:44:44, Sebastian Noack wrote: > > I'd rather generate the opacity values rather than hard-coding them: > > > > for (let opacity = 0; opacity <= 1; opacity += 0.1;) > > As would I but unfortunately that doesn't work. (Try 0.2 + 0.1 in the console!) Alright, damn floating point precision. But you could generate numbers form 0 to 10 and, then divide them by 10. Alternatively, you could write the loop like that: for (let opacity = 0; opacity <= 1; opacity = (opacity * 10 + 1) / 10) https://codereview.adblockplus.org/29334223/diff/29334490/lib/icon.js#newcode168 lib/icon.js:168: clearInterval(frameInterval); On 2016/01/26 15:01:42, kzar wrote: > On 2016/01/25 15:44:43, Sebastian Noack wrote: > > I think the previous logic, using global variables rather than passing around > > functions was either to understand. > > I disagree there. It's not that we return a function with different logic deepening on some state, in which case functional programming might make sense. The calling code isn't even completely agnostic of the state as it still has to check whether the stopAnimation is set, which is a global variable as well. And since we are in a module here it doesn't matter whether we have 1 or n global variables. https://codereview.adblockplus.org/29334223/diff/29334548/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29334223/diff/29334548/lib/icon.js#newcode34 lib/icon.js:34: * Loads an image, returning a promise Nit: The return value is already doumented below. https://codereview.adblockplus.org/29334223/diff/29334548/lib/icon.js#newcode113 lib/icon.js:113: if (!(frameKey in frames)) If you iterate over the sizes in the inner loop you don't need this check. As a side-effect you'd also generate the key twice as often. data = {}; for (let size of [19, 38]) ... data[size] = context.getImageData(0, 0, size, size); frames["" + opacity + whitelisted] = data; https://codereview.adblockplus.org/29334223/diff/29334548/safari/ext/backgrou... File safari/ext/background.js (right): https://codereview.adblockplus.org/29334223/diff/29334548/safari/ext/backgrou... safari/ext/background.js:158: var pageId = addPage(activeTab, activeTab.url, false); Add page registers a new page (in our data structure) when reported by our content script on page load, or when discovering existing pages during extension intilization. When a tab becomes active later it is already known. Even worse calling addPage() again will generate a new page ID which messes up the internal state. The page you are looking for should be tab._visiblePage. https://codereview.adblockplus.org/29334223/diff/29334548/safari/ext/backgrou... safari/ext/background.js:178: this._frames = [{url: new URL(url || "about:blank"), parent: null}]; This change seems to be unrelated.
Patch Set 14 : We don't need to convert icons to data URLs Patch Set 15 : Iterate over size in inner loop https://codereview.adblockplus.org/29334223/diff/29334548/lib/icon.js File lib/icon.js (right): https://codereview.adblockplus.org/29334223/diff/29334548/lib/icon.js#newcode34 lib/icon.js:34: * Loads an image, returning a promise On 2016/01/26 16:12:37, Sebastian Noack wrote: > Nit: The return value is already doumented below. Acknowledged. https://codereview.adblockplus.org/29334223/diff/29334548/lib/icon.js#newcode113 lib/icon.js:113: if (!(frameKey in frames)) On 2016/01/26 16:12:37, Sebastian Noack wrote: > If you iterate over the sizes in the inner loop you don't need this check. As a > side-effect you'd also generate the key twice as often. > > data = {}; > for (let size of [19, 38]) > ... > data[size] = context.getImageData(0, 0, size, size); > frames["" + opacity + whitelisted] = data; Done.
https://codereview.adblockplus.org/29334223/diff/29334584/lib/notificationHel... File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29334223/diff/29334584/lib/notificationHel... lib/notificationHelper.js:203: }, function() {}); While changing this code, note that the callback is optional can just be omitted.
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 lib/icon.js:62: let greyed = whitelistedState.get(page) && !safariPlatform && true || false; On 2016/01/26 16:12:36, Sebastian Noack wrote: > On 2016/01/26 15:01:41, kzar wrote: > > On 2016/01/25 15:44:43, Sebastian Noack wrote: > > > Nit: Please don't add redundant boilerplate to cast values too bool. Not > that > > it > > > would be any other type here anyway. > > > > Actually the value here is either null or the whitelisting filter. Also that > > does matter, we're converting the bool to a string as part of the frame key. > > I see. But how about: > > let whitelisted = !!whitelistedState.get(page) && !safariPlatform > > > Also note that I called the variable whitelisted for consistence with the > terminology everywhere else here. Done. 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, 0.5, 0.6, 0.7, 0.8, 0.9, 1]) On 2016/01/26 16:12:36, Sebastian Noack wrote: > On 2016/01/26 15:01:40, kzar wrote: > > On 2016/01/25 15:44:44, Sebastian Noack wrote: > > > I'd rather generate the opacity values rather than hard-coding them: > > > > > > for (let opacity = 0; opacity <= 1; opacity += 0.1;) > > > > As would I but unfortunately that doesn't work. (Try 0.2 + 0.1 in the > console!) > > Alright, damn floating point precision. But you could generate numbers form 0 to > 10 and, then divide them by 10. Alternatively, you could write the loop like > that: > > for (let opacity = 0; opacity <= 1; opacity = (opacity * 10 + 1) / 10) Yea, I thought about those options but in the end figured that just hard coding the values was easier. The ideal solution IMHO would be something like `for (let opacity of uniques(frameOpacities)) {` but the creating the uniques function would then be more complexity... https://codereview.adblockplus.org/29334223/diff/29334490/lib/icon.js#newcode168 lib/icon.js:168: clearInterval(frameInterval); On 2016/01/26 16:12:35, Sebastian Noack wrote: > On 2016/01/26 15:01:42, kzar wrote: > > On 2016/01/25 15:44:43, Sebastian Noack wrote: > > > I think the previous logic, using global variables rather than passing > around > > > functions was either to understand. > > > > I disagree there. > > It's not that we return a function with different logic deepening on some state, > in which case functional programming might make sense. The calling code isn't > even completely agnostic of the state as it still has to check whether the > stopAnimation is set, which is a global variable as well. And since we are in a > module here it doesn't matter whether we have 1 or n global variables. I just think that having as much of the animation state encapsulated as possible is good. I find it easier to reason about code that uses less global state. Also during working on this code I sometimes found edge cases where two animations would run at once and the shared global state got in a mess, resulting in flickering and other glitchy behaviour. https://codereview.adblockplus.org/29334223/diff/29334584/lib/notificationHel... File lib/notificationHelper.js (right): https://codereview.adblockplus.org/29334223/diff/29334584/lib/notificationHel... lib/notificationHelper.js:203: }, function() {}); On 2016/01/26 17:23:14, Sebastian Noack wrote: > While changing this code, note that the callback is optional can just be > omitted. Done.
Patch Set 17 : Fix onActivated dispatch for Safari
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, 0.5, 0.6, 0.7, 0.8, 0.9, 1]) On 2016/01/26 17:35:09, kzar wrote: > On 2016/01/26 16:12:36, Sebastian Noack wrote: > > On 2016/01/26 15:01:40, kzar wrote: > > > On 2016/01/25 15:44:44, Sebastian Noack wrote: > > > > I'd rather generate the opacity values rather than hard-coding them: > > > > > > > > for (let opacity = 0; opacity <= 1; opacity += 0.1;) > > > > > > As would I but unfortunately that doesn't work. (Try 0.2 + 0.1 in the > > console!) > > > > Alright, damn floating point precision. But you could generate numbers form 0 > to > > 10 and, then divide them by 10. Alternatively, you could write the loop like > > that: > > > > for (let opacity = 0; opacity <= 1; opacity = (opacity * 10 + 1) / 10) > > Yea, I thought about those options but in the end figured that just hard coding > the values was easier. > > The ideal solution IMHO would be something like `for (let opacity of > uniques(frameOpacities)) {` but the creating the uniques function would then be > more complexity... Well, instead a custom unique() function, we should rather use a Set. But we would need a polyfill for Chrome <39 and Safari <7.1. With our current requirements a polyfill could be as simple as: function Set(iterable) { this._map = Object.create(null); if (iterable) for (let value of iterable) this.add(value); } Set.prototype = { add: function(value) { this._map[JSON.stringify(value)] = null; }, values: function() { return Object.getOwnPropertyNames(this._map).map(JSON.parse); } } But I'm still undecided whether this would be reasonable as long as this is the only use case. However, I'm opposed to hard-coding an array of 11 values in a for-of loop. Also keep in mind what piece of crap jsHydra will generate there. https://codereview.adblockplus.org/29334223/diff/29334490/lib/icon.js#newcode168 lib/icon.js:168: clearInterval(frameInterval); On 2016/01/26 17:35:09, kzar wrote: > On 2016/01/26 16:12:35, Sebastian Noack wrote: > > On 2016/01/26 15:01:42, kzar wrote: > > > On 2016/01/25 15:44:43, Sebastian Noack wrote: > > > > I think the previous logic, using global variables rather than passing > > around > > > > functions was either to understand. > > > > > > I disagree there. > > > > It's not that we return a function with different logic deepening on some > state, > > in which case functional programming might make sense. The calling code isn't > > even completely agnostic of the state as it still has to check whether the > > stopAnimation is set, which is a global variable as well. And since we are in > a > > module here it doesn't matter whether we have 1 or n global variables. > > I just think that having as much of the animation state encapsulated as possible > is good. I find it easier to reason about code that uses less global state. You don't have any less global state. But the global variables are now just attached to the closure of this function rather than the closure of the module. > Also during working on this code I sometimes found edge cases where two animations > would run at once and the shared global state got in a mess, resulting in > flickering and other glitchy behaviour. You still have only one global variable, all state depends on, so if two animations would run at the same time you would still end up in a mess. Even worse, the function top stop the first animation will be gone and never be called.
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 (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, 0.5, 0.6, 0.7, 0.8, 0.9, 1]) On 2016/01/26 18:10:34, Sebastian Noack wrote: > On 2016/01/26 17:35:09, kzar wrote: > > On 2016/01/26 16:12:36, Sebastian Noack wrote: > > > On 2016/01/26 15:01:40, kzar wrote: > > > > On 2016/01/25 15:44:44, Sebastian Noack wrote: > > > > > I'd rather generate the opacity values rather than hard-coding them: > > > > > > > > > > for (let opacity = 0; opacity <= 1; opacity += 0.1;) > > > > > > > > As would I but unfortunately that doesn't work. (Try 0.2 + 0.1 in the > > > console!) > > > > > > Alright, damn floating point precision. But you could generate numbers form > 0 > > to > > > 10 and, then divide them by 10. Alternatively, you could write the loop like > > > that: > > > > > > for (let opacity = 0; opacity <= 1; opacity = (opacity * 10 + 1) / 10) > > > > Yea, I thought about those options but in the end figured that just hard > coding > > the values was easier. > > > > The ideal solution IMHO would be something like `for (let opacity of > > uniques(frameOpacities)) {` but the creating the uniques function would then > be > > more complexity... > > Well, instead a custom unique() function, we should rather use a Set. But we > would need a polyfill for Chrome <39 and Safari <7.1. With our current > requirements a polyfill could be as simple as: > > function Set(iterable) > { > this._map = Object.create(null); > if (iterable) > for (let value of iterable) > this.add(value); > } > Set.prototype = { > add: function(value) > { > this._map[JSON.stringify(value)] = null; > }, > values: function() > { > return Object.getOwnPropertyNames(this._map).map(JSON.parse); > } > } > > But I'm still undecided whether this would be reasonable as long as this is the > only use case. > > However, I'm opposed to hard-coding an array of 11 values in a for-of loop. Also > keep in mind what piece of crap jsHydra will generate there. Fair enough, how about this? https://codereview.adblockplus.org/29334223/diff/29334490/lib/icon.js#newcode168 lib/icon.js:168: clearInterval(frameInterval); On 2016/01/26 18:10:34, Sebastian Noack wrote: > On 2016/01/26 17:35:09, kzar wrote: > > On 2016/01/26 16:12:35, Sebastian Noack wrote: > > > On 2016/01/26 15:01:42, kzar wrote: > > > > On 2016/01/25 15:44:43, Sebastian Noack wrote: > > > > > I think the previous logic, using global variables rather than passing > > > around > > > > > functions was either to understand. > > > > > > > > I disagree there. > > > > > > It's not that we return a function with different logic deepening on some > > state, > > > in which case functional programming might make sense. The calling code > isn't > > > even completely agnostic of the state as it still has to check whether the > > > stopAnimation is set, which is a global variable as well. And since we are > in > > a > > > module here it doesn't matter whether we have 1 or n global variables. > > > > I just think that having as much of the animation state encapsulated as > possible > > is good. I find it easier to reason about code that uses less global state. > > You don't have any less global state. But the global variables are now just > attached to the closure of this function rather than the closure of the module. > > > Also during working on this code I sometimes found edge cases where two > animations > > would run at once and the shared global state got in a mess, resulting in > > flickering and other glitchy behaviour. > > You still have only one global variable, all state depends on, so if two > animations would run at the same time you would still end up in a mess. Even > worse, the function top stop the first animation will be gone and never be > called. Fair enough, Done.
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 below is inconsistent with the arrow functions in the surrounding code. https://codereview.adblockplus.org/29334223/diff/29334632/lib/icon.js#newcode54 lib/icon.js:54: if (notificationType && opacity > 0.5) The check for notificationType seems to be redundant. If we are not in an icon animation opacity should be 0 anyway. https://codereview.adblockplus.org/29334223/diff/29334632/lib/icon.js#newcode66 lib/icon.js:66: }); Nit: You should close the braces on the same depth of indentation you opened it. https://codereview.adblockplus.org/29334223/diff/29334632/lib/icon.js#newcode84 lib/icon.js:84: let images = { Nit: You declare a variable that is already defined here. https://codereview.adblockplus.org/29334223/diff/29334632/lib/icon.js#newcode132 lib/icon.js:132: pages.forEach(page => { Nit: Why not a for-of loop? https://codereview.adblockplus.org/29334223/diff/29334632/lib/icon.js#newcode152 lib/icon.js:152: 10000); Nit: Wrapping the line here doesn't seem to be necessary to stay under 80 chars per line, but certainly looks weird.
https://codereview.adblockplus.org/29334223/diff/29334548/safari/ext/backgrou... File safari/ext/background.js (right): https://codereview.adblockplus.org/29334223/diff/29334548/safari/ext/backgrou... safari/ext/background.js:178: this._frames = [{url: new URL(url || "about:blank"), parent: null}]; On 2016/01/26 16:12:37, Sebastian Noack wrote: > This change seems to be unrelated. It seems that you forgot to reply here. At least this change is still in the latest patch set. And I'm still wondering in which scenario url will be empty, and how this is related to the change here.
Patch Set 19 : Addressed final nits https://codereview.adblockplus.org/29334223/diff/29334548/safari/ext/backgrou... File safari/ext/background.js (right): https://codereview.adblockplus.org/29334223/diff/29334548/safari/ext/backgrou... safari/ext/background.js:158: var pageId = addPage(activeTab, activeTab.url, false); On 2016/01/26 16:12:38, Sebastian Noack wrote: > Add page registers a new page (in our data structure) when reported by our > content script on page load, or when discovering existing pages during extension > intilization. When a tab becomes active later it is already known. Even worse > calling addPage() again will generate a new page ID which messes up the internal > state. > > The page you are looking for should be tab._visiblePage. Acknowledged. https://codereview.adblockplus.org/29334223/diff/29334548/safari/ext/backgrou... safari/ext/background.js:178: this._frames = [{url: new URL(url || "about:blank"), parent: null}]; On 2016/01/26 22:44:50, Sebastian Noack wrote: > On 2016/01/26 16:12:37, Sebastian Noack wrote: > > This change seems to be unrelated. > > It seems that you forgot to reply here. At least this change is still in the > latest patch set. And I'm still wondering in which scenario url will be empty, > and how this is related to the change here. Whoops, you're right. Somehow in the past I was seeing warnings which this resolved. I've re-tested the animations after removing this change however and they're working fine. I guess there was a bug elsewhere previously. 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", () => On 2016/01/26 22:35:15, Sebastian Noack wrote: > Nit: The indentation of the block below is inconsistent with the arrow functions > in the surrounding code. Done. https://codereview.adblockplus.org/29334223/diff/29334632/lib/icon.js#newcode54 lib/icon.js:54: if (notificationType && opacity > 0.5) On 2016/01/26 22:35:17, Sebastian Noack wrote: > The check for notificationType seems to be redundant. If we are not in an icon > animation opacity should be 0 anyway. Done. https://codereview.adblockplus.org/29334223/diff/29334632/lib/icon.js#newcode66 lib/icon.js:66: }); On 2016/01/26 22:35:15, Sebastian Noack wrote: > Nit: You should close the braces on the same depth of indentation you opened it. Done. https://codereview.adblockplus.org/29334223/diff/29334632/lib/icon.js#newcode84 lib/icon.js:84: let images = { On 2016/01/26 22:35:15, Sebastian Noack wrote: > Nit: You declare a variable that is already defined here. I think it's pretty readable in this instance. https://codereview.adblockplus.org/29334223/diff/29334632/lib/icon.js#newcode132 lib/icon.js:132: pages.forEach(page => { On 2016/01/26 22:35:16, Sebastian Noack wrote: > Nit: Why not a for-of loop? Done. https://codereview.adblockplus.org/29334223/diff/29334632/lib/icon.js#newcode152 lib/icon.js:152: 10000); On 2016/01/26 22:35:16, Sebastian Noack wrote: > Nit: Wrapping the line here doesn't seem to be necessary to stay under 80 chars > per line, but certainly looks weird. Done.
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
