Issue 11161031: Show notifications on startup (Chrome) (Closed)
|
Created:
July 19, 2013, 5:26 p.m. by Felix Dahlke Modified:
July 25, 2013, 10:06 a.m. Reviewers:
Wladimir Palant Visibility:
Public. |
DescriptionNote that the UI is a bit more complex than that for FF, it's implemented as discussed in the forum.
Patch Set 1 #Patch Set 2 : Address issue from the Firefox review #
Total comments: 21
Patch Set 3 : Addressed issues (except for the animation) #
Total comments: 19
Patch Set 4 : Improved animation (animate for all severities, new icons, don't animate continuously, respect whit… #Patch Set 5 : Use just one click handler for all links #
Total comments: 4
Patch Set 6 : Addressed issues, fixed issues with tab/window switching #
Total comments: 2
Patch Set 7 : Address remaining issue #
Total comments: 10
Patch Set 8 : Addressed issues #
MessagesTotal messages: 15
http://codereview.adblockplus.org/11161031/diff/5001/background.js File background.js (right): http://codereview.adblockplus.org/11161031/diff/5001/background.js#newcode139 background.js:139: setNotificationPageAction(tab); This logic seems wrong to me - you are overriding refreshIconAndContextMenu() logic here instead of extending it. Suggestion: you set the state in showNotification() and refreshIconAndContextMenu() decides which icon to show and whether to animate between two icons. If the state changes you can call refreshIconAndContextMenu() for all tabs. http://codereview.adblockplus.org/11161031/diff/5001/background.js#newcode480 background.js:480: var activeNotification; Initialize with null so you don't work with undefined values in refreshIconAndContextMenu? http://codereview.adblockplus.org/11161031/diff/5001/background.js#newcode492 background.js:492: if (Object.keys(images).length === imageFiles.length) I really don't like relying on Object.keys() only having the keys you put into it, use a proper counter? http://codereview.adblockplus.org/11161031/diff/5001/background.js#newcode500 background.js:500: var abpIconFile = "icons/abp-19.png"; What about "icons/abp-19-whitelisted.png"? http://codereview.adblockplus.org/11161031/diff/5001/background.js#newcode542 background.js:542: }); Frankly, drawing images 60 times per second is a huge overkill (e.g. think of the CPU use). I suggest precomputing two images (abp-19.png with notification icon and abp-19-whitelisted.png with notification icon). For the animation switching between the regular icon and the version with notification every three seconds should be sufficient. http://codereview.adblockplus.org/11161031/diff/5001/background.js#newcode575 background.js:575: var notificationIconFile = "icons/notification-critical.png"; Again, what about abp-19-whitelisted.png? Same comment here - we can precompile the images (e.g. in showNotification), doing this all the time is unnecessary. http://codereview.adblockplus.org/11161031/diff/5001/lib/prefs.js File lib/prefs.js (right): http://codereview.adblockplus.org/11161031/diff/5001/lib/prefs.js#newcode36 lib/prefs.js:36: notificationdata: {} What about notificationurl? http://codereview.adblockplus.org/11161031/diff/5001/metadata.opera File metadata.opera (right): http://codereview.adblockplus.org/11161031/diff/5001/metadata.opera#newcode13 metadata.opera:13: notification Does Opera support it? http://codereview.adblockplus.org/11161031/diff/5001/notification.js File notification.js (right): http://codereview.adblockplus.org/11161031/diff/5001/notification.js#newcode11 notification.js:11: var texts = backgroundPage.getLocalizedTexts(notification); Call Notification and Utils directly here (via require) rather than asking the background page to do it? http://codereview.adblockplus.org/11161031/diff/5001/notification.js#newcode13 notification.js:13: titleElement.innerHTML = texts.title; textContent please, not innerHTML. http://codereview.adblockplus.org/11161031/diff/5001/notification.js#newcode15 notification.js:15: messageElement.innerHTML = texts.message; Please use the same code as in Firefox here, not innerHTML.
The animation thing needs discussing, I'll address the other comments in a bit. http://codereview.adblockplus.org/11161031/diff/5001/background.js File background.js (right): http://codereview.adblockplus.org/11161031/diff/5001/background.js#newcode542 background.js:542: }); On 2013/07/21 11:15:30, Wladimir Palant wrote: > Frankly, drawing images 60 times per second is a huge overkill (e.g. think of > the CPU use). I suggest precomputing two images (abp-19.png with notification > icon and abp-19-whitelisted.png with notification icon). For the animation > switching between the regular icon and the version with notification every three > seconds should be sufficient. Well, drawing images 60 times per second is how animations normally work, and we wouldn't be the only Chrome extension doing it. Note that this is more like _at_most_ 60 times per second, so I thought we'd at worst get a stuttering animation. We could move to simple blinking, but I personally prefer the animation. In any case, this is mostly an aesthetical issue, so we might want to delay that until lthe next release, given that we're in a bit of a hurry. I'm sure we'll get various ideas on how to improve this if I demonstrate it on Monday. Regarding abp-19-whitelisted.png, I did try to get the path of the current icon and use that, but that's not possible in Chrome so I didn't bother for now. We can of course save the current icon path whenever we set it. http://codereview.adblockplus.org/11161031/diff/5001/notification.js File notification.js (right): http://codereview.adblockplus.org/11161031/diff/5001/notification.js#newcode15 notification.js:15: messageElement.innerHTML = texts.message; On 2013/07/21 11:15:30, Wladimir Palant wrote: > Please use the same code as in Firefox here, not innerHTML. Hehe, I thought I'd try :D This should be safe in Chrome, but I can see how you'd rather not rely on that.
Addressed all issues except the animation for now. I did reduce the frame rate though (from 60fps to 3fps), which still looks good and uses only 2% of CPU on my machine, instead of 14%. The new idea with the animation is to have both the critical and the information icon animated, but animate it only every once in a while. I'll upload a new patch set with these changes in a bit. http://codereview.adblockplus.org/11161031/diff/5001/background.js File background.js (right): http://codereview.adblockplus.org/11161031/diff/5001/background.js#newcode139 background.js:139: setNotificationPageAction(tab); On 2013/07/21 11:15:30, Wladimir Palant wrote: > This logic seems wrong to me - you are overriding refreshIconAndContextMenu() > logic here instead of extending it. Suggestion: you set the state in > showNotification() and refreshIconAndContextMenu() decides which icon to show > and whether to animate between two icons. If the state changes you can call > refreshIconAndContextMenu() for all tabs. Done. http://codereview.adblockplus.org/11161031/diff/5001/background.js#newcode480 background.js:480: var activeNotification; On 2013/07/21 11:15:30, Wladimir Palant wrote: > Initialize with null so you don't work with undefined values in > refreshIconAndContextMenu? Done, didn't notice that we always tend to do that. http://codereview.adblockplus.org/11161031/diff/5001/background.js#newcode492 background.js:492: if (Object.keys(images).length === imageFiles.length) On 2013/07/21 11:15:30, Wladimir Palant wrote: > I really don't like relying on Object.keys() only having the keys you put into > it, use a proper counter? I had a hunch you'd say this :) Done. http://codereview.adblockplus.org/11161031/diff/5001/lib/prefs.js File lib/prefs.js (right): http://codereview.adblockplus.org/11161031/diff/5001/lib/prefs.js#newcode36 lib/prefs.js:36: notificationdata: {} On 2013/07/21 11:15:30, Wladimir Palant wrote: > What about notificationurl? Done. http://codereview.adblockplus.org/11161031/diff/5001/metadata.opera File metadata.opera (right): http://codereview.adblockplus.org/11161031/diff/5001/metadata.opera#newcode13 metadata.opera:13: notification On 2013/07/21 11:15:30, Wladimir Palant wrote: > Does Opera support it? I didn't test in Opera yet, got it on the TODO list though. Think it can wait until after we release for FF/Chrome? http://codereview.adblockplus.org/11161031/diff/5001/notification.js File notification.js (right): http://codereview.adblockplus.org/11161031/diff/5001/notification.js#newcode11 notification.js:11: var texts = backgroundPage.getLocalizedTexts(notification); On 2013/07/21 11:15:30, Wladimir Palant wrote: > Call Notification and Utils directly here (via require) rather than asking the > background page to do it? Done. Not sure why I didn't do it in fact. http://codereview.adblockplus.org/11161031/diff/5001/notification.js#newcode13 notification.js:13: titleElement.innerHTML = texts.title; On 2013/07/21 11:15:30, Wladimir Palant wrote: > textContent please, not innerHTML. Done. http://codereview.adblockplus.org/11161031/diff/5001/notification.js#newcode15 notification.js:15: messageElement.innerHTML = texts.message; On 2013/07/21 12:19:53, Felix H. Dahlke wrote: > On 2013/07/21 11:15:30, Wladimir Palant wrote: > > Please use the same code as in Firefox here, not innerHTML. > > Hehe, I thought I'd try :D This should be safe in Chrome, but I can see how > you'd rather not rely on that. Done. I think it's less than great that this code is duplicated in Chrome, but I don't see a way of using the same code in a manner that makes a lot of sense. I mean, we could have some function with callbacks for creating/attaching the objects in the Notification module, but I think it's not ideal. Got a better idea?
Improved the animation as discussed: 1. Animate the icon for both critical/information 2. Use new icons created by Sven 3. Don't animate continuously, only every 5 seconds (I know you wanted every 60 seconds, so we'll have some haggling coming up :D) 4. Respect the whitelisted icon Note that I do not cache individual frames, this appears to be easy enough on the CPU. The images are preloaded, so drawing two images on top of each other (even with alpha blending) 5 times per second should be fine. CPU usage barely spikes at all on my machine.
I've reviewed Patch Set 3, not Patch Set 4. Please have a look at my comments - IMHO, Patch Set 4 goes into the wrong direction again. http://codereview.adblockplus.org/11161031/diff/11001/background.js File background.js (right): http://codereview.adblockplus.org/11161031/diff/11001/background.js#newcode124 background.js:124: setNotificationPageAction(tab, iconFilename); setNotificationPageAction() won't do anything for non-critical notifications - does this mean that we don't set any icon then until the animation catches up? Should be if (activeNotification && activeNotification.severify == "critical) I think. http://codereview.adblockplus.org/11161031/diff/11001/background.js#newcode174 background.js:174: activeNotification = null; Shouldn't activeNotification be reset before calling refreshIconAndContextMenu()? Then again, resetting it here seems to be the wrong thing to do anyway - neither required nor really expected. Mind you, this is an async action so it will reset activeNotification at some random point in time. http://codereview.adblockplus.org/11161031/diff/11001/background.js#newcode198 background.js:198: var imageData = context.getImageData(0, 0, canvas.width, canvas.height); The code to combine two images with some opacity and get image data back could be moved into a separate function - to be reused in startInformationNotificationAnimation(). http://codereview.adblockplus.org/11161031/diff/11001/background.js#newcode545 background.js:545: var abpIconFile = "icons/abp-19.png"; This still hardcodes the icon and fails to consider that there can be two. This is why I asked for the animation to be started from refreshIconAndContextMenu() (and restarted for each tab switch) - you are building up a second control flow for the same object here, with duplicated logic. http://codereview.adblockplus.org/11161031/diff/11001/background.js#newcode573 background.js:573: / (duration - fadeOutStartTime); Pretty complicated and IMHO not really necessary calculations: var animationInterval = 200; var currentStep = 0; var opacityValues = [0.0, 0.2, 0.4, 0.6, 0.8, 1.0, 1.0, 1.0, 1.0, 1.0, 0.8, 0.6, 0.4, 0.2, 0.0]; function animationStep() { var opacity = opacityValues[++currentStep % opacityValues.length]; ... if (currentStep % (opacityValues.length * 3) == 0) setTimeout(animationStep, 60000); else setTimeout(animationStep, animationInteval); } Note that Date.now() isn't exactly precise (e.g. its value is known to be cached) and you using async actions in between distorts things even further. IMHO you shouldn't try to bind this to some specific time. http://codereview.adblockplus.org/11161031/diff/11001/notification.js File notification.js (right): http://codereview.adblockplus.org/11161031/diff/11001/notification.js#newcode89 notification.js:89: } So this loop is only there to attach event listeners to links? This is pointless, one event listener at the top level will do: messageElement.addEventListener("click", function(event) { var link = event.target; while (link && link != messageElement && link.localName != "a") link = link.parentNode; if (link) { event.preventDefault(); event.stopPropagation(); chrome.tabs.create({url: link.href}); } }, false);
Commented on all issues. Seems like most have already been addressed by the latest patch set. Some things need discussing. http://codereview.adblockplus.org/11161031/diff/11001/background.js File background.js (right): http://codereview.adblockplus.org/11161031/diff/11001/background.js#newcode124 background.js:124: setNotificationPageAction(tab, iconFilename); On 2013/07/22 14:18:36, Wladimir Palant wrote: > setNotificationPageAction() won't do anything for non-critical notifications - > does this mean that we don't set any icon then until the animation catches up? > Should be if (activeNotification && activeNotification.severify == "critical) I > think. Yes, it was like that. In patch set 4, all notification icons are animated, so that's the case for both information and critical now. But since the animation is updated every 200ms, that seems fine. http://codereview.adblockplus.org/11161031/diff/11001/background.js#newcode174 background.js:174: activeNotification = null; On 2013/07/22 14:18:36, Wladimir Palant wrote: > Shouldn't activeNotification be reset before calling > refreshIconAndContextMenu()? > > Then again, resetting it here seems to be the wrong thing to do anyway - neither > required nor really expected. Mind you, this is an async action so it will reset > activeNotification at some random point in time. Yes, I fixed that in patch set 4. http://codereview.adblockplus.org/11161031/diff/11001/background.js#newcode198 background.js:198: var imageData = context.getImageData(0, 0, canvas.width, canvas.height); On 2013/07/22 14:18:36, Wladimir Palant wrote: > The code to combine two images with some opacity and get image data back could > be moved into a separate function - to be reused in > startInformationNotificationAnimation(). Not necessary anymore, as of patch set 4. http://codereview.adblockplus.org/11161031/diff/11001/background.js#newcode545 background.js:545: var abpIconFile = "icons/abp-19.png"; On 2013/07/22 14:18:36, Wladimir Palant wrote: > This still hardcodes the icon and fails to consider that there can be two. This > is why I asked for the animation to be started from refreshIconAndContextMenu() > (and restarted for each tab switch) - you are building up a second control flow > for the same object here, with duplicated logic. Has been fixed in patch set 4. But yeah, second control flow. I considered starting an animation for each tab, but that means we have one timer for every tab, seems wasteful. http://codereview.adblockplus.org/11161031/diff/11001/background.js#newcode573 background.js:573: / (duration - fadeOutStartTime); On 2013/07/22 14:18:36, Wladimir Palant wrote: > Pretty complicated and IMHO not really necessary calculations: > > var animationInterval = 200; > var currentStep = 0; > var opacityValues = [0.0, 0.2, 0.4, 0.6, 0.8, > 1.0, 1.0, 1.0, 1.0, 1.0, > 0.8, 0.6, 0.4, 0.2, 0.0]; > function animationStep() > { > var opacity = opacityValues[++currentStep % opacityValues.length]; > ... > > if (currentStep % (opacityValues.length * 3) == 0) > setTimeout(animationStep, 60000); > else > setTimeout(animationStep, animationInteval); > } > > Note that Date.now() isn't exactly precise (e.g. its value is known to be > cached) and you using async actions in between distorts things even further. > IMHO you shouldn't try to bind this to some specific time. It's not really possible to get smooth animations without adjusting for the time between steps, but it might work out for us since we have a pretty low frame rate. I'd rather keep it like this though. http://codereview.adblockplus.org/11161031/diff/11001/notification.js File notification.js (right): http://codereview.adblockplus.org/11161031/diff/11001/notification.js#newcode89 notification.js:89: } On 2013/07/22 14:18:36, Wladimir Palant wrote: > So this loop is only there to attach event listeners to links? This is > pointless, one event listener at the top level will do: > > messageElement.addEventListener("click", function(event) > { > var link = event.target; > while (link && link != messageElement && link.localName != "a") > link = link.parentNode; > if (link) > { > event.preventDefault(); > event.stopPropagation(); > chrome.tabs.create({url: link.href}); > } > }, false); Yes, that would also work. Note that we have pretty much the same code in Firefox though, wouldn't we want to change it there as well then?
http://codereview.adblockplus.org/11161031/diff/11001/background.js File background.js (right): http://codereview.adblockplus.org/11161031/diff/11001/background.js#newcode573 background.js:573: / (duration - fadeOutStartTime); On 2013/07/22 15:10:22, Felix H. Dahlke wrote: > On 2013/07/22 14:18:36, Wladimir Palant wrote: > > Pretty complicated and IMHO not really necessary calculations: > > > > var animationInterval = 200; > > var currentStep = 0; > > var opacityValues = [0.0, 0.2, 0.4, 0.6, 0.8, > > 1.0, 1.0, 1.0, 1.0, 1.0, > > 0.8, 0.6, 0.4, 0.2, 0.0]; > > function animationStep() > > { > > var opacity = opacityValues[++currentStep % opacityValues.length]; > > ... > > > > if (currentStep % (opacityValues.length * 3) == 0) > > setTimeout(animationStep, 60000); > > else > > setTimeout(animationStep, animationInteval); > > } > > > > Note that Date.now() isn't exactly precise (e.g. its value is known to be > > cached) and you using async actions in between distorts things even further. > > IMHO you shouldn't try to bind this to some specific time. > > It's not really possible to get smooth animations without adjusting for the time > between steps, but it might work out for us since we have a pretty low frame > rate. I'd rather keep it like this though. But I definitely prefer how you approached the delay between animations to how I did it in patch set 4! Not sure why I ended up using a second timer...
On 2013/07/22 15:16:45, Felix H. Dahlke wrote: > But I definitely prefer how you approached the delay between animations to how I > did it in patch set 4! Not sure why I ended up using a second timer... Just implemented it this way, but it doesn't simplify things as much as I thought, and this also doesn't ensure anymore that the icon is being reset. So I'll wait for what you say about the approach.
http://codereview.adblockplus.org/11161031/diff/11001/background.js File background.js (right): http://codereview.adblockplus.org/11161031/diff/11001/background.js#newcode545 background.js:545: var abpIconFile = "icons/abp-19.png"; On 2013/07/22 15:10:22, Felix H. Dahlke wrote: > Has been fixed in patch set 4. But yeah, second control flow. I considered > starting an animation for each tab, but that means we have one timer for every > tab, seems wasteful. No, we can still work with a single global timer - refreshIconAndContextMenu() would simply clear it and create a new one for the current tab. http://codereview.adblockplus.org/11161031/diff/11001/background.js#newcode573 background.js:573: / (duration - fadeOutStartTime); On 2013/07/22 15:10:22, Felix H. Dahlke wrote: > It's not really possible to get smooth animations without adjusting for the time > between steps, but it might work out for us since we have a pretty low frame > rate. I'd rather keep it like this though. You don't have a smooth animation right now, exactly because what the time adjustment doesn't really work. So I'd rather prefer you switch to a simpler approach that should also provide better animations. http://codereview.adblockplus.org/11161031/diff/11001/notification.js File notification.js (right): http://codereview.adblockplus.org/11161031/diff/11001/notification.js#newcode89 notification.js:89: } On 2013/07/22 15:10:22, Felix H. Dahlke wrote: > Yes, that would also work. Note that we have pretty much the same code in > Firefox though, wouldn't we want to change it there as well then? Sure, it should be changed there as well. http://codereview.adblockplus.org/11161031/diff/16002/background.js File background.js (right): http://codereview.adblockplus.org/11161031/diff/16002/background.js#newcode124 background.js:124: activeNotification.tabIcons[tab.id] = iconFilename; This will leak memory if many tabs are opened and closed while the notification is active. http://codereview.adblockplus.org/11161031/diff/16002/background.js#newcode484 background.js:484: var iconAnimationRestartTimer = null; A single timer should definitely be sufficient.
http://codereview.adblockplus.org/11161031/diff/11001/background.js File background.js (right): http://codereview.adblockplus.org/11161031/diff/11001/background.js#newcode545 background.js:545: var abpIconFile = "icons/abp-19.png"; On 2013/07/23 07:25:04, Wladimir Palant wrote: > On 2013/07/22 15:10:22, Felix H. Dahlke wrote: > > Has been fixed in patch set 4. But yeah, second control flow. I considered > > starting an animation for each tab, but that means we have one timer for every > > tab, seems wasteful. > > No, we can still work with a single global timer - refreshIconAndContextMenu() > would simply clear it and create a new one for the current tab. Done. I do prefer this, just wasn't able to come up with it. Well, at least not in a hurry :) http://codereview.adblockplus.org/11161031/diff/11001/background.js#newcode573 background.js:573: / (duration - fadeOutStartTime); On 2013/07/23 07:25:04, Wladimir Palant wrote: > On 2013/07/22 15:10:22, Felix H. Dahlke wrote: > > It's not really possible to get smooth animations without adjusting for the > time > > between steps, but it might work out for us since we have a pretty low frame > > rate. I'd rather keep it like this though. > > You don't have a smooth animation right now, exactly because what the time > adjustment doesn't really work. So I'd rather prefer you switch to a simpler > approach that should also provide better animations. Implemented this as you prefer it, since it's not really important enough to argue a lot about. The good thing with this approach is that it's easier to understand, and at our low frame rate and since this is just fading, not moving, it works well enough. It does however stutter a bit now, and that's going to be worse on slower systems. We cannot enforce a fixed frame rate. Then again, it doesn't really look brilliant on slow systems even if we adjust for the variable frame rate properly. http://codereview.adblockplus.org/11161031/diff/11001/notification.js File notification.js (right): http://codereview.adblockplus.org/11161031/diff/11001/notification.js#newcode89 notification.js:89: } On 2013/07/23 07:25:04, Wladimir Palant wrote: > On 2013/07/22 15:10:22, Felix H. Dahlke wrote: > > Yes, that would also work. Note that we have pretty much the same code in > > Firefox though, wouldn't we want to change it there as well then? > > Sure, it should be changed there as well. Done. http://codereview.adblockplus.org/11161031/diff/16002/background.js File background.js (right): http://codereview.adblockplus.org/11161031/diff/16002/background.js#newcode124 background.js:124: activeNotification.tabIcons[tab.id] = iconFilename; On 2013/07/23 07:25:04, Wladimir Palant wrote: > This will leak memory if many tabs are opened and closed while the notification > is active. Done. http://codereview.adblockplus.org/11161031/diff/16002/background.js#newcode484 background.js:484: var iconAnimationRestartTimer = null; On 2013/07/23 07:25:04, Wladimir Palant wrote: > A single timer should definitely be sufficient. Done. http://codereview.adblockplus.org/11161031/diff/20003/background.js File background.js (right): http://codereview.adblockplus.org/11161031/diff/20003/background.js#newcode580 background.js:580: chrome.tabs.query({active: true, currentWindow: true}, refreshAll); I just noticed that I still use currentWindow here, which is apparently not supported in Chrome 18. Will fix that in a bit.
That final issues is address as well now. http://codereview.adblockplus.org/11161031/diff/20003/background.js File background.js (right): http://codereview.adblockplus.org/11161031/diff/20003/background.js#newcode580 background.js:580: chrome.tabs.query({active: true, currentWindow: true}, refreshAll); On 2013/07/23 10:38:08, Felix H. Dahlke wrote: > I just noticed that I still use currentWindow here, which is apparently not > supported in Chrome 18. Will fix that in a bit. Done. Note that this was incorrect anyway, the current window is not necessarily the currently focused window.
LGTM if all the comments are addressed. http://codereview.adblockplus.org/11161031/diff/1031/background.js File background.js (right): http://codereview.adblockplus.org/11161031/diff/1031/background.js#newcode124 background.js:124: startIconAnimation(tab, iconFilename); Don't we need to check whether this is actually the active tab? We do some refreshes for background tabs when we stop animations. http://codereview.adblockplus.org/11161031/diff/1031/background.js#newcode493 background.js:493: animatedIconTab = null; refreshIconAndContextMenu(animatedIconTab)? Well, not really necessary I guess... http://codereview.adblockplus.org/11161031/diff/1031/background.js#newcode577 background.js:577: chrome.tabs.query(null, refreshAll); Do we still need to refresh all icons here? if (animatedIconTab) { var tab = animatedIconTab; stopIconAnimation(); refreshAll([tab]); } refreshTabs() might be a better name than refreshAll(), it shouldn't actually apply to all tabs. http://codereview.adblockplus.org/11161031/diff/1031/background.js#newcode744 background.js:744: }); How about: chrome.tabs.get(activeInfo.tabId, refreshIconAndContextMenu); http://codereview.adblockplus.org/11161031/diff/1031/background.js#newcode751 background.js:751: refreshIconAndContextMenu(tabs[0]); How about dropping the assumption that there is only one tab: tabs.forEach(refreshIconAndContextMenu);
Uploaded a new patch set. Hopefully the last one :) http://codereview.adblockplus.org/11161031/diff/1031/background.js File background.js (right): http://codereview.adblockplus.org/11161031/diff/1031/background.js#newcode124 background.js:124: startIconAnimation(tab, iconFilename); On 2013/07/23 11:53:17, Wladimir Palant wrote: > Don't we need to check whether this is actually the active tab? We do some > refreshes for background tabs when we stop animations. I don't think it's necessary, since we only refresh background tabs on startup and right after activeAnimation is set to null. We could do it to be really safe, but I think then we'd also have to ensure that the tab is in the currently focused window, and we already have that logic below. http://codereview.adblockplus.org/11161031/diff/1031/background.js#newcode493 background.js:493: animatedIconTab = null; On 2013/07/23 11:53:17, Wladimir Palant wrote: > refreshIconAndContextMenu(animatedIconTab)? Well, not really necessary I > guess... Not necessary, no. I used to do that here, but removed it now that stopIconAnimation is called in startIconAnimation - doesn't really make much sense to set the icon right before it's being animated IMO. http://codereview.adblockplus.org/11161031/diff/1031/background.js#newcode577 background.js:577: chrome.tabs.query(null, refreshAll); On 2013/07/23 11:53:17, Wladimir Palant wrote: > Do we still need to refresh all icons here? > > if (animatedIconTab) > { > var tab = animatedIconTab; > stopIconAnimation(); > refreshAll([tab]); > } > > refreshTabs() might be a better name than refreshAll(), it shouldn't actually > apply to all tabs. Done. You're right, it's not necessary anymore now that we reset the icon for tabs that become inactive. http://codereview.adblockplus.org/11161031/diff/1031/background.js#newcode744 background.js:744: }); On 2013/07/23 11:53:17, Wladimir Palant wrote: > How about: > > chrome.tabs.get(activeInfo.tabId, refreshIconAndContextMenu); Done. http://codereview.adblockplus.org/11161031/diff/1031/background.js#newcode751 background.js:751: refreshIconAndContextMenu(tabs[0]); On 2013/07/23 11:53:17, Wladimir Palant wrote: > How about dropping the assumption that there is only one tab: > > tabs.forEach(refreshIconAndContextMenu); This should really only be one, but yeah, forEach is safer. I think the list can actually be empty sometimes.
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
