MD Bookmarks: Add basic page features to new data-flow system

chrome/browser/resources/md_bookmarks/reducers.js

Index: chrome/browser/resources/md_bookmarks/reducers.js
diff --git a/chrome/browser/resources/md_bookmarks/reducers.js b/chrome/browser/resources/md_bookmarks/reducers.js
index c78731d26b0f72dd8f62660eb421deab6a20e4e6..256644c1eae86e48b359509032127252c33bdc57 100644
--- a/chrome/browser/resources/md_bookmarks/reducers.js
+++ b/chrome/browser/resources/md_bookmarks/reducers.js
@@ -10,6 +10,162 @@
  */
 
 cr.define('bookmarks', function() {
+  var NodeState = {};
+
+  /**
+   * @param {NodeList} nodes
+   * @param {string} id
+   * @param {function(BookmarkNode):BookmarkNode} callback
+   * @return {NodeList}
+   */
+  NodeState.modifyNode_ = function(nodes, id, callback) {
+    var nodeModification = {};
+    nodeModification[id] = callback(nodes[id]);
+    return Object.assign({}, nodes, nodeModification);

[calamity, 2017/03/07 07:27:35]

How does closure understand this?

[tsergeant, 2017/03/08 02:47:10]

I would guess that here Closure sees that Object.assign returns an Object, and
is able to silently convert that to a NodeList, which is a variant of Object
(Object<string, BookmarkNode>).

+  };
+
+  /**
+   * @param {NodeList} nodes
+   * @param {Action} action
+   * @return {NodeList}
+   */
+  NodeState.editBookmark = function(nodes, action) {
+    return NodeState.modifyNode_(nodes, action.id, function(node) {
+      return /** @type {BookmarkNode} */ (
+          Object.assign({}, node, action.changeInfo));

[calamity, 2017/03/07 07:27:35]

While it doesn't understand this.

[tsergeant, 2017/03/08 02:47:10]

But here, it isn't able to convert Object to the specific BookmarkNode record
type.

[calamity, 2017/03/08 06:57:19]

Ech.

+    });
+  };
+
+  /**
+   * @param {NodeList} nodes
+   * @param {Action} action
+   * @return {NodeList}
+   */
+  NodeState.removeBookmark = function(nodes, action) {
+    return NodeState.modifyNode_(nodes, action.parentId, function(node) {
+      var newChildren = node.children.slice();
+      newChildren.splice(action.index, 1);
+      return /** @type {BookmarkNode} */ (
+          Object.assign({}, node, {children: newChildren}));
+    });
+  };
+
+  /**
+   * @param {NodeList} nodes
+   * @param {Action} action
+   * @return {NodeList}
+   */
+  NodeState.updateNodes = function(nodes, action) {
+    switch (action.name) {
+      case 'edit-bookmark':
+        return NodeState.editBookmark(nodes, action);
+      case 'remove-bookmark':
+        return NodeState.removeBookmark(nodes, action);
+      case 'refresh-nodes':
+        return action.nodes;
+      default:
+        return nodes;
+    }
+  };
+
+  var SelectedFolderState = {};
+
+  /**
+   * @param {NodeList} nodes
+   * @param {string} ancestorId
+   * @param {string} childId
+   * @return {boolean}
+   */
+  SelectedFolderState.isAncestorOf = function(nodes, ancestorId, childId) {
+    var currentId = childId;
+    // Work upwards through the tree from child.
+    while (currentId) {
+      if (currentId == ancestorId)
+        return true;
+      currentId = nodes[currentId].parentId;
+    }
+    return false;
+  };
+
+  /**
+   * @param {?string} selectedFolder
+   * @param {Action} action
+   * @param {NodeList} nodes
+   * @return {?string}
+   */
+  SelectedFolderState.updateSelectedFolder = function(
+      selectedFolder, action, nodes) {
+    // TODO(tsergeant): It should not be possible to select a non-folder.

[calamity, 2017/03/07 07:27:35]

What's the plan for this TODO? Are you just going to
assert(nodes[action.id].children)?

[tsergeant, 2017/03/08 02:47:10]

I think that if you try to select a folder that either doesn't exist or isn't an
item, it will return the existing selectedFolder without change.

[calamity, 2017/03/08 06:57:19]

I'd prefer throwing an exception over failing silently since it'll make it
easier to catch. Maybe bailing out in the action creation is a good place since
it won't affect the state at all.

[tsergeant, 2017/03/08 23:17:53]

Yeah, maybe. I have a feeling we want to avoid asserting in reducers to keep
them pure.

+    switch (action.name) {
+      case 'select-folder':
+        return action.id;
+      case 'change-folder-open':
+        // When hiding the selected folder by closing its ancestor, select
+        // that ancestor instead.
+        if (!action.open && selectedFolder &&
+            SelectedFolderState.isAncestorOf(
+                nodes, action.id, selectedFolder)) {
+          return action.id;
+        }
+        return selectedFolder;
+      default:
+        return selectedFolder;
+    }
+  };
+
+  var ClosedFolderState = {};
+
+  /**
+   * @param {ClosedFolderState} state
+   * @param {Action} action
+   * @param {NodeList} nodes
+   * @return {ClosedFolderState}
+   */
+  ClosedFolderState.openAncestorsOf = function(state, action, nodes) {
+    var id = action.id;
+    var modifications = {};
+    var parentId = nodes[id].parentId;
+    while (parentId) {
+      if (state[parentId]) {
+        modifications[parentId] = false;
+      }
+      parentId = nodes[parentId].parentId;
+    }
+
+    return Object.assign({}, state, modifications);
+  };
+
+  /**
+   * @param {ClosedFolderState} state
+   * @param {Action} action
+   * @return {ClosedFolderState}
+   */
+  ClosedFolderState.toggleFolderOpen = function(state, action) {
+    var id = action.id;

[calamity, 2017/03/07 07:27:35]

nit: This var seems superfluous.

[tsergeant, 2017/03/08 02:47:10]

Done.

+    var closed = !action.open;
+    var modification = {};
+    modification[id] = closed;
+
+    return Object.assign({}, state, modification);
+  };
+
+  /**
+   * @param {ClosedFolderState} state
+   * @param {Action} action
+   * @param {NodeList} nodes
+   * @return {ClosedFolderState}
+   */
+  ClosedFolderState.updateClosedFolders = function(state, action, nodes) {
+    switch (action.name) {
+      case 'change-folder-open':
+        return ClosedFolderState.toggleFolderOpen(state, action);
+      case 'select-folder':
+        return ClosedFolderState.openAncestorsOf(state, action, nodes);
+      default:
+        return state;
+    };
+  };
+
   /**
    * Root reducer for the Bookmarks page. This is called by the store in
    * response to an action, and the return value is used to update the UI.
@@ -18,10 +174,19 @@ cr.define('bookmarks', function() {
    * @return {!BookmarksPageState}
    */
   function reduceAction(state, action) {
-    return {};
+    return {
+      nodes: NodeState.updateNodes(state.nodes, action),
+      selectedFolder: SelectedFolderState.updateSelectedFolder(
+          state.selectedFolder, action, state.nodes),
+      closedFolders: ClosedFolderState.updateClosedFolders(
+          state.closedFolders, action, state.nodes),
+    };
   }
 
   return {
     reduceAction: reduceAction,
+    ClosedFolderState: ClosedFolderState,
+    NodeState: NodeState,
+    SelectedFolderState: SelectedFolderState,
   };
 });