Issue 3532 - Improved preformance of icon animation and fixed a memory leak

lib/icon.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 98 matching lines @@
           imageData[size] = context.getImageData(0, 0, size, size);
         }
         frames["" + opacity + whitelisted] = imageData;
       }
     }
 
     return frames;
   });
 }
 
-function runAnimation(notificationType)
-{
-  function playAnimation(frames)
-  {
-    ext.pages.query({active: true}, pages =>
-    {
-      let animationStep = 0;
-      let opacity = 0;
-
-      onActivated = page => {

[kzar, 2016/01/28 09:51:04]

Arrow functions  are for anonymous functions, if it needs a name surely it's
better to use the old syntax? (Also shouldn't there be a newline before the
opening "{"?)

[Sebastian Noack, 2016/01/28 10:14:21]

We have to assign to the global variable onActivated here. So |function
onActivated() {}| wouldn't work. So yes, we need an anonymous function here,
that we assign to a global variable.

(Also shouldn't there be a newline before the
Done.

[kzar, 2016/01/28 10:19:57]

Acknowledged.

-        pages.push(page);
-        setIcon(page, notificationType, opacity, frames);
-      };
-      ext.pages.onActivated.addListener(onActivated);
-
-      frameInterval = setInterval(() =>
+function animateIcon(notificationType, frames)
+{
+  ext.pages.query({active: true}, pages =>
+  {
+    let animationStep = 0;
+    let opacity = 0;
+
+    onActivated = page =>
+    {
+      pages.push(page);
+      setIcon(page, notificationType, opacity, frames);
+    };
+    ext.pages.onActivated.addListener(onActivated);
+
+    frameInterval = setInterval(() =>
+    {
+      let oldOpacity = opacity;
+      opacity = frameOpacities[animationStep++];
+
+      if (opacity != oldOpacity)
       {
-        let oldOpacity = opacity;
-        opacity = frameOpacities[animationStep++];
-
-        if (opacity != oldOpacity)

[Sebastian Noack, 2016/01/27 19:09:20]

On my notebook (X230 with i7), I observed significant CPU usage during the 3
seconds the icon is animating. It was usally around 25% (which is already too
much) and while switching tabs during the animation it got up 90%.

Things didn't look too bad in the profiler (peaks of 1-3ms every 100ms). So I
assume that the bottleneck is in the native code that runs in Chrome's UI thread
after chrome.pageAction.setIcon() has been called.

However, not calling setIcon when the opacity didn't change seems to
significantly improve the performance.

[kzar, 2016/01/28 09:51:04]

Of course, good idea.

+        for (let page of pages)
         {
-          for (let page of pages)
-          {
-            if (whitelistedState.has(page))
-              setIcon(page, notificationType, opacity, frames);
-          }
+          if (whitelistedState.has(page))
+            setIcon(page, notificationType, opacity, frames);
         }
-
-        if (animationStep > numberOfFrames)
-        {
-          clearInterval(frameInterval);
-          ext.pages.onActivated.removeListener(onActivated);
-          frameInterval = onActivated = null;
-        }
-      }, 100);
-    });
-  }
-
+      }
+
+      if (animationStep > numberOfFrames)
+      {
+        clearInterval(frameInterval);
+        ext.pages.onActivated.removeListener(onActivated);
+        frameInterval = onActivated = null;
+      }
+    }, 100);
+  });
+}
+
+function runAnimationLoop(notificationType)
+{
   renderFrames(notificationType).then(frames =>
   {
-    playAnimation(frames);
-    animationInterval = setInterval(() => { playAnimation(frames); }, 10000);
+    animateIcon(notificationType, frames);
+    animationInterval = setInterval(() =>
+    {
+      animateIcon(notificationType, frames);
+    }, 10000);
   });
 }
 
 /**
  * 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
  */
 exports.updateIcon = function(page, whitelisted)
 {
   whitelistedState.set(page, whitelisted);
   if (frameInterval == null)
     setIcon(page);
 };
 
 let stopIconAnimation =
 /**
  * Stops to animate the browser action icon.
  */
 exports.stopIconAnimation = function()
 {
   if (frameInterval != null)
     clearInterval(frameInterval);
   if (animationInterval != null)
     clearInterval(animationInterval);
   if (onActivated)

[Sebastian Noack, 2016/01/27 19:09:20]

Yeah, that was the memory leak mentioned in the issue and review title. This
listener was previously only removed when the animation finished. Not when it
was stopped while animating.

[kzar, 2016/01/28 09:51:04]

Dang, good point. (Note, my stopAnimation closure is now looking increasingly
like a good idea again.)

[Sebastian Noack, 2016/01/28 10:14:21]

(Not really, it doesn't matter where you do the cleanup, as long as you do it)

     ext.pages.onActivated.removeListener(onActivated);
   frameInterval = animationInterval = onActivated = null;
 };
 
 /**
  * Starts to animate the browser action icon to indicate a pending notifcation.
  *
  * @param {string} type  The notification type (i.e: "information" or "critical")
  */
 exports.startIconAnimation = function(type)
 {
   stopIconAnimation();
-  runAnimation(type);
-};
+  runAnimationLoop(type);
+};