MD Settings: Fix CrScrollableBehavior

ui/webui/resources/cr_elements/cr_scrollable_behavior.js

 // Copyright 2016 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.
 
 /**
  * @fileoverview Behavior for scrollable containers with <iron-list>.
  *
  * Any containers with the 'scrollable' attribute set will have the following
  * classes toggled appropriately: can-scroll, is-scrolled, scrolled-to-bottom.
  * These classes are used to style the container div and list elements
@@ skipping 38 matching lines @@
       for (let scrollable of scrollableElements)
         this.updateScroll_(scrollable);
     }.bind(this));
 
     // Listen to the 'scroll' event for each scrollable container.
     for (let scrollable of scrollableElements)
       scrollable.addEventListener('scroll', this.updateScrollEvent_.bind(this));
   },
 
   detached: function() {
-    if (this.intervalId_)
+    if (this.intervalId_ !== undefined)
       clearInterval(this.intervalId_);
   },
 
   /**
    * Called any time the contents of a scrollable container may have changed.
    * This ensures that the <iron-list> contents of dynamically sized
    * containers are resized correctly.
    */
   updateScrollableContents() {
     let nodeList = this.root.querySelectorAll('[scrollable] iron-list');
     // Use setTimeout to avoid initial render / sizing issues.
-    this.intervalId_ = setInterval(function() {
+    if (this.intervalId_ === undefined)

[dpapad, 2016/11/23 01:12:51]

Shouldn't this be the opposite?

if (this.intervalId_ !== undefined)
  return;

such that we return early if this method has already been called?

[stevenjb, 2016/11/23 18:07:01]

Er, ugh, yes. I must already be in vacation mode. I guess this "fixed" the
problem by never running. I'll have to make sure to more carefully test the
cases where we need this logic.
Done.

+      return;
+    this.intervalId_ = window.setInterval(function() {
       let unreadyNodes = [];
       for (let node of nodeList) {
         if (node.parentNode.scrollHeight == 0) {
           unreadyNodes.push(node);
           continue;
         }
         let ironList = /** @type {!IronListElement} */ (node);
         ironList.notifyResize();
       }
-      if (unreadyNodes.length == 0)
-        clearInterval(this.intervalId_);
-      else
+      if (unreadyNodes.length == 0) {
+        window.clearInterval(this.intervalId_);
+        this.intervalId_ = undefined;

[dpapad, 2016/11/23 01:12:51]

Setting something to |undefined| is counter intuitive. |undefined| in JS is the
default value of something that has not been defined yet like "var a;".

Can we use null instead and initialize intervalId_ to null at line 41? |null|
signifies more explicitly that a value has been assigned, unlike |undefined|
which can be the result of various errors (typos for example).

[stevenjb, 2016/11/23 18:07:01]

It seems odd to me to set a Number to null, but I guess it isn't really
different than setting it to undefined (and I have to pacify Closure either
way). Done.

+      } else {
         nodeList = unreadyNodes;
+      }
     }.bind(this), 10);
   },
 
   /**
    * Event wrapper for updateScroll_.
    * @param {!Event} event
    * @private
    */
   updateScrollEvent_(event) {
     let scrollable = /** @type {!HTMLElement} */ (event.target);
     this.updateScroll_(scrollable);
   },
 
   /**
    * This gets called once intially and any time a scrollable container scrolls.
    * @param {!HTMLElement} scrollable
    * @private
    */
   updateScroll_(scrollable) {
     scrollable.classList.toggle(
         'can-scroll', scrollable.clientHeight < scrollable.scrollHeight);
     scrollable.classList.toggle('is-scrolled', scrollable.scrollTop > 0);
     scrollable.classList.toggle(
         'scrolled-to-bottom', scrollable.scrollTop + scrollable.clientHeight >=
             scrollable.scrollHeight);
   },
 };