MD Settings: First iteration of searching within settings.

chrome/browser/resources/settings/settings_main/settings_main.js

Index: chrome/browser/resources/settings/settings_main/settings_main.js
diff --git a/chrome/browser/resources/settings/settings_main/settings_main.js b/chrome/browser/resources/settings/settings_main/settings_main.js
index 11cd9dd684eebba61b2ff7ec7ffed466e63f3575..5e48d473b14c072c5ba65fd890712ec7c90d8a52 100644
--- a/chrome/browser/resources/settings/settings_main/settings_main.js
+++ b/chrome/browser/resources/settings/settings_main/settings_main.js
@@ -41,6 +41,12 @@ Polymer({
     },
 
     /** @private */
+    whenAdvancedPageReady_: {
+      type: Object,
+      value: new PromiseResolver(),

[michaelpg, 2016/06/27 23:00:04]

i know this is sort of a singleton, but for the sake of testability, set this in
created()

[dpapad, 2016/06/27 23:37:26]

Done.

+    },
+
+    /** @private */
     showAdvancedToggle_: {
       type: Boolean,
       value: true,
@@ -108,4 +114,37 @@ Polymer({
   toggleAdvancedPage_: function() {
     this.fire('toggle-advanced-page', !this.isAdvancedMenuOpen_);
   },
+
+  /** @private */
+  onAdvancedPageReady_: function() {
+    this.whenAdvancedPageReady_.resolve();
+  },
+
+  /**
+   * Navigates to the default search page (if necessary).
+   * @private
+   */
+  ensureInDefaultSearchPage_: function() {
+    if (this.currentRoute.page != 'basic' ||
+        this.currentRoute.section != '' ||
+        this.currentRoute.subpage.length != 0) {
+      this.currentRoute = {page: 'basic', section: '', subpage: [], url: ''};
+    }
+  },
+
+  /**
+   * @param {string} query
+   */
+  searchContents: function(query) {
+    this.ensureInDefaultSearchPage_();
+
+    // Search basic page immediately.
+    settings.search(query, assert(this.$$('settings-basic-page')));
+
+    // Force the advanced page to be rendered and search once ready.
+    this.showAdvancedPage_ = true;

[michaelpg, 2016/06/27 23:00:04]

without using a PromiseResolver:

var searchAdvancedPage = function() {
  settings.search(query, assert(this.$$('settings-advanced-page')));
}.bind(this);

if (!this.showAdvancedPage_) {
  this.showAdvancedPage_ = true;
  setTimeout(searchAdvancedPage);
} else {
  searchAdvancedPage();
}

[dpapad, 2016/06/27 23:37:27]

What is the benefit of not using PromiseResolver here?

The proposed code snippet is not entirely equivalent in behavior. The following
line (146), guarantees that even if the advanced page is already rendered (via a
previous search, or maybe the user navigated manually), the code is yielding to
the message loop before performing a blocking search operation. The proposed
snippet only yields if the advanced page was not already rendered. Also using
setTimeout() instead of Promises feels a bit hackier than using a Promise.

[michaelpg, 2016/06/29 01:18:57]

PromiseResolver is sugar for creating a Promise when you need one but don't have
one. I don't think we need one so I don't think we need to add sugar for one.
Oh. Add a comment describing that intention, then. (To be clear, the desired
behavior is to search the basic page synchronously, then in a separate task,
search the advanced page, even if the advanced page is already visible?)
okay:

  this.showAdvancedPage_ = true;
  this.async(function() {
    settings.search(query, assert(this.$$('settings-advanced-page')));
  });

and you can delete 11 LOC for firing an event *at a known time* to resolve a
promise. All that does is schedule a microtask, the above does the same thing
without unnecessary indirection. (Or use setTimeout if you actually want to
schedule a new task for the next event loop.)

By the way, line 139 synchronously enables the basic page's dom-if but doesn't
wait to perform the search on line 142. Why don't you need a whenBasicPageReady_
construct? Does this work if you trigger it from the About page without having
loaded the Basic page first?

I guess it may work because showBasicPage_ is initialized to true. I wonder
why we do that.

[dpapad, 2016/06/29 18:20:12]

Yes, added a comment for the current intention. Keep in mind that the argument
about
asynchronicity here will be obsolete very soon, as I am asyncifying search
internals
in follow up CLs anyway (even for the basic page).
Changed in favor of setTimeout().

For the record, using this.async() is not equivalent. The advanced page contents
have a series of dom-ifs, and when using this.async()
search happens before the underlying dom-if have been rendered (for example
before settings-private-page ready). When I use the promise (or setTimeout) all
the underlying dom-ifs have been rendered. It can be easily showcased with a few
console.logs, but the bottom line is that this.async() and using a Promise here
are not behaviorally equivalent.
As stated above, changed to setTimeout.
The basic page is always rendered, even if you navigate directly to
chrome://md-settings/advanced. So I don't think that the dom-if around the basic
page and showBasicPage_ have any reason for existence anymore.

[michaelpg, 2016/06/29 19:40:51]

Makes sense. Polymer queues effects like dom-if's rendering and schedules a
microtask to process the queue. A Promise will exec after the entire queue has
been flushed (assuming, I think, that you've created the Promise before Polymer
created the MutationObserver it uses to schedule its own microtasks, but some of
that may be platform-dependent -- hooray for undefined and underspecified
behavior!) This also presumes the template rendering does not trigger more
templates which themselves rely on Promises to complete, which maybe we do (e.g.
for initializing languages).

Ultimately there's enough asynchronicity in Settings (e.g., talking to the
browser process) that you'd be hard-pressed to find the perfect time to run a
search.
The basic page is rendered even when you navigate to the About page? Why?

+    this.whenAdvancedPageReady_.promise.then(function() {
+      settings.search(query, assert(this.$$('settings-advanced-page')));
+    }.bind(this));
+  },
 });