Loading Indicator for Distilled Pages

components/dom_distiller/content/resources/dom_distiller_viewer.js

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 function addToPage(html) {
   var div = document.createElement('div');
   div.innerHTML = html;
   document.getElementById('content').appendChild(div);
 }
 
 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.
 // CSS classes must agree with distilledpage.css.
 function useTheme(theme) {
   var cssClass;
   if (theme == "sepia") {
     cssClass = "sepia";
   } else if (theme == "dark") {
     cssClass = "dark";
   } else {
     cssClass = "light";
   }
   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);
+    }
+  };
+}();