Issue 5196306347720704: Issue 1965 - Simplified and fixed missing image for icon animation (Closed)
|
Created:
Feb. 27, 2014, 4:54 p.m. by Sebastian Noack Modified:
Feb. 13, 2015, 4:12 p.m. CC:
kzar Visibility:
Public. |
DescriptionI realized that the filename for the whitelisted icon overlayed with a notification icon at full opacity was generated wrong.
While fixing this, I re-considered the approach for generating the animated icon's filename. It worked by recording the filename for the icon of every tab and patching the corresponding suffix into the filename with the power of regex. If this approach wouldn't be already worse enough, fixing the issue would involve stripping "-whitelisted" out of the filename with another regex.
So I moved the code generating the icon's filename in a separate function, that can be reused.
Patch Set 1 #Patch Set 2 : #
Total comments: 16
Patch Set 3 : Rabased and addressed comments #Patch Set 4 : #Patch Set 5 : Rabased and refactored code #
Total comments: 5
Patch Set 6 : Rebased and addressed comment #
MessagesTotal messages: 18
http://codereview.adblockplus.org/5196306347720704/diff/5156048109305856/back... File background.js (right): http://codereview.adblockplus.org/5196306347720704/diff/5156048109305856/back... background.js:117: if (whitelisted && (!animation || animation.step < 10)) So we will use the red ABP icon if the animation step is >= 10? Why not the grey one? http://codereview.adblockplus.org/5196306347720704/diff/5156048109305856/back... background.js:147: // But don't interfere the icon animation if active. s/interfere/interrupt/?
http://codereview.adblockplus.org/5196306347720704/diff/5156048109305856/back... File background.js (right): http://codereview.adblockplus.org/5196306347720704/diff/5156048109305856/back... background.js:117: if (whitelisted && (!animation || animation.step < 10)) On 2014/02/27 22:02:05, Felix H. Dahlke wrote: > So we will use the red ABP icon if the animation step is >= 10? Why not the grey > one? We are neither going to use the red default nor the gray whitelisted icon, but the notification icon at full opacity, which is the same for both states, and is called "abp-[16|19]-notification-[information|critical].png". http://codereview.adblockplus.org/5196306347720704/diff/5156048109305856/back... background.js:147: // But don't interfere the icon animation if active. On 2014/02/27 22:02:05, Felix H. Dahlke wrote: > s/interfere/interrupt/? Don't know. It wouldn't just pause the animation, but inject a wrong frame (showing the default icon) into the animation. Do you call that interrupting or interfering?
http://codereview.adblockplus.org/5196306347720704/diff/5156048109305856/back... File background.js (right): http://codereview.adblockplus.org/5196306347720704/diff/5156048109305856/back... background.js:128: icon += animation.severity; Nit: why split this to two statements? This seems more intuitive: icon += "-notification-" + animation.severity; Same below with animation.step. http://codereview.adblockplus.org/5196306347720704/diff/5156048109305856/back... background.js:147: // But don't interfere the icon animation if active. If we now have a generic function to calculate the icon, why add this special case in the first place? var animation = iconAnimation.tabs.get(tab) ? iconAnimation : null; This can be added to getTabStatus() instead of the animation parameter. And then you will always set the correct icon, animation or not. http://codereview.adblockplus.org/5196306347720704/diff/5156048109305856/icon... File iconAnimation.js (right): http://codereview.adblockplus.org/5196306347720704/diff/5156048109305856/icon... iconAnimation.js:38: }, I don't really understand why registerTab() was removed. What about new tabs being opened while animation is already running?
http://codereview.adblockplus.org/5196306347720704/diff/5156048109305856/back... File background.js (right): http://codereview.adblockplus.org/5196306347720704/diff/5156048109305856/back... background.js:147: // But don't interfere the icon animation if active. On 2014/03/06 14:31:35, Wladimir Palant wrote: > var animation = iconAnimation.tabs.get(tab) ? iconAnimation : null; In fact, this isn't even necessary. If iconAnimation.stop() resets iconAnimation.step to zero then getTabStatus() can just always work with iconAnimation.step. Not sure whether iconAnimation.tabs is even required then.
http://codereview.adblockplus.org/5196306347720704/diff/5156048109305856/back... File background.js (right): http://codereview.adblockplus.org/5196306347720704/diff/5156048109305856/back... background.js:128: icon += animation.severity; On 2014/03/06 14:31:35, Wladimir Palant wrote: > Nit: why split this to two statements? I found it slightly more readable to have one operation per line here. > This seems more intuitive: > > icon += "-notification-" + animation.severity; > > Same below with animation.step. Done. http://codereview.adblockplus.org/5196306347720704/diff/5156048109305856/back... background.js:147: // But don't interfere the icon animation if active. On 2014/03/06 14:31:35, Wladimir Palant wrote: > If we now have a generic function to calculate the icon, why add this special > case in the first place? > > var animation = iconAnimation.tabs.get(tab) ? iconAnimation : null; > > This can be added to getTabStatus() instead of the animation parameter. And then > you will always set the correct icon, animation or not. Done. On 2014/03/06 14:34:33, Wladimir Palant wrote: > On 2014/03/06 14:31:35, Wladimir Palant wrote: > > var animation = iconAnimation.tabs.get(tab) ? iconAnimation : null; > > In fact, this isn't even necessary. If iconAnimation.stop() resets > iconAnimation.step to zero then getTabStatus() can just always work with > iconAnimation.step. Not sure whether iconAnimation.tabs is even required then. The reason this is necessary is that otherwise when you open a new tab while the animation is running, the icon for the new tab will be a still of the current step of the animation.
http://codereview.adblockplus.org/5196306347720704/diff/5156048109305856/back... File background.js (right): http://codereview.adblockplus.org/5196306347720704/diff/5156048109305856/back... background.js:147: // But don't interfere the icon animation if active. On 2014/03/06 20:41:13, Sebastian Noack wrote: > The reason this is necessary is that otherwise when you open a new tab while the > animation is running, the icon for the new tab will be a still of the current > step of the animation. Isn't that exactly the issue that registerTab() was addressing? The animation should be running for new tabs as well. You seem to have overlooked that comment.
http://codereview.adblockplus.org/5196306347720704/diff/5156048109305856/back... File background.js (right): http://codereview.adblockplus.org/5196306347720704/diff/5156048109305856/back... background.js:147: // But don't interfere the icon animation if active. On 2014/03/06 20:49:44, Wladimir Palant wrote: > On 2014/03/06 20:41:13, Sebastian Noack wrote: > > The reason this is necessary is that otherwise when you open a new tab while > the > > animation is running, the icon for the new tab will be a still of the current > > step of the animation. > > Isn't that exactly the issue that registerTab() was addressing? The animation > should be running for new tabs as well. You seem to have overlooked that > comment. Now it is addressed within getTabStatus() as you suggested.
http://codereview.adblockplus.org/5196306347720704/diff/5156048109305856/back... File background.js (right): http://codereview.adblockplus.org/5196306347720704/diff/5156048109305856/back... background.js:147: // But don't interfere the icon animation if active. Still missing something that would start the animation for new tabs (like the registerTab() function you removed).
http://codereview.adblockplus.org/5196306347720704/diff/5156048109305856/back... File background.js (right): http://codereview.adblockplus.org/5196306347720704/diff/5156048109305856/back... background.js:147: // But don't interfere the icon animation if active. On 2014/03/07 22:13:29, Wladimir Palant wrote: > Still missing something that would start the animation for new tabs (like the > registerTab() function you removed). Considering that the animation is interrupted anyway when you switch to an other open tab, I don't know it is worth to keep the animation running while opening a new tab. If we don't want to interrupt the animation when switching tabs, we would have to update the icon always for all tabs. What do you think?
http://codereview.adblockplus.org/5196306347720704/diff/5156048109305856/back... File background.js (right): http://codereview.adblockplus.org/5196306347720704/diff/5156048109305856/back... background.js:147: // But don't interfere the icon animation if active. On 2014/03/08 11:36:39, Sebastian Noack wrote: > Considering that the animation is interrupted anyway when you switch to an other > open tab, I don't know it is worth to keep the animation running while opening > a new tab. Why interrupted? The animation step doesn't change when you open a new tab. So the icon we show for this tab should be correct - if we consider the animation there at all.
http://codereview.adblockplus.org/5196306347720704/diff/5156048109305856/back... File background.js (right): http://codereview.adblockplus.org/5196306347720704/diff/5156048109305856/back... background.js:147: // But don't interfere the icon animation if active. On 2014/03/21 14:10:11, Wladimir Palant wrote: > On 2014/03/08 11:36:39, Sebastian Noack wrote: > > Considering that the animation is interrupted anyway when you switch to an > other > > open tab, I don't know it is worth to keep the animation running while > opening > > a new tab. > > Why interrupted? The animation step doesn't change when you open a new tab. So > the icon we show for this tab should be correct - if we consider the animation > there at all. Since the icon is set per tab, when the active tab changes, the icon changes as well. Of course we could update the icon every time a new tab becomes active. But beside that case, onActivated wouldn't be needed anymore and therefore is going to be removed with http://codereview.adblockplus.org/5464830253203456/. Alternatively we could update the icon for every tab, regardless whether it is visible or not, while animating. Or we could limit the animation to the tabs that were visible when the animation started. This is how it is currently implemented.
Is this still something we want to merge? Considering the last patch set is almost a year old, I'd expect it to have bitrotted.
On 2015/02/05 03:51:38, Felix H. Dahlke wrote: > Is this still something we want to merge? Considering the last patch set is > almost a year old, I'd expect it to have bitrotted. I rebased, or actually rewrote the changes based on the current code. While on it, I also made it a proper module, and further simplified the code. The new patch also doesn't check whether the page is whitelisted on every animation frame anymore. So feel free to review the changes. ;)
http://codereview.adblockplus.org/5196306347720704/diff/5753952654065664/lib/... File lib/icon.js (right): http://codereview.adblockplus.org/5196306347720704/diff/5753952654065664/lib/... lib/icon.js:62: if (animationStep < 10) We're checking for `animationStep < 10` three times in this file - this calls for a constant. http://codereview.adblockplus.org/5196306347720704/diff/5753952654065664/lib/... lib/icon.js:116: function stopIconAnimation() Now that I'm giving this code a good read - don't we have to reset the icon for all tabs (active or inactive) when we call this? How I see it: If a tab becomes inactive during the animation, it's going to get stuck with some intermediate frame. background.js will only update the icon when a new URL is loaded, not when the tab is made active again.
http://codereview.adblockplus.org/5196306347720704/diff/5753952654065664/lib/... File lib/icon.js (right): http://codereview.adblockplus.org/5196306347720704/diff/5753952654065664/lib/... lib/icon.js:62: if (animationStep < 10) On 2015/02/12 04:52:56, Felix H. Dahlke wrote: > We're checking for `animationStep < 10` three times in this file - this calls > for a constant. Done. http://codereview.adblockplus.org/5196306347720704/diff/5753952654065664/lib/... lib/icon.js:116: function stopIconAnimation() On 2015/02/12 04:52:56, Felix H. Dahlke wrote: > Now that I'm giving this code a good read - don't we have to reset the icon for > all tabs (active or inactive) when we call this? To keep things simple we let the current transition finish, when the icon animation is stopped. Otherwise we would need to consider all internal timeouts and intervals used by the animation, clearing them. Moreover, it looks more smooth when the current transition finishes instead resetting the icon immediately. > How I see it: If a tab becomes inactive during the animation, it's going to get > stuck with some intermediate frame. background.js will only update the icon when > a new URL is loaded, not when the tab is made active again. We check for active tabs everytime we start to transition the icon to the notification icon and back. That means if a tab becomes inactive while the transition runs, we just keep running the transition for it. So it doesn't get stuck.
LGTM http://codereview.adblockplus.org/5196306347720704/diff/5753952654065664/lib/... File lib/icon.js (right): http://codereview.adblockplus.org/5196306347720704/diff/5753952654065664/lib/... lib/icon.js:116: function stopIconAnimation() On 2015/02/12 11:15:29, Sebastian Noack wrote: > On 2015/02/12 04:52:56, Felix H. Dahlke wrote: > > Now that I'm giving this code a good read - don't we have to reset the icon > for > > all tabs (active or inactive) when we call this? > > To keep things simple we let the current transition finish, when the icon > animation is stopped. Otherwise we would need to consider all internal timeouts > and intervals used by the animation, clearing them. Moreover, it looks more > smooth when the current transition finishes instead resetting the icon > immediately. > > > How I see it: If a tab becomes inactive during the animation, it's going to > get > > stuck with some intermediate frame. background.js will only update the icon > when > > a new URL is loaded, not when the tab is made active again. > > We check for active tabs everytime we start to transition the icon to the > notification icon and back. That means if a tab becomes inactive while the > transition runs, we just keep running the transition for it. So it doesn't get > stuck. > You're right, I somehow thought we'd abort the transition. The behaviour does make most sense the way it's implemented. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
