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

lib/icon.js

Index: lib/icon.js
===================================================================
--- a/lib/icon.js
+++ b/lib/icon.js
@@ -27,6 +27,7 @@
 
 let frameInterval = null;
 let animationInterval = null;
+let onActivated = null;
 let whitelistedState = new ext.PageMap();
 
 function loadImage(url)
@@ -119,30 +120,36 @@
 {
   function playAnimation(frames)

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

If you don't mind, I'd also like to move that function to the top-level. Yes, it
would need an additional argument then. But IMO less nested blocks make this
code somewhat easier to read.

(The reason I didn't go ahead, was that it would make the changes below harder
to review, if doing so in the same patch set)

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

Sure I don't mind if you'd like to do that. (The functions could also use better
names, I just couldn't think of anything better.)

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

Done.

   {
-    let animationStep = 0;

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

Unrelated: We always define variables where we need them, not in any parent
scope.

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

Acknowledged.

     ext.pages.query({active: true}, pages =>
     {
-      function appendActivePage(page)
-      {
+      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);
-      }
-      ext.pages.onActivated.addListener(appendActivePage);
+        setIcon(page, notificationType, opacity, frames);
+      };
+      ext.pages.onActivated.addListener(onActivated);
 
       frameInterval = setInterval(() =>
       {
-        let opacity = frameOpacities[animationStep++];
-        for (let page of pages)
+        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.

         {
-          if (whitelistedState.has(page))
-            setIcon(page, notificationType, opacity, frames);
-        };

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

Unrelated: Removed redundant semicolon.

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

Acknowledged.

+          for (let page of pages)
+          {
+            if (whitelistedState.has(page))
+              setIcon(page, notificationType, opacity, frames);
+          }
+        }
 
         if (animationStep > numberOfFrames)
         {
-          animationStep = 0;

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

Unrelated: Resetting that value is unneeded, as that variable isn't used anymore
after clearing this interval.

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

Acknowledged.

           clearInterval(frameInterval);
-          frameInterval = null;
-          ext.pages.onActivated.removeListener(appendActivePage);
+          ext.pages.onActivated.removeListener(onActivated);
+          frameInterval = onActivated = null;
         }
       }, 100);
     });
@@ -179,7 +186,9 @@
     clearInterval(frameInterval);
   if (animationInterval != null)
     clearInterval(animationInterval);
-  frameInterval = animationInterval = null;
+  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;
 };
 
 /**