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

Unified Diff: lib/icon.js

Issue 5196306347720704: Issue 1965 - Simplified and fixed missing image for icon animation (Closed)
Patch Set: Rabased and refactored code Created Feb. 8, 2015, 2:25 p.m.
Use n/p to move between diff chunks; N/P to move between comments.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « background.js ('k') | metadata.common » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: lib/icon.js
===================================================================
rename from iconAnimation.js
rename to lib/icon.js
--- a/iconAnimation.js
+++ b/lib/icon.js
@@ -15,93 +15,112 @@
* along with Adblock Plus. If not, see <http://www.gnu.org/licenses/>.
*/
-iconAnimation = {
- _icons: new ext.PageMap(),
- _animatedPages: new ext.PageMap(),
- _step: 0,
+let whitelistedState = new ext.PageMap();
+let notificationType = null;
+let animationInterval = null;
+let animationStep = 0;
- update: function(type)
+function getIconPath(whitelisted)
+{
+ let filename = "icons/abp-$size";
+
+ // If the current page is whitelisted, pick an icon that indicates that
+ // Adblock Plus is disabled, however not when the notification icon has
+ // full opacity, or on Safari where icons are genrally grayscale-only.
+ if (whitelisted && animationStep < 10 && require("info").platform != "safari")
+ filename += "-whitelisted";
+
+ // If the icon is currently animating to indicate a pending notification,
+ // pick the icon for the corresponing notification type and animation frame.
+ if (notificationType && animationStep > 0)
{
- if (type == this._type)
- return;
+ filename += "-notification-" + notificationType;
- if (!this._type)
- this._start();
+ if (animationStep < 10)
+ filename += "-" + animationStep;
+ }
- this._type = type;
- },
- stop: function()
+ return filename + ".png";
+}
+
+function setIcon(page)
+{
+ page.browserAction.setIcon(getIconPath(whitelistedState.get(page)));
+}
+
+function runAnimation()
+{
+ return setInterval(function()
{
- clearInterval(this._interval);
+ ext.pages.query({active: true}, function(pages)
+ {
+ let fadeInInterval = setInterval(function()
+ {
+ animationStep++;
+ pages.forEach(setIcon);
- delete this._interval;
- delete this._type;
-
- this._animatedPages.clear();
- },
- registerPage: function(page, icon)
- {
- this._icons.set(page, icon);
-
- if (this._animatedPages.has(page))
- this._updateIcon(page);
- },
- _start: function()
- {
- this._interval = setInterval(function()
- {
- ext.pages.query({active: true}, function(pages)
- {
- if (pages.length == 0)
+ if (animationStep < 10)
Felix Dahlke 2015/02/12 04:52:56 We're checking for `animationStep < 10` three time
Sebastian Noack 2015/02/12 11:15:29 Done.
return;
- for (var i = 0; i < pages.length; i++)
- this._animatedPages.set(pages[i], null);
+ setTimeout(function()
+ {
+ let fadeOutInterval = setInterval(function()
+ {
+ animationStep--;
+ pages.forEach(setIcon);
- var interval = setInterval(function()
- {
- this._step++;
- pages.forEach(this._updateIcon.bind(this));
+ if (animationStep > 0)
+ return;
- if (this._step < 10)
- return;
+ clearInterval(fadeOutInterval);
+ }, 100);
+ },1000);
- clearInterval(interval);
- setTimeout(function()
- {
- interval = setInterval(function()
- {
- this._step--;
- pages.forEach(this._updateIcon.bind(this));
+ clearInterval(fadeInInterval);
+ }, 100);
+ });
+ }, 15000);
+}
- if (this._step > 0)
- return;
+/**
+ * Set the browser action icon for the given page, indicating whether
+ * adblocking is active there, and considering the icon animation.
+ *
+ * @param {Page} [page] The page to set the browser action icon for
+ * @param {Boolean} [whitelisted] Whether the page has been whitelisted
+ */
+function updateIcon(page, whitelisted)
+{
+ page.browserAction.setIcon(getIconPath(whitelisted));
+ whitelistedState.set(page, whitelisted);
+}
+exports.updateIcon = updateIcon;
- clearInterval(interval);
- this._animatedPages.clear();
- }.bind(this), 100);
- }.bind(this), 1000);
- }.bind(this), 100);
- }.bind(this));
- }.bind(this), 15000);
- },
- _updateIcon: function(page)
+/**
+ * Starts to animate the browser action icon to indicate a pending notifcation.
+ *
+ * @param {string} [type] The notifaction type (i.e: "information" or "critical")
+ */
+function startIconAnimation(type)
+{
+ notificationType = type;
+
+ if (animationInterval == null)
+ animationInterval = runAnimation();
+}
+exports.startIconAnimation = startIconAnimation;
+
+/**
+ * Stops to animate the browser action icon.
+ */
+function stopIconAnimation()
Felix Dahlke 2015/02/12 04:52:56 Now that I'm giving this code a good read - don't
Sebastian Noack 2015/02/12 11:15:29 To keep things simple we let the current transitio
Felix Dahlke 2015/02/12 14:14:30 You're right, I somehow thought we'd abort the tra
+{
+ if (animationInterval != null)
{
- var path = this._icons.get(page);
+ clearInterval(animationInterval);
+ animationInterval = null;
+ }
- if (!path)
- return;
-
- if (this._step > 0)
- {
- var suffix = "-notification-" + this._type;
-
- if (this._step < 10)
- suffix += "-" + this._step;
-
- path = path.replace(/(?=\..+$)/, suffix);
- }
-
- page.browserAction.setIcon(path);
- }
-};
+ notificationType = null;
+}
+exports.stopIconAnimation = stopIconAnimation;
« no previous file with comments | « background.js ('k') | metadata.common » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld