MD Bookmarks: Remove deleted nodes from state tree

chrome/browser/resources/md_bookmarks/store_client.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 Defines StoreClient, a Polymer behavior to tie a front-end
  * element to back-end data from the store.
  */
 
 cr.define('bookmarks', function() {
@@ skipping 20 matching lines @@
       bookmarks.Store.getInstance().removeObserver(this);
     },
 
     /**
      * Watches a particular part of the state tree, updating |localProperty|
      * to the return value of |valueGetter| whenever the state changes. Eg, to
      * keep |this.item| updated with the value of a node:
      *   watch('item', (state) => state.nodes[this.itemId]);
      *
      * Note that object identity is used to determine if the value has changed
-     * before updating the UI, rather than Polymer-style deep equality.
+     * before updating the UI, rather than Polymer-style deep equality. If the
+     * getter function returns |undefined|, no changes will propagate to the UI.
      *
      * Typechecking is supressed because this conflicts with
      * Object.prototype.watch, which is a Gecko-only method that is recognized
      * by Closure.
      * @suppress {checkTypes}
      * @param {string} localProperty
      * @param {function(!BookmarksPageState)} valueGetter
      */
     watch: function(localProperty, valueGetter) {
       // TODO(tsergeant): Warn if localProperty is not a defined property.
@@ skipping 14 matching lines @@
 
     /** @param {string} newState */
     onStateChanged: function(newState) {
       this.watches_.forEach(function(watch) {
         var oldValue = this[watch.localProperty];
         var newValue = watch.valueGetter(newState);
 
         // Avoid poking Polymer unless something has actually changed. Reducers
         // must return new objects rather than mutating existing objects, so
         // any real changes will pass through correctly.
-        if (oldValue == newValue)
+        if (oldValue == newValue || newValue == undefined)

[calamity, 2017/04/03 06:54:24]

What case does this prevent? Anything deleted in the nodeList shouldn't be
showing anywhere already, right?

[tsergeant, 2017/04/04 04:42:28]

It comes down to an ordering thing. The sequence of events is

1. <bookmarks-list> creates <bookmarks-item id="X">
2. state.nodes["X"] is removed from the map
3. <bookmarks-item> is notified that the state changed, and sees that the new
value of `state.nodes[this.itemId]` is `undefined`
4. <bookmarks-list> is notified that the state changed, updates the
bookmarks-item to id="Y"
5. <bookmarks-item> updates with its new, defined item.

It's step 3 that's the problem: it is only undefined while the update propagates
fully, but it still causes problems in listeners that expect a defined value.

           return;
 
         this[watch.localProperty] = newValue;
       }.bind(this));
     },
 
     updateFromStore: function() {
       if (bookmarks.Store.getInstance().isInitialized())
         this.onStateChanged(bookmarks.Store.getInstance().data);
     },
 
     /** @return {!BookmarksPageState} */
     getState: function() {
       return bookmarks.Store.getInstance().data;
     },
   };
 
   return {
     StoreClient: StoreClient,
   };
 });