MD Settings: Show/hide spinner when searching starts/ends.

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 1b95ac9b25189d4548692ad0d896d7bc7551b1d6..843c6f9b06e98dd0cee1a574722ab3316751afdd 100644
--- a/chrome/browser/resources/settings/search_settings.js
+++ b/chrome/browser/resources/settings/search_settings.js
@@ -5,6 +5,9 @@
 /** @typedef {{id: number, rawQuery: ?string, regExp: ?RegExp}} */
 var SearchContext;
 
+/** @typedef {{onSearchStatusChanged: function(boolean)}} */
+var SearchObserver;
+
 cr.define('settings', function() {
   /** @const {string} */
   var WRAPPER_CSS_CLASS = 'search-highlight-wrapper';
@@ -358,9 +361,11 @@ cr.define('settings', function() {
         var task = this.popNextTask_();
         if (!task) {
           this.running_ = false;
+          SearchManager.getInstance().observer_.onSearchStatusChanged(false);

[Dan Beam, 2016/07/15 19:23:01]

do you need to null-check here?  (note: I know I started looking at this before
you published, so it's just probably something you didn't add yet)

[dpapad, 2016/07/15 19:41:04]

Done.

           return;
         }
 
+        this.running_ = true;
         window.requestIdleCallback(function() {
           function startNextTask() {
             this.running_ = false;
@@ -389,6 +394,9 @@ cr.define('settings', function() {
 
     /** @private {!SearchContext} */
     this.activeContext_ = {id: 0, rawQuery: null, regExp: null};
+
+    /** @private {?SearchObserver} */
+    this.observer_ = null;
   };
   cr.addSingletonGetter(SearchManager);
 
@@ -396,6 +404,11 @@ cr.define('settings', function() {
   SearchManager.SANITIZE_REGEX_ = /[-[\]{}()*+?.,\\^$|#\s]/g;
 
   SearchManager.prototype = {
+    /** @param {!SearchObserver} observer */
+    setObserver: function(observer) {
+      this.observer_ = observer;

[Dan Beam, 2016/07/15 19:23:01]

i think if there's only 1 of these, maybe just make it a "delegate"?

[dpapad, 2016/07/15 19:41:04]

Per discussion, I changed this to be a simple callback for now. This is likely
to change soon though, for example in next CL we need to communicate to
<settings-main> whether any search results were found, to display a "No results"
message.

+    },
+
     /**
      * @param {string} text The text to search for.
      * @param {!Node} page
@@ -417,6 +430,7 @@ cr.define('settings', function() {
         // Drop all previously scheduled tasks, since a new search was just
         // issued.
         this.queue_.reset();
+        this.observer_.onSearchStatusChanged(true);
       }
 
       this.queue_.addTopLevelSearchTask(
@@ -424,15 +438,12 @@ cr.define('settings', function() {
     },
   };
 
-  /**
-   * @param {string} text
-   * @param {!Node} page
-   */
-  function search(text, page) {
-    SearchManager.getInstance().search(text, page);
+  /** @return {!SearchManager} */
+  function getSearchManager() {

[Dan Beam, 2016/07/15 19:23:01]

maybe just remove this and make callers do:

  settings.SearchManager.getInstance()

it's probably more common?

[dpapad, 2016/07/15 19:41:04]

Acknowledged. I prefer getSearchManager() over SearchManager.getInstance()
because,
a) it is shorter
b) it makes SearchManager constructor private to this file.

+    return SearchManager.getInstance();
   }
 
   return {
-    search: search,
+    getSearchManager: getSearchManager,

[Dan Beam, 2016/07/15 19:23:01]

SearchManager: SearchManager,

[dpapad, 2016/07/15 19:41:04]

Same rationale as before. Exposing SearchManager constructor is unnecessary,
since the intention is to only expose the singleton instance.

   };
 });