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..fa5cf0f0fad02d3613de72c5989b3e2c5edb132f 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
+ * }}

[Dan Beam, 2016/07/12 00:08:42]

nit: /** @typedef {{id: number, rawQuery: ?string, regExp: ?RegExp}} */

[dpapad, 2016/07/12 02:20:28]

Done. I don't think this is consistent with other typedefs though (and I am not
sure if styleguide is mandating about using as few lines as possible).

[Dan Beam, 2016/07/13 01:26:11]

blame
english
for
reading
from
left
to
right
rather
than
top
to
bottom
as
the
first
axis.

[dpapad, 2016/07/13 02:27:01]

[sarcasm follows]
Thanks for the insightful answer, now I know who to blame (apparently Javascript
typedef definition === English free form text). I am now wondering why natural
language processing is considered a hard problem, should be trivial as my new
found knowledge suggests.

+ */
+var SearchContext;
+
 cr.define('settings', function() {
   /** @const {string} */
   var WRAPPER_CSS_CLASS = 'search-highlight-wrapper';
@@ -97,19 +106,48 @@ cr.define('settings', function() {
   }
 
   /**
+   * Checks whether the given |node| requires force rendering.
+   *
+   * @param {!SearchContext} context
+   * @param {!Node} node
+   * @return {boolean} Whether a forced rendering task was scheduled.
+   * @private
+   */
+  function forceRenderNeeded_(context, node) {
+    if (node.nodeName != 'TEMPLATE' ||
+        !node.hasAttribute('name') ||
+        node.if) {

[Dan Beam, 2016/07/12 00:08:42]

nit:

if (node.nodeName != 'TEMPLATE' || !node.hasAttribute('name') || node.if)
  return false;

[dpapad, 2016/07/12 02:20:28]

Done.

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

[Dan Beam, 2016/07/12 00:08:42]

nit:

return node.getAttribute('name').indexOf('site-') != 0;

[dpapad, 2016/07/12 02:20:27]

Done.

+  }
+
+  /**
    * 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 (IGNORED_ELEMENTS.has(node.tagName))
+      if (forceRenderNeeded_(context, node)) {
+        SearchManager.getInstance().queue_.addRenderTask(
+            new RenderTask(context, node));
+        return;
+      }
+
+
+      if (IGNORED_ELEMENTS.has(node.nodeName))
         return;
 
       if (node.nodeType == Node.TEXT_NODE) {
@@ -117,9 +155,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 +177,7 @@ cr.define('settings', function() {
         doSearch(shadowRoot);
     }
 
-    doSearch(page);
+    doSearch(root);
   }
 
   /**
@@ -149,7 +187,7 @@ cr.define('settings', function() {
   function revealParentSection_(node) {

[Dan Beam, 2016/07/12 00:08:43]

why is this used only once?

[dpapad, 2016/07/12 02:20:27]

It is used only once, because it is needed only once ;>
Per discussion, I am not inlining this, because it seems more readable to keep
it separately.

     // Find corresponding SETTINGS-SECTION parent and make it visible.
     var parent = node;
-    while (parent && parent.tagName !== 'SETTINGS-SECTION') {
+    while (parent && parent.nodeName !== 'SETTINGS-SECTION') {
       parent = parent.nodeType == Node.DOCUMENT_FRAGMENT_NODE ?
           parent.host : parent.parentNode;
     }
@@ -158,7 +196,7 @@ cr.define('settings', function() {
   }
 
   /**
-   * @param {!Element} page
+   * @param {!Node} page
    * @param {boolean} visible
    * @private
    */
@@ -169,24 +207,252 @@ 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 = {

[Dan Beam, 2016/07/12 00:08:42]

can you make this clearer that we're rendering dom-ifs in this class name? 
DomIfRenderer or something?

[dpapad, 2016/07/12 02:20:27]

I added a description in the constructor. Given that this is the only "render"
task class in this file, I prefer not to reflect more detail in the class name
(there is no ambiguity within the context of this file).

+    /** @override */
+    exec: function() {
+      var subpageTemplate =
+          this.node['_content'].querySelector('settings-subpage');

[Dan Beam, 2016/07/12 00:08:42]

why can't you use HTMLTemplateElement#content?

https://github.com/google/closure-compiler/blob/f62f12046b063b2fd91ec7a860675...

[dpapad, 2016/07/12 02:20:28]

They are not the same. I don't know the details of why they are not the same (or
whether they are supposed to), but |_content| is what works here, |content| does
not (tried this). Perhaps @michaelpg knows, since I got this code from
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin....

+      subpageTemplate.id = subpageTemplate.id || this.node.getAttribute('name');
+      assert(!this.node.if);
+      this.node.if = true;
+
+      return new Promise(function(resolve, reject) {
+        var parent = this.node.parentNode;
+        parent.async(function() {
+          var renderedNode = parent.querySelector('#' + subpageTemplate.id);
+          // Register a SearchTask for the part of the DOM that was just
+          // rendered.
+          SearchManager.getInstance().queue_.addSearchTask(
+              new SearchTask(this.context, renderedNode));

[Dan Beam, 2016/07/12 00:08:42]

I still think it'd be nice if RenderTask didn't need to know about SearchTask,
but whatever

[dpapad, 2016/07/12 02:20:27]

Acknowledged.

+          resolve();
+        }.bind(this));
+      }.bind(this));
+    },
+  };
+
+  /**
+   * @constructor
+   * @extends {Task}
+   *
+   * @param {!SearchContext} context
+   * @param {!Node} node
+   */
+  function SearchTask(context, node) {

[Dan Beam, 2016/07/12 00:08:42]

can this be a HighlightMatches task?

[dpapad, 2016/07/12 02:20:27]

I find the proposed name confusing, for the following reason. "HighlightMatches"
task implies in my mind that there is no searching involved. It makes me think
that a SearchTask produced the result and a HighlightMatchesTask would take
those results and highlight them. This is not the case. This task actually
searches a subtree and highlights matches as they are found.

If you dislike current name, how about renaming to SubLevelSearchTask, as
opposed to TopLevelSearchTask?

[Dan Beam, 2016/07/13 01:26:11]

I find the existing name confusing that it doesn't mention DOM *mutation* will
be involved
SearchAndHighlightTask, FindAndHighlightTask?

ultimately I do think we may want to batch the times we *mutate* the DOM from
when we *access* it

[dpapad, 2016/07/13 02:27:01]

Renamed to SearchAndHighlightTask. Technically with this change,
TopLevelSearchTask should also be renamed to TopLevelSearchAndHighlight.
The problem we are trying to solve is to not block the UI. Per my understanding,
it does not matter if we separate searching from highlighting to different
tasks. What matters is to do work within a fixed time slot, pause and yield to
the browser, then continue. Interleaving/Separating search and highlight work
seems orthogonal to the overall problem.

+    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);
+
+      var shouldSearch = this.context.regExp !== null;
+      setSectionsVisibility_(this.node, !shouldSearch);
+      if (shouldSearch)
+        findAndHighlightMatches_(this.context, this.node);

[Dan Beam, 2016/07/12 00:08:42]

how is this part different than starting a SearchTask?

[dpapad, 2016/07/12 02:20:27]

SearchTask and TopLevelSearchTask have a small overlap as you discovered. I
thought about removing that overlap, but I decided not to do this. Even though I
could simply post a SearchTask here, the task priorities would get more complex.
Current model is very simple, where there are three types of task and each of
them have a different priority.

I would like to avoid having SearchTask instances that can have different
priorities.

Edit (after our discussion): Let's keep TopLevelSearchTask and SearchTask
separate for this iteration, and experiment with the idea of merging them (as
well as returning early from a SearchTask if time is up) in follow up CLs.

+
+      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 = {
+    /** Drops all tasks. */
+    reset: function() {
+      this.queues_.high.length = 0;
+      this.queues_.middle.length = 0;
+      this.queues_.low.length = 0;
+    },
+
+    /** @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_();
+    },
+
+    /** @param {!RenderTask} task */
+    addRenderTask: function(task) {
+      this.queues_.low.push(task);
+      this.consumePending_();
+    },
+
+    /**
+     * @return {?Task}

[Dan Beam, 2016/07/12 00:08:42]

is this type still correct?

[dpapad, 2016/07/12 02:20:27]

Compiler seems to accept it. I am not 100% if the compiler differentiates
between {?Task} or {undefined|Task}. This is one more reason why I prefer using
null to begin with.

+     * @private
+     */
+    popNextTask_: function() {
+      return this.queues_.high.shift() ||
+          this.queues_.middle.shift() ||
+          this.queues_.low.shift();
+    },
+
+    /**
+     * @param {!Task} task
+     * @return {boolean} Whether the given task is now obsolete (refers to a
+     *     previous search query).
+     * @private
+     */
+    isTaskObsolete_: function(task) {
+      return task.context.id != SearchManager.getInstance().activeContext_.id;
+    },
+
+    /** @private */
+    consumePending_: function() {
+      if (this.activeTask_)
+        return;
+
+      while (1) {
+        this.activeTask_ = this.popNextTask_();
+        if (!this.activeTask_)
+          return;
+
+        if (this.isTaskObsolete_(this.activeTask_)) {
+          // Dropping this task without ever executing it, since a new search
+          // has been issued since this task was queued.
+          continue;
+        }
+
+        window.requestIdleCallback(function() {
+          // Need to check if this task is obsolete again, since a new search
+          // might have been issued after requestIdleCallback() was called.
+          if (this.isTaskObsolete_(this.activeTask_)) {
+            this.activeTask_ = null;
+            this.consumePending_();
+          } else {
+            this.activeTask_.exec().then(function(result) {
+              this.activeTask_ = null;
+              this.consumePending_();
+            }.bind(this));
+          }
+        }.bind(this));
+        return;
+      }
+    },
+  };
+
+  /**
+   * @constructor
+   */
+  var SearchManager = function() {
+    /** @private {!TaskQueue} */
+    this.queue_ = new TaskQueue();
+
+    /** @private {!SearchContext} */
+    this.activeContext_ = {
+      id: 0, rawQuery: null, regExp: null,
+    };

[Dan Beam, 2016/07/12 00:08:42]

nit: fits in 1 line

[dpapad, 2016/07/12 02:20:28]

Done.

+  };
+  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) {

[Dan Beam, 2016/07/12 00:08:42]

why are we doing anything if text == this.activeContext_.rawQuery?

[dpapad, 2016/07/12 02:20:27]

We are not. <cr-toolbar> guarantees that no event is fired if the text did not
change.

+        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/12 00:08:42]

nit: fits in 1 line?

[dpapad, 2016/07/12 02:20:27]

Done.

+
+        // Drop all previously scheduled tasks, since a new search was just
+        // issued.
+        this.queue_.reset();
+      }
+
+      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 {