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..aa8f4b9d9de77887eb55a7bcb0ac1023eed0f7dc 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<DeferredAction>} */
     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;
@@ -58,27 +58,54 @@ 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
+     * Handles a 'deferred' action, which can asynchronously dispatch actions
+     * to the Store in order to reach a new UI state. DeferredActions have the
+     * form `handleDeferredAction(function(dispatch) { ... })`). Inside that

[calamity, 2017/05/03 07:49:46]

Should this really be called dispatch? It's a bit confusing because this
directly calls reduce, and dispatch is an overloaded term due to StoreClient.

[tsergeant, 2017/05/04 01:08:49]

If anything, I think it's confusing that the function names are different
between Store and StoreClient. I would prefer to rename handleAction and
handleDeferredAction to match the names from StoreClient.

This way, 'dispatch' always refers to 'sending an action to the store', it's
just that there's a few different contexts where that can happen. No matter
where you are, if you call a function called 'dispatch', you're going to end up
triggering 'reduce' and 'notifyObservers'.

I've uploaded a new patchset with this change. WDYT?

[calamity, 2017/05/04 07:14:26]

Hmm. I guess now that all DeferredActions go through dispatchAsync, this feels
better. It was worse when one version of dispatch took an Action|DeferredAction
but the callback version only took an Action.

Are there any possible future use cases where queuedActions gets used after
initialization? Like, pausing the action queue? The callback dispatch will skip
that. Is it unreasonable to expect clients to use StoreClient's dispatch rather
than passing it?

I don't actually mind here right now. This seems fine, but it just seems
uncomfortable to have two callback-y things that do very subtly different
things.

[tsergeant, 2017/05/04 07:25:37]

That's a fair point. Injecting the dispatch method is necessary to make Closure
happy when writing deferred actions in actions.js. I'll leave it as it is for
now, but it's something to keep in mind if we add any more functionality to the
Store.

+     * function, the |dispatch| callback can be called asynchronously to
+     * dispatch Actions directly to the Store.

[calamity, 2017/05/03 07:49:46]

So clients can totally mess this up and send bogus actions if the state changes
out from underneath them, right?

I guess we need to be careful about what can happen before |dispatch| gets
called.

[tsergeant, 2017/05/04 01:08:50]

Yeah. This isn't a problem for any actions at the moment, but it could come up
in the future.

Redux gets around this by passing two functions to the DeferredAction:

return function(dispatch, getState) {

}

where getState is a function which returns the current state.

We could probably make that happen fairly easily if we need it.

[calamity, 2017/05/04 07:14:26]

Acknowledged.

+     * @param {DeferredAction} action
      */
-    handleAction: function(action) {
+    handleDeferredAction: function(action) {
       if (!this.initialized_) {
         this.queuedActions_.push(action);
         return;
       }
 
-      this.reduce_(action);
-      this.notifyObservers_(this.data_);
+      this.handleActionInternal_(action);
+    },
+
+    /**
+     * 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
+     */
+    handleAction: function(action) {
+      this.handleDeferredAction(function(dispatch) {
+        dispatch(action);
+      });
+    },
+
+    /**
+     * @param {DeferredAction} action
+     */
+    handleActionInternal_: function(action) {
+      action(this.reduce_.bind(this));

[calamity, 2017/05/03 07:49:46]

nit: May as well inline this now.

[tsergeant, 2017/05/04 01:08:49]

It's still called from multiple places, so I'd prefer to leave it as a separate
function.

     },
 
     /**
-     * @param {Action} action
+     * @param {?Action} action
      * @private
      */
     reduce_: function(action) {
+      if (!action)
+        return;
+
       this.data_ = bookmarks.reduceAction(this.data_, action);
+      // Batch notifications until after all initialization queuedActions are
+      // resolved.
+      if (this.isInitialized())
+        this.notifyObservers_(this.data_);
     },
 
     /**