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

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

Index: chrome/test/data/webui/md_bookmarks/test_store.js
diff --git a/chrome/test/data/webui/md_bookmarks/test_store.js b/chrome/test/data/webui/md_bookmarks/test_store.js
index 10fab150dd8ad5be18aa6e94106125c4a714deb4..44b6573a8dec3a14b0455aaa9606fa7df77b9ba9 100644
--- a/chrome/test/data/webui/md_bookmarks/test_store.js
+++ b/chrome/test/data/webui/md_bookmarks/test_store.js
@@ -12,6 +12,7 @@ suiteSetup(function() {
       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 = {
@@ -54,6 +55,8 @@ suiteSetup(function() {
         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);
       },
 
       /**
@@ -71,6 +74,27 @@ suiteSetup(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 {