MD Settings: Adding some unit tests for <settings-main>.

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

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 /**
  * @fileoverview
  * 'settings-main' displays the selected settings page.
  */
 Polymer({
   is: 'settings-main',
@@ skipping 174 matching lines @@
 
   /**
    * Navigates to the default search page (if necessary).
    * @private
    */
   ensureInDefaultSearchPage_: function() {
     settings.navigateTo(settings.Route.BASIC);
   },
 
   /**
    * @param {string} query

[michaelpg, 2016/07/30 05:38:44]

@return

[dpapad, 2016/08/01 21:23:06]

Done.

    */
   searchContents: function(query) {
+    var promiseResolver = new PromiseResolver();

[michaelpg, 2016/07/30 05:38:44]

this promise/setTimeout business is scary, if we're adding more I'm gonna review
the whole function.

[michaelpg, 2016/07/30 05:38:44]

ok, after looking over it... we're resolving this promise within the same
function, why use a PromiseResolver instead of returning a new Promise directly?

To me, it doesn't look any messier that way and it's one fewer state to keep
track of. Is it just the desire to avoid the extra indent?

[dpapad, 2016/08/01 21:23:06]

Removed PromiseResolver. Yes, it was used to avoid the extra indentation.

+
     this.ensureInDefaultSearchPage_();
     this.toolbarSpinnerActive = true;
 
     // Trigger rendering of the basic and advanced pages and search once ready.
     this.showPages_ = {about: false, basic: true, advanced: true};
 
     setTimeout(function() {
       settings.getSearchManager().search(
           query, assert(this.$$('settings-basic-page')));

[michaelpg, 2016/07/30 05:38:44]

We've gotten into trouble by assuming that doing something asynchronously
automagically means a property is still the same, or the current DOM reflects
the property. In particular, searchContents is triggered by an event which has
no correlation with currentRoute.

I guess these asserts are fine until they break; then we'll have to decide
whether to enforce the page exists here or to stop if the page doesn't exist.

[dpapad, 2016/08/01 21:23:06]

Ack. If the asserts break, we should find a more concrete way of triggering a
dom-if, and being notified when the stamped element is ready, but until then, I
think setTimeout() is good enough.

     }.bind(this), 0);
     setTimeout(function() {

[michaelpg, 2016/07/30 05:38:44]

SearchManager is a singleton, has only one task queue, and calls
requestIdleCallback before starting any serious work, which means everything is
FIFO and async.

So why does settings-main have to use two setTimeouts? We are requesting
everything to be rendered, waiting an event loop, and then queueing tasks --
seems like we can call search() twice in a row with no downside. The DOM
traversals happen inside separate requestIdleCallbacks which means which means
the browser will likely schedule a gap between them[1], and the browser is
likely to run both of these setTimeouts before working on the idle task list
anyway since the timeout is 0.

If I'm missing something, then IMO SearchManager should be updated to work
better so users don't have to throttle it manually.

[1]
https://www.w3.org/TR/requestidlecallback/#dfn-invoke-idle-callbacks-algorithm

[dpapad, 2016/08/01 21:23:06]

Merged the two setTimeouts to a single call.

       settings.getSearchManager().search(
           query, assert(this.$$('settings-advanced-page'))).then(

[michaelpg, 2016/07/30 05:38:44]

this ".then" is okay (I think) because this line actually returns the same
Promise object as the basic search() above, but that's only true because:

1. both these setTimeouts execute before any later setTimeouts, since their time
is as low as possible (now + 0) and setTimeouts with the same time are executed
in order; 
2. nobody else calls search() synchronously with a different query anywhere in
the entire app in this current event loop (including in any .async(fn) or
.async(fn, 0) calls), which you have to grep the rest of the code to know
because SearchManager is a singleton anybody could use;
3. the SearchManager happens to key requests by query, not create a duplicate
request for the current query, and return a singleton promise owned by the
request (none of this is documented in the SearchManager interface)

That's a lot for me to reason about on a Friday night.

Promise.all([search(query, basic), search(query, advanced)]).then() would be
simpler even if it's probably always redundant.

[dpapad, 2016/08/01 21:23:06]

Cleaned up the code a bit to address your points.
I did get a bit confused by your reasoning (mostly about 2). It seems that you
are listing too many assumptions on why this code works. The only assumption I
think was made (and now is enforced with an assert()), was that SearchManager
returns the same Promise if the query text did not change. Let me know if latest
version of this function addresses your concerns.

           function(request) {
             if (!request.finished) {
               // Nothing to do here. A previous search request was canceled
               // because a new search request was issued before the first one
               // completed.
+              promiseResolver.resolve();

[michaelpg, 2016/07/30 05:38:44]

we should just resolve this before the if so we don't have to have the same line
twice. The promise is always resolved regardless of whether this |if| is true.
(A promise's callbacks execute asynchronously after it is resolved.)

[dpapad, 2016/08/01 21:23:06]

Done.

               return;
             }
 
             this.toolbarSpinnerActive = false;
             this.showNoResultsFound_ =
                 !request.isSame('') && !request.didFindMatches();
+            promiseResolver.resolve();
           }.bind(this));
     }.bind(this), 0);
+
+    return promiseResolver.promise;
   },
 
   /**
    * @param {(boolean|undefined)} visibility
    * @return {boolean} True unless visibility is false.
    * @private
    */
   showAdvancedSettings_: function(visibility) {
     return visibility !== false;
   },
 });