MD Bookmarks: Implement basis for new data-binding system

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() {
+  /**
+   * @polymerBehavior
+   * @implements {StoreObserver}

[michaelpg, 2017/03/02 09:30:36]

So, this is a StoreObserver but also has its own [different kind of]
"observers". I wonder if there's less ambiguous language that could work here.

[tsergeant, 2017/03/02 23:20:35]

Hmm, I'm not sure. StoreObserver was only added for closure, so I'm happy to
rename that, but I can't think of anything much better.

I also can't think of anything better for StoreClient.observe
(StoreClient.watch?)

Chris, any ideas?

[calamity, 2017/03/03 02:29:03]

I actually do like watch. It implies to me that it's not the usual observer
pattern we see in Chrome which is useful. Another option is link, ala Polymer,
but that's probably also overloaded.

[tsergeant, 2017/03/03 05:08:00]

Alright, renamed to |watch| and |watches_|

+   */
+  var StoreClient = {
+    attached: function() {
+      /**
+       * @type {!Array<{
+       *   localProperty: string,
+       *   storeGetter: function(BookmarksPageState)
+       * }>}
+       */
+      this.observers_ = [];
+      bookmarks.Store.getInstance().addObserver(this);
+    },
+
+    detached: function() {
+      bookmarks.Store.getInstance().removeObserver(this);
+    },
+
+    /**
+     * Observe a particular part of the state tree, updating |localProperty|

[michaelpg, 2017/03/02 09:30:36]

nit: Observes

[tsergeant, 2017/03/02 23:20:35]

Done.

+     * whenever the state changes. |storeGetter| should be a function which,
+     * given the whole state tree, returns the particular Object that this
+     * Client is interested in. Eg:
+     *   observe('item', (state) => state.nodes[this.itemId]);
+     *
+     * @param {string} localProperty
+     * @param {function(BookmarksPageState)} storeGetter
+     */
+    observe: function(localProperty, storeGetter) {
+      this.observers_.push({
+        localProperty: localProperty,
+        storeGetter: storeGetter,
+      });
+    },
+
+    /**
+     * Helper to dispatch an action to the store, which will update the store
+     * data and then (possibly) flow through to the UI.
+     * @param {Action} action
+     */
+    dispatch: function(action) {
+      bookmarks.Store.getInstance().handleAction(action);
+    },
+
+    /** @param {string} newState */
+    onStateChanged: function(newState) {
+      if (!this.observers_)
+        return;
+      this.observers_.forEach(function(observer) {
+

[michaelpg, 2017/03/02 09:30:36]

nit: remove blank line

[tsergeant, 2017/03/02 23:20:35]

Done.

+        var oldValue = this[observer.localProperty];
+        var newValue = observer.storeGetter(newState);

[michaelpg, 2017/03/02 09:30:36]

I had some trouble understanding what storeGetter was supposed to do. based on
this invocation, seems like it should simply return the desired new value of
|localProperty|. If I'm understanding it correctly, could we use a more obvious
name, e.g. "newPropertyGetter"?

Or maybe update the comment for |observe| to say it returns what |localProperty|
should be updated to when the state changes.

[tsergeant, 2017/03/02 23:20:35]

Hmm, you're right: storeGetter is a weird name (sounds like you're getting the
Store itself). I've changed it to `valueGetter`, and tweaked the comment on
`observe` to hopefully be clearer.

+
+        // 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)

[michaelpg, 2017/03/02 09:30:36]

nit: mention in one of the function JSDoc comments that this uses object
identity rather than Polymer-style "deep" equality

[tsergeant, 2017/03/02 23:20:35]

I've added a note to the comment for `observe`

+          return;
+
+        this[observer.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};
+});