MD Bookmarks: Prevent navigating to invalid folders

chrome/browser/resources/md_bookmarks/store.js

Index: chrome/browser/resources/md_bookmarks/store.js
diff --git a/chrome/browser/resources/md_bookmarks/store.js b/chrome/browser/resources/md_bookmarks/store.js
index 077c6db535a2412f8e49c40e9861f914579a0300..069668cdac87519f8b3410f0d84376a57a5acb11 100644
--- a/chrome/browser/resources/md_bookmarks/store.js
+++ b/chrome/browser/resources/md_bookmarks/store.js
@@ -15,7 +15,7 @@ cr.define('bookmarks', function() {
     this.data_ = bookmarks.util.createEmptyState();
     /** @type {boolean} */
     this.initialized_ = false;
-    /** @type {!Array<!Action>} */
+    /** @type {!Array<?Action|DeferredAction>} */

[calamity, 2017/05/02 08:27:15]

This is an abomination of a type.

[tsergeant, 2017/05/03 02:57:31]

Acknowledged.

     this.queuedActions_ = [];
     /** @type {!Array<!StoreObserver>} */
     this.observers_ = [];
@@ -29,7 +29,7 @@ cr.define('bookmarks', function() {
       this.data_ = initialState;
 
       this.queuedActions_.forEach(function(action) {
-        this.reduce_(action);
+        this.handleActionInternal_(action);
       }.bind(this));
 
       this.initialized_ = true;
@@ -61,24 +61,48 @@ cr.define('bookmarks', function() {
      * Transition to a new UI state based on the supplied |action|, and notify
      * observers of the change. If the Store has not yet been initialized, the
      * action will be queued and performed upon initialization.
-     * @param {Action} action
+     * If the action requires work to be performed asynchronously (eg, getting
+     * results from the bookmarks API), it is possible to dispatch a
+     * DeferredAction (of the form `handleAction(function(dispatch) { ... })`).
+     * Inside that function, |dispatch| can be called asynchronously to dispatch
+     * Actions directly to the Store.
+     * @param {?Action|DeferredAction} action
      */
     handleAction: function(action) {
+      if (!action)
+        return;

[calamity, 2017/05/02 08:27:15]

When does this happen?

[tsergeant, 2017/05/03 02:57:31]

It's the null check that replaces the no-op action. Looks like it was duplicated
unnecessarily, though: the same check is below at line 100. I've removed this
one.

+
       if (!this.initialized_) {
         this.queuedActions_.push(action);
         return;
       }
 
+      this.handleActionInternal_(action);
+    },
+
+    /**
+     * @param {?Action|DeferredAction} action
+     */
+    handleActionInternal_: function(action) {
+      if (typeof action == 'function') {

[calamity, 2017/05/02 08:27:15]

So one option is to make everything internally a DeferredAction, and wrap all
normal Actions in a simple function that immediately calls this.reduce_(action).
You can also add a separate handleAction and handleDeferredAction that makes
things more explicit.

Are there good reasons to make the Action vs DeferredAction (sort of)
isomorphism implicit like this? I'm not sure what the general policy is on
multi-type params.

[tsergeant, 2017/05/03 02:57:31]

Hmmm. There is some precedent for passing in parameters as "a thing, or a
function which creates a thing". Eg:

https://cs.chromium.org/chromium/src/ui/webui/resources/js/cr/ui.js?q=typeof....

I think that Closure also has a lot of trouble with "?Action|DeferredAction", so
separating them out probably makes sense in that way too.

I've made the changes you suggest. It's a bit weird that the common case bounces
from handling an Action to a DeferredAction then back to Action, but other than
that I think it's an improvement.

+        action(this.reduce_.bind(this));
+        return;
+      }
+
       this.reduce_(action);
-      this.notifyObservers_(this.data_);
     },
 
     /**
-     * @param {Action} action
+     * @param {?Action} action
      * @private
      */
     reduce_: function(action) {
+      if (!action)
+        return;
+
       this.data_ = bookmarks.reduceAction(this.data_, action);
+      if (this.isInitialized())

[calamity, 2017/05/02 08:27:15]

// Batch notification until after all initialization queuedActions are resolved.

[tsergeant, 2017/05/03 02:57:31]

Done.

+        this.notifyObservers_(this.data_);
     },
 
     /**