Loading Indicator for Distilled Pages

components/dom_distiller/content/resources/dom_distiller_viewer.js

Index: components/dom_distiller/content/resources/dom_distiller_viewer.js
diff --git a/components/dom_distiller/content/resources/dom_distiller_viewer.js b/components/dom_distiller/content/resources/dom_distiller_viewer.js
index 94299467ef3df0b6ffa129f1ee149d008e58eec8..4ed1e012bd11dc9fd547e3a1b235c3f9647a9fa5 100644
--- a/components/dom_distiller/content/resources/dom_distiller_viewer.js
+++ b/components/dom_distiller/content/resources/dom_distiller_viewer.js
@@ -9,8 +9,13 @@ function addToPage(html) {
 }
 
 function showLoadingIndicator(isLastPage) {
-  document.getElementById('loadingIndicator').className =
-      isLastPage ? 'hidden' : 'visible';
+  if (isLastPage) {

[nyquist, 2014/08/11 20:47:05]

Could this just be something short like this?

  document.getElementById('loadingIndicator').className =
      isLastPage ? 'hidden' : 'visible';
  updateLoadingIndicator(isLastPage);

Or is the ordering of setting the classname and updating the loading indicator
important (since you had different ordering in if and else branches). I could
see that you might have to "create"/update the loading indicator before showing
it.

[sunangel, 2014/08/12 01:56:08]

I think it would be better if it were ordered like this so that the icon is
immediately hidden first and so that by the time you show the loading indicator,
it's already started.
On 2014/08/11 20:47:05, nyquist wrote:

[nyquist, 2014/08/12 22:04:32]

AFAIK the interval only triggers after 600 ms, and not instantly, so I think you
should simplify this. See my previous comment.

[sunangel, 2014/08/12 23:17:55]

Done.

+    document.getElementById('loadingIndicator').className ='hidden';
+    updateLoadingIndicator(isLastPage);
+  } else {
+    updateLoadingIndicator(isLastPage);
+    document.getElementById('loadingIndicator').className ='visible';
+  }
 }
 
 // Maps JS theme to CSS class and then changes body class name.
@@ -26,3 +31,21 @@ function useTheme(theme) {
   }
   document.body.className = cssClass;
 }
+
+var updateLoadingIndicator = function() {
+  var colors = ["red", "yellow", "green", "blue"];
+  return function(isLastPage) {
+    if (!isLastPage && typeof this.colorShuffle == "undefined") {
+      var loader = document.getElementById("loader");
+      if (loader) {
+        var colorIndex = -1;
+        this.colorShuffle = setInterval(function() {
+        colorIndex = (colorIndex + 1) % colors.length;

[nyquist, 2014/08/12 22:04:32]

I think these two lines are missing indentation.

[sunangel, 2014/08/12 23:17:55]

Done.

+        loader.className = colors[colorIndex];
+        }, 600);
+      }
+    } else if (isLastPage && typeof this.colorShuffle != "undefined") {
+      clearInterval(this.colorShuffle);
+    }
+  };
+}();