MD Bookmarks: Disable 'Open in Incognito Window' when Incognito is disabled

chrome/test/data/webui/md_bookmarks/test_store.js

 // Copyright 2017 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.
 
 suiteSetup(function() {
   cr.define('bookmarks', function() {
     var TestStore = function(data) {
       bookmarks.Store.call(this);
       this.data_ = Object.assign(bookmarks.util.createEmptyState(), data);
       this.initialized_ = true;
 
       this.lastAction_ = null;
       this.acceptInit_ = false;
       this.enableReducers_ = false;
+      this.resolverMap_ = new Map();

[calamity, 2017/05/29 06:52:28]

Type annotation?

[tsergeant, 2017/05/30 00:18:21]

Done.

     };
 
     TestStore.prototype = {
       __proto__: bookmarks.Store.prototype,
 
       init: function(state) {
         if (this.acceptInit_)
           bookmarks.Store.prototype.init.call(this, state);
       },
 
@@ skipping 22 matching lines @@
        * @param {boolean} enabled
        */
       setReducersEnabled: function(enabled) {
         this.enableReducers_ = enabled;
       },
 
       reduce_: function(action) {
         this.lastAction_ = action;
         if (this.enableReducers_)
           bookmarks.Store.prototype.reduce_.call(this, action);
+        if (this.resolverMap_.has(action.name))
+          this.resolverMap_.get(action.name).resolve(action);
       },
 
       /**
        * Notifies UI elements that the store data has changed. When reducers are
        * disabled, tests are responsible for manually changing the data to make
        * UI elements update correctly (eg, tests must replace the whole list
        * when changing a single element).
        */
       notifyObservers: function() {
         this.notifyObservers_(this.data);
       },
 
       // Call in order to accept data from an init call to the TestStore once.
       acceptInitOnce: function() {
         this.acceptInit_ = true;
         this.initialized_ = false;
       },
+
+      /**
+       * Track actions called |name|, allowing that type of action to be waited
+       * for with `waitForAction`.
+       * @param {string} name
+       */
+      expectAction: function(name) {
+        this.resolverMap_.set(name, new PromiseResolver());
+      },
+
+      /**
+       * Returns a Promise that will resolve when an action called |name| is
+       * dispatched. Specifically, it will resolve the first time the action is
+       * seen after `expectAction` is called with that name. To reset the
+       * promise after that, call `expectAction` again.

[calamity, 2017/05/29 06:52:28]

Seems a bit risky to have subsequent calls to waitForAction immediately resolve.
I think it would be better to explicitly clear the map when the promise is
resolved so that subsequent calls to waitForAction fail.

[tsergeant, 2017/05/30 00:18:21]

Doing it this way is necessary to support the following situation:

  expectAction('select')
  dispatchSelect()
  waitForAction('select').then(...)

If the map gets cleared after resolve, this doesn't work when dispatchSelect()
is synchronous, since the promise would be gone by the time waitForAction is
called.

This might work if dispatchSelect is asynchronous, depending on the timing, but
there's plenty of situations where the timing is not really dependable. For
example, in the Policy test, the action is dispatched after the callback to
chrome.bookmarks.getTree().

The alternative is to get the promise before there's any chance of the action
being dispatched:

  waitForAction('select').then(...)
  dispatchSelect()

But this makes tests much harder to read/write.

So, I think having the promise stick around until it is explicitly cleared makes
the tests a bit nicer, and it also roughly matches the API of TestBrowserProxy
from settings.

[calamity, 2017/06/01 04:07:44]

Can you work around the synchronicity issue? (e.g would returning
promise.then(<delete from map>) from waitForAction work?)

My concern is that this API will lead to tests that pass when they shouldn't,
because someone cargo culted from an example with a single wait and it wasn't
obvious that another expect was necessary. I'd prefer a guarantee of failure
when the API isn't used as expected.

I mean, I'm happy to stick with this if there's a case where multiple
waitForActions are needed from one expectAction, but I can't really think of
one.

I think TestBrowserProxy would also benefit from such a constraint.

[tsergeant, 2017/06/01 05:27:16]

Interesting idea to queue up resetting the promise when waitForAction is called.
Done.

+       * @param {string} name
+       * @return {!Promise<!Action>}
+       */
+      waitForAction: function(name) {
+        return this.resolverMap_.get(name).promise;
+      },
     };
 
     return {
       TestStore: TestStore,
     };
   });
 });