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..db55c8b9bc5b80c2af5d406604779ab2921296b8 100644
--- a/chrome/browser/resources/settings/search_settings.js
+++ b/chrome/browser/resources/settings/search_settings.js
@@ -2,6 +2,16 @@
 // 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;
+

[Dan Beam, 2016/07/09 01:36:07]

why \n\n?

[dpapad, 2016/07/11 21:18:04]

Done.

+
 cr.define('settings', function() {
   /** @const {string} */
   var WRAPPER_CSS_CLASS = 'search-highlight-wrapper';
@@ -97,18 +107,46 @@ cr.define('settings', function() {
   }
 
   /**
+   * Checks whether the given |node| requires force rendering and schedules an
+   * async RenderTask if necessary.
+   *
+   * @param {!SearchContext} searchContext
+   * @param {!Node} node
+   * @return {boolean} Whether a forced rendering task was scheduled.
+   * @private
+   */
+  function forceRenderNeeded_(searchContext, node) {
+    if (node.tagName != 'TEMPLATE' ||
+        node.getAttribute('name') === null ||
+        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)
+      return false;
+
+    SearchManager.getInstance().queue_.addTask(
+        new RenderTask(searchContext, node));

[Dan Beam, 2016/07/09 01:36:07]

see below about "postExec"


var queue = SearchManager.getInstance().queue_;
var renderTask = new RenderTask(searchContext, node);
renderTask.postExec = function(result) {
  queue.addTask(new SearchTask(searchContext, result));
};
queue.addTask(renderTask);

[dpapad, 2016/07/11 21:18:04]

Addressed in a slightly different (I think simpler) way. Specifically I am
adding the new SearchTask as part of the RenderTask exec(), and avoided
introducing the concept of postTask.

+    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} searchContext
+   * @param {!Node} root The root of the sub-tree to be searched
    * @private
    */
-  function findAndHighlightMatches_(page, regExp) {
+  function findAndHighlightMatches_(searchContext, root) {
     function doSearch(node) {
+      if (forceRenderNeeded_(searchContext, node))
+        return;
+
       if (IGNORED_ELEMENTS.has(node.tagName))
         return;
 
@@ -117,9 +155,9 @@ cr.define('settings', function() {
         if (textContent.length == 0)
           return;
 
-        if (regExp.test(textContent)) {
+        if (searchContext.regExp.test(textContent)) {
           revealParentSection_(node);
-          highlight_(node, textContent.split(regExp));
+          highlight_(node, textContent.split(searchContext.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);
   }
 
   /**
@@ -158,7 +196,7 @@ cr.define('settings', function() {
   }
 
   /**
-   * @param {!Element} page
+   * @param {!Node} page
    * @param {boolean} visible
    * @private
    */
@@ -169,27 +207,251 @@ cr.define('settings', function() {
   }
 
   /**
-   * Performs hierarchical search, starting at the given page element.
-   * @param {string} text
-   * @param {!Element} page Must be either <settings-basic-page> or
-   *     <settings-advanced-page>.
+   * @constructor
+   *
+   * @param {!SearchContext} searchContext
+   * @param {!Node} node
    */
-  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;
-    }
+  function Task(searchContext, node) {
+    /** @protected {!SearchContext} */
+    this.searchContext = searchContext;

[Dan Beam, 2016/07/09 01:36:07]

nit: this.context while there's no other context?

[dpapad, 2016/07/11 21:18:05]

Renamed,
s/searchContext/context
s/activeSearchContext/activeContext

+
+    /** @protected {!Node} */
+    this.node = node;

[Dan Beam, 2016/07/09 01:36:07]

/** @type {function(?):void|undefined} */
this.postExec;

[dpapad, 2016/07/11 21:18:04]

Acknowledged.

+  }
+
+  Task.prototype = {
+    /**
+     * @abstract
+     * @return {!Promise}
+     */
+    exec: function() {},
+  };
 
-    setSectionsVisibility_(page, false);
-    findAndHighlightMatches_(page, new RegExp('(' + searchText + ')', 'i'));
+  /**
+   * @constructor
+   * @extends {Task}
+   *
+   * @param {!SearchContext} searchContext
+   * @param {!Node} node
+   */
+  function RenderTask(searchContext, node) {
+    Task.call(this, searchContext, node);
+  }
+
+  RenderTask.prototype = {
+    /** @override */
+    exec: function() {
+      return new Promise(function(resolve, reject) {
+        var subpageTemplate =
+            this.node._content.querySelector('settings-subpage');

[Dan Beam, 2016/07/09 01:36:07]

_content :(

[dpapad, 2016/07/11 21:18:04]

It gets a bit worse (but don't know of a better alternative). I had to use
quoted notation node_['_content'], to please the compiler.

+        if (!subpageTemplate.id)
+          subpageTemplate.id = this.node.getAttribute('name');
+
+        assert(!this.node.if);
+        this.node.if = true;
+        this.getRenderedNode_(subpageTemplate.id).then(resolve, reject);
+      }.bind(this));

[Dan Beam, 2016/07/09 01:36:07]

var subpageTemplate = this.node._content.querySelector('settings-subpage');
subpageTemplate.id = subpageTemplate.id || this.node.getAttribute('name');

assert(!this.node.if);
this.node.if = true;

var resolver = new PromiseResolver;
this.node.parentNode.async(resolver.resolve);
return resolver.promise;

[dpapad, 2016/07/11 21:18:04]

Done. As said earlier, I am registering a new Search task inside
RenderTask#exec(). I think is fine to make the RenderTask a bit smarter (by
having it refer to SearchManager.getInstance().queue_.addTask here).

+    },
+
+    /**
+     * @param {string} id
+     * @return {!Promise<!Node>}
+     */
+    getRenderedNode_: function(id) {
+      var parent = this.node.parentNode;
+      return new Promise(function(resolve, reject) {
+        parent.async(function() {
+          resolve(parent.querySelector('#' + id));
+        });
+      });
+    },
+  };
+
+  /**
+   * @constructor
+   * @extends {Task}
+   *
+   * @param {!SearchContext} searchContext
+   * @param {!Node} node
+   */
+  function SearchTask(searchContext, node) {
+    Task.call(this, searchContext, node);
+  }
+
+  SearchTask.prototype = {
+    /** @override */
+    exec: function() {
+      return new Promise(function(resolve, reject) {
+        findAndHighlightMatches_(this.searchContext, this.node);
+        resolve();
+      }.bind(this));

[Dan Beam, 2016/07/09 01:36:07]

findAndHighlightMatches_(this.searchContext, this.node);
return Promise.resolve();

[dpapad, 2016/07/11 21:18:05]

Done.

+    },
+  };
+
+  /**
+   * @constructor
+   * @extends {Task}
+   *
+   * @param {!SearchContext} searchContext
+   * @param {!Node} page
+   */
+  function TopLevelSearchTask(searchContext, page) {
+    Task.call(this, searchContext, page);
   }
 
+  TopLevelSearchTask.prototype = {
+    /** @override */
+    exec: function() {
+      return new Promise(function(resolve, reject) {
+        findAndRemoveHighlights_(this.node);
+
+        if (this.searchContext.regExp === null) {
+          setSectionsVisibility_(this.node, true);
+          resolve();
+          return;
+        }
+
+        setSectionsVisibility_(this.node, false);
+        findAndHighlightMatches_(this.searchContext, this.node);
+        resolve();
+      }.bind(this));
+    },
+  };
+
+  /**
+   * @constructor
+   */
+  function TaskQueue() {
+    /** @private {!Array<!Task>} */
+    this.highPriorityQueue_ = [];

[Dan Beam, 2016/07/09 01:36:07]

nit: this.queues_ = {high: [], middle: [], low: []};

[dpapad, 2016/07/11 21:18:04]

Done.

+
+    /** @private {!Array<!Task>} */
+    this.middlePriorityQueue_ = [];
+
+    /** @private {!Array<!Task>} */
+    this.lowPriorityQueue_ = [];
+
+    /** @private {?Task} */
+    this.currentActiveTask_ = null;

[Dan Beam, 2016/07/09 01:36:07]

nit: why both current and active?  just one or the other?

[dpapad, 2016/07/11 21:18:05]

Renamed to activeTask_.

+  }
+
+  TaskQueue.prototype = {
+    /** @param {!Task} task */
+    addTask: function(task) {
+      if (task instanceof TopLevelSearchTask)

[Dan Beam, 2016/07/09 01:36:07]

i think it'd be nice not to do these instanceof checks (i.e.
addHighPriorityTask, addTask(PriorityEnum), if each task actually has its own
|priority| member), but I don't know how slow instanceof actually is

[dpapad, 2016/07/11 21:18:04]

Removed all instanceof checks in favor of three dedicated methods
(addTopLevelSearchTask, addSearchTask, addRenderTask).

+        this.highPriorityQueue_.push(task);
+      else if (task instanceof SearchTask)
+        this.middlePriorityQueue_.push(task);
+      else
+        this.lowPriorityQueue_.push(task);
+      this.consumePending_();
+    },
+
+    /**
+     * @return {?Task}
+     * @private
+     */
+    popNextTask_: function() {
+      var queue = null;
+      if (this.highPriorityQueue_.length > 0)
+        queue = this.highPriorityQueue_;
+      else if (this.middlePriorityQueue_.length > 0)
+        queue = this.middlePriorityQueue_;
+      else if (this.lowPriorityQueue_.length > 0)
+        queue = this.lowPriorityQueue_;
+
+      if (queue === null)
+        return null;
+
+      return queue.splice(0, 1)[0];

[Dan Beam, 2016/07/09 01:36:07]

return this.highPriorityQueue_.shift() || this.middlePriorityQueue_.shift() ||
    this.lowPriorityQueue_.shift();

[dpapad, 2016/07/11 21:18:04]

Done.

+    },
+
+    /** @private */
+    consumePending_: function() {
+      if (this.currentActiveTask_ !== null)
+        return;
+
+      if ((this.currentActiveTask_ = this.popNextTask_()) === null)

[Dan Beam, 2016/07/09 01:36:07]

if (this.currentActiveTask_)
  return;

var manager = SearchManager.getInstance();
for (;;) {
  this.currentActiveTask_ = this.popNextTask_();
  if (!this.currentActiveTask_)
    return;

  if (this.currentActiveTask_.context.id != manager.context.id)
    continue;
  
  this.currentActiveTask_.exec().then(function(result) {
    if (this.currentActiveTask_.postExec)
      this.currentActiveTask_.postExec(result);
    this.currentActiveTask_ = null;
    window.requestIdleCallback(this.consumePending_.bind(this));
  }.bind(this));
  break;
}

[dpapad, 2016/07/11 21:18:05]

Done, with some modifications
1) Using while instead of for.
2) Always calling exec() within requestIdleCallback(). IIUC your suggestion only
does that
   when consumePending_() is called by itself, but not from all the other places
where it is called.
3) There is no postExec() stuff, (see other related comments).

+        return;
+
+      if (this.currentActiveTask_.searchContext.id !==
+          SearchManager.getInstance().activeSearchContext_.id) {
+        // Dropping this task without ever executing it, since a new search has
+        // been issued since this task was submitted.
+        this.currentActiveTask_ = null;
+        this.consumePending_();
+        return;
+      }
+
+      window.requestIdleCallback(function() {
+        this.execTask_(assert(this.currentActiveTask_)).then(function() {
+          this.currentActiveTask_ = null;
+          this.consumePending_();
+        }.bind(this));
+      }.bind(this));
+    },
+
+    /**
+     * @param {!Task} task
+     * @return {!Promise}
+     * @private
+     */
+    execTask_: function(task) {

[Dan Beam, 2016/07/09 01:36:07]

why do we need this if it's only called once?

why do we need to pass |task| if it's the same as |currentActiveTask_|?

[dpapad, 2016/07/11 21:18:04]

Removed. This is a left over from a previous version of the code where I allowed
multiple tasks to run in parallel (no notion of currentActiveTask), so
execTask_() was called with different task objects.

+      return task.exec().then(function(result) {
+        if (task instanceof RenderTask) {
+          // Register a SearchTask for the part of the DOM that was just
+          // rendered.
+          this.addTask(new SearchTask(task.searchContext, result));

[Dan Beam, 2016/07/09 01:36:07]

it'd be nice if the taskqueue didn't need to know what type of tasks it's
running, i.e. we make a postExec member and here we do

if (task.postExec)
  task.postExec();

[dpapad, 2016/07/11 21:18:05]

Done, this part has been removed. TaskQueue is unaware of specific types.

+        }
+      }.bind(this));
+    },
+  };
+

[Dan Beam, 2016/07/09 01:36:07]

nit: why \n\n?

[dpapad, 2016/07/11 21:18:04]

Removed. Initially added because \n\n makes visual parsing easier (IMO).

+
+  /**
+   * @constructor
+   */
+  var SearchManager = function() {
+    /** @private {!TaskQueue} */
+    this.queue_ = new TaskQueue();
+
+    /** @private {!SearchContext} */
+    this.activeSearchContext_ = {
+      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.activeSearchContext_.rawQuery != text) {
+        var newId = this.activeSearchContext_.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.activeSearchContext_ = {
+          id: newId, rawQuery: text, regExp: regExp,
+        };
+      }
+
+      this.queue_.addTask(
+          new TopLevelSearchTask(this.activeSearchContext_, page));
+    },
+  };
+
   return {
-    search: search,
+    search: function(text, page) {
+      SearchManager.getInstance().search(text, page);
+    },
   };
 });