MD Settings: Second iteration of search within settings.

chrome/browser/resources/settings/search_settings.js

Index: chrome/browser/resources/settings/search_settings.js
diff --git a/chrome/browser/resources/settings/search_settings.js b/chrome/browser/resources/settings/search_settings.js
index 53d5d22986b3799ea336f7df4233775d81e69604..1c1201a739a268bd7741df16cdcadc00182856e8 100644
--- a/chrome/browser/resources/settings/search_settings.js
+++ b/chrome/browser/resources/settings/search_settings.js
@@ -2,6 +2,15 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+/**
+ * @typedef {{
+ *   id: number,
+ *   rawQuery: ?string,
+ *   regExp: ?RegExp
+ * }}
+ */
+var SearchContext;
+
 cr.define('settings', function() {
   /** @const {string} */
   var WRAPPER_CSS_CLASS = 'search-highlight-wrapper';
@@ -97,18 +106,46 @@ cr.define('settings', function() {
   }
 
   /**
+   * Checks whether the given |node| requires force rendering and schedules an
+   * async RenderTask if necessary.
+   *
+   * @param {!SearchContext} context
+   * @param {!Node} node
+   * @return {boolean} Whether a forced rendering task was scheduled.
+   * @private
+   */
+  function forceRenderNeeded_(context, node) {
+    if (node.tagName != 'TEMPLATE' ||

[Dan Beam, 2016/07/11 21:37:04]

if this is truly a node, tagName -> nodeName

tagName is on Element
https://developer.mozilla.org/en-US/docs/Web/API/Element/tagName

[dpapad, 2016/07/11 23:25:32]

Done (here and elsewhere). Wondering why the compiler did not complain.

+        node.getAttribute('name') === null ||

[Dan Beam, 2016/07/11 21:37:05]

!node.hasAttribute('name') ?

[dpapad, 2016/07/11 23:25:32]

Done.

+        node.if) {
+      return false;
+    }
+
+    // TODO(dpapad): Temporarily ignore site-settings because it throws an
+    // assertion error during force-rendering.
+    if (node.getAttribute('name').indexOf('site-') != -1)

[Dan Beam, 2016/07/11 21:37:05]

consider caching the result of getAttribute('name') if you don't end up using
hasAttribute() (or maybe instead of that)

[dpapad, 2016/07/11 23:25:32]

This is meant to be temporary. We should be searching site-settings subpages, so
I am reluctant doing anything special here, since this if block should go away
soon.

+      return false;
+
+    SearchManager.getInstance().queue_.addRenderTask(
+        new RenderTask(context, node));

[Dan Beam, 2016/07/11 21:37:05]

i might make sense to move this scheduling lower

if (forceRenderNeeded_(context, node)) {
  SearchManager.getInstance().queue_.addRenderTask(context, node);
  return;
}

and drop the side effects / make this method const-er.

I know the doc says it might schedule stuff, but I think this would be more
generic and expected.

[dpapad, 2016/07/11 23:25:32]

Done.

+    return true;
+  }
+
+  /**
    * Traverses the entire DOM (including Shadow DOM), finds text nodes that
    * match the given regular expression and applies the highlight UI. It also
    * ensures that <settings-section> instances become visible if any matches
    * occurred under their subtree.
    *
-   * @param {!Element} page The page to be searched, should be either
-   *     <settings-basic-page> or <settings-advanced-page>.
-   * @param {!RegExp} regExp The regular expression to detect matches.
+   * @param {!SearchContext} context
+   * @param {!Node} root The root of the sub-tree to be searched
    * @private
    */
-  function findAndHighlightMatches_(page, regExp) {
+  function findAndHighlightMatches_(context, root) {
     function doSearch(node) {
+      if (forceRenderNeeded_(context, node))
+        return;
+
       if (IGNORED_ELEMENTS.has(node.tagName))
         return;
 
@@ -117,9 +154,9 @@ cr.define('settings', function() {
         if (textContent.length == 0)
           return;
 
-        if (regExp.test(textContent)) {
+        if (context.regExp.test(textContent)) {
           revealParentSection_(node);
-          highlight_(node, textContent.split(regExp));
+          highlight_(node, textContent.split(context.regExp));
         }
         // Returning early since TEXT_NODE nodes never have children.
         return;
@@ -139,7 +176,7 @@ cr.define('settings', function() {
         doSearch(shadowRoot);
     }
 
-    doSearch(page);
+    doSearch(root);
   }
 
   /**
@@ -158,7 +195,7 @@ cr.define('settings', function() {
   }
 
   /**
-   * @param {!Element} page
+   * @param {!Node} page
    * @param {boolean} visible
    * @private
    */
@@ -169,24 +206,234 @@ cr.define('settings', function() {
   }
 
   /**
-   * Performs hierarchical search, starting at the given page element.
+   * @constructor
+   *
+   * @param {!SearchContext} context
+   * @param {!Node} node
+   */
+  function Task(context, node) {
+    /** @protected {!SearchContext} */
+    this.context = context;
+
+    /** @protected {!Node} */
+    this.node = node;
+  }
+
+  Task.prototype = {
+    /**
+     * @abstract
+     * @return {!Promise}
+     */
+    exec: function() {},
+  };
+
+  /**
+   * @constructor
+   * @extends {Task}
+   *
+   * @param {!SearchContext} context
+   * @param {!Node} node
+   */
+  function RenderTask(context, node) {
+    Task.call(this, context, node);
+  }
+
+  RenderTask.prototype = {
+    /** @override */
+    exec: function() {
+      var subpageTemplate =
+          this.node['_content'].querySelector('settings-subpage');
+      subpageTemplate.id = subpageTemplate.id || this.node.getAttribute('name');
+      assert(!this.node.if);
+      this.node.if = true;
+
+      return this.getRenderedNode_(subpageTemplate.id).then(function(node) {
+        // Register a SearchTask for the part of the DOM that was just
+        // rendered.
+        SearchManager.getInstance().queue_.addSearchTask(
+            new SearchTask(this.context, node));
+      }.bind(this));
+    },
+
+    /**
+     * @param {string} id
+     * @return {!Promise<!Node>}
+     */
+    getRenderedNode_: function(id) {

[Dan Beam, 2016/07/11 21:37:05]

what is the point of making this a separate function?

[dpapad, 2016/07/11 23:25:32]

Inlined. It was more readable IMO.

+      var parent = this.node.parentNode;
+      return new Promise(function(resolve, reject) {
+        parent.async(function() {
+          resolve(parent.querySelector('#' + id));
+        });
+      });
+    },
+  };
+
+  /**
+   * @constructor
+   * @extends {Task}
+   *
+   * @param {!SearchContext} context
+   * @param {!Node} node
+   */
+  function SearchTask(context, node) {
+    Task.call(this, context, node);
+  }
+
+  SearchTask.prototype = {
+    /** @override */
+    exec: function() {
+      findAndHighlightMatches_(this.context, this.node);
+      return Promise.resolve();
+    },
+  };
+
+  /**
+   * @constructor
+   * @extends {Task}
+   *
+   * @param {!SearchContext} context
+   * @param {!Node} page
+   */
+  function TopLevelSearchTask(context, page) {
+    Task.call(this, context, page);
+  }
+
+  TopLevelSearchTask.prototype = {
+    /** @override */
+    exec: function() {
+      findAndRemoveHighlights_(this.node);
+
+      if (this.context.regExp === null) {
+        setSectionsVisibility_(this.node, true);
+        return Promise.resolve();
+      }
+
+      setSectionsVisibility_(this.node, false);
+      findAndHighlightMatches_(this.context, this.node);

[Dan Beam, 2016/07/11 21:37:05]

nit: slightly less duplication

var searchActive = this.context.regExp !== null;
setSectionsVisibility_(this.node, !searchActive);
if (searchActive)
  findAndHighlightMatches_(this.context, this.node);

return Promise.resolve();

[dpapad, 2016/07/11 23:25:32]

Done.

+      return Promise.resolve();
+    },
+  };
+
+  /**
+   * @constructor
+   */
+  function TaskQueue() {
+    /**
+     * @private {{
+     *   high: !Array<!Task>,
+     *   middle: !Array<!Task>,
+     *   low: !Array<!Task>
+     * }}
+     */
+    this.queues_ = {high: [], middle: [], low: []};
+
+    /** @private {?Task} */
+    this.activeTask_ = null;
+  }
+
+  TaskQueue.prototype = {
+    /** @param {!TopLevelSearchTask} task */
+    addTopLevelSearchTask: function(task) {
+      this.queues_.high.push(task);
+      this.consumePending_();
+    },
+
+    /** @param {!SearchTask} task */
+    addSearchTask: function(task) {
+      this.queues_.middle.push(task);
+      this.consumePending_();
+    },
+

[Dan Beam, 2016/07/11 21:37:05]

nit: @param here as well

[dpapad, 2016/07/11 23:25:32]

Done.

+    addRenderTask: function(task) {
+      this.queues_.low.push(task);
+      this.consumePending_();
+    },
+
+    /**
+     * @return {?Task}
+     * @private
+     */
+    popNextTask_: function() {
+      return this.queues_.high.shift() ||
+          this.queues_.middle.shift() ||
+          this.queues_.low.shift() || null;

[Dan Beam, 2016/07/11 21:37:05]

i don't really see the point of |null vs |undefined, but OK

[dpapad, 2016/07/11 23:25:32]

Removed null. But since you asked explaining below why null was there.

I should probably write a document instead of repeating my view on null VS
undefined (have explained this in the past). Until I write that doc, the short
version is as follows.

null in JS indicates programmer's intention, whereas undefined can be
intentional or accidental (bug). The latter combined with relaxed truthiness
checks (implicit boolean conversions), like "if (foo) {}" instead of "if (foo
!== null) {}" can hide bugs that otherwise would blow up immediately if null was
used and no implicit boolean conversions.

Ways to accidentally introduce undefined values
1) Typos: example window.helo instead of window.hello
2) Forgetting to return a value from a function.
3) Forgetting to pass an argument to a function.
4) Programmer's error about scope (expecting a variable to be in scope where it
is not).

So to sum up, I prefer having undefined values blow up the code, and use null
were appropriate,
because it increases the chances that a bug like 1-4 will be caught right away.

+    },
+
+    /** @private */
+    consumePending_: function() {
+      if (this.activeTask_)
+        return;
+
+      var manager = SearchManager.getInstance();
+      while (1) {
+        this.activeTask_ = this.popNextTask_();
+        if (!this.activeTask_)
+          return;
+
+        if (this.activeTask_.context.id != manager.activeContext_.id) {

[Dan Beam, 2016/07/11 21:37:05]

why can't we drop all tasks when search() is called with new text?  that'd
simplify this code, right?

[dpapad, 2016/07/11 23:25:32]

Done, see more detailed comment at line 423.

+          // Dropping this task without ever executing it, since a new search
+          // has been issued since this task was submitted.

[Dan Beam, 2016/07/11 21:37:04]

nit: submitted -> queued

[dpapad, 2016/07/11 23:25:32]

Done.

+          continue;
+        }
+
+        window.requestIdleCallback(function() {

[Dan Beam, 2016/07/11 21:37:05]

so if this is called with nothing to do, we still wait for an idle callback. 
why is this better than doing 1 thing and deferring /other/ things like i
suggested?  does this code imply even the search for something to do might take
a while?

[Dan Beam, 2016/07/11 21:42:13]

ignore this if running even 1 task synchronously causes the UI to pause

[dpapad, 2016/07/11 23:25:32]

Yes, a single task could potentially freeze the UI.

But also as discussed offline, let's always requestIdleCallback before calling
exec(). The code is simpler to reason that way IMO, since every call to
consumePending_() behaves the same, whether it was a recursive call, or a direct
call.
The previously proposed version is more complicated to reason about, because
recursive consumePending_() calls happen within an idle callback (and therefore
call exec() in the same idle callback), but direct calls do not call exec() in
an idle callback. This complexity seems unnecessary.

+          this.activeTask_.exec().then(function(result) {
+            this.activeTask_ = null;
+            this.consumePending_();
+          }.bind(this));
+        }.bind(this));
+        break;
+      }
+    },
+  };
+
+  /**
+   * @constructor
+   */
+  var SearchManager = function() {
+    /** @private {!TaskQueue} */
+    this.queue_ = new TaskQueue();
+
+    /** @private {!SearchContext} */
+    this.activeContext_ = {
+      id: 0, rawQuery: null, regExp: null,
+    };
+  };
+  cr.addSingletonGetter(SearchManager);
+
+  SearchManager.prototype = {
+    /**
+     * @param {string} text The text to search for.
+     * @param {!Node} page
+     */
+    search: function(text, page) {
+      if (this.activeContext_.rawQuery != text) {
+        var newId = this.activeContext_.id + 1;
+
+        var regExp = null;
+        // Generate search text by escaping any characters that would be
+        // problematic for regular expressions.
+        var searchText = text.trim().replace(SANITIZE_REGEX, '\\$&');
+        if (searchText.length > 0)
+          regExp = new RegExp('(' + searchText + ')', 'i');
+
+        this.activeContext_ = {
+          id: newId, rawQuery: text, regExp: regExp,
+        };

[Dan Beam, 2016/07/11 21:37:05]

why don't we clear the queue right here?

[dpapad, 2016/07/11 23:25:32]

Done.
This made me discover a small bug that I also fixed. After calling
requestIdleCallback() and before the idle callback is called by the browser, the
activeTask_ might have been obsoleted, so it is necessary to check again the
context id before executing.

+      }
+
+      this.queue_.addTopLevelSearchTask(
+          new TopLevelSearchTask(this.activeContext_, page));
+    },
+  };
+
+  /**
    * @param {string} text
-   * @param {!Element} page Must be either <settings-basic-page> or
-   *     <settings-advanced-page>.
+   * @param {!Node} page
    */
   function search(text, page) {
-    findAndRemoveHighlights_(page);
-
-    // Generate search text by escaping any characters that would be problematic
-    // for regular expressions.
-    var searchText = text.trim().replace(SANITIZE_REGEX, '\\$&');
-    if (searchText.length == 0) {
-      setSectionsVisibility_(page, true);
-      return;
-    }
-
-    setSectionsVisibility_(page, false);
-    findAndHighlightMatches_(page, new RegExp('(' + searchText + ')', 'i'));
+    SearchManager.getInstance().search(text, page);
   }
 
   return {