MD Bookmarks: Prevent navigating to invalid folders

chrome/browser/resources/md_bookmarks/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.
 
 /**
  * @fileoverview A singleton datastore for the Bookmarks page. Page state is
  * publicly readable, but can only be modified by dispatching an Action to
  * the store.
  */
 
 cr.define('bookmarks', function() {
   /** @constructor */
   function Store() {
     /** @type {!BookmarksPageState} */
     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_ = [];
   }
 
   Store.prototype = {
     /**
      * @param {!BookmarksPageState} initialState
      */
     init: function(initialState) {
       this.data_ = initialState;
 
       this.queuedActions_.forEach(function(action) {
-        this.reduce_(action);
+        this.handleActionInternal_(action);
       }.bind(this));
 
       this.initialized_ = true;
       this.notifyObservers_(this.data_);
     },
 
     /** @type {!BookmarksPageState} */
     get data() {
       return this.data_;
     },
@@ skipping 11 matching lines @@
     /** @param {!StoreObserver} observer */
     removeObserver: function(observer) {
       var index = this.observers_.indexOf(observer);
       this.observers_.splice(index, 1);
     },
 
     /**
      * 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.reduce_(action);
-      this.notifyObservers_(this.data_);
+      this.handleActionInternal_(action);
     },
 
     /**
-     * @param {Action} 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);
+    },
+
+    /**
+     * @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_);
     },
 
     /**
      * @param {!BookmarksPageState} state
      * @private
      */
     notifyObservers_: function(state) {
       this.observers_.forEach(function(o) {
         o.onStateChanged(state);
       });
     },
   };
 
   cr.addSingletonGetter(Store);
 
   return {
     Store: Store,
   };
 });