MD Bookmarks: Implement basis for new data-binding system

MD Bookmarks: Implement basis for new data-binding system

This CL adds two new classes to MD Bookmarks:
- bookmarks.Store, a globally readable datastore which can only be
  modified through predefined actions.
- bookmarks.StoreClient, a Polymer behavior for binding front-end
  Polymer elements to the global Store.

These two classes form the basis for a new one-way dataflow system which
we are building for MD Bookmarks. They are currently unused, but
follow-up CLs will build up the remainder of the functionality (see bug
for details).

BUG=697706
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2731473002
Cr-Commit-Position: refs/heads/master@{#455373}
Committed: https://chromium.googlesource.com/chromium/src/+/0d9346b1e64f41e97cd44a291ae43343bc0edcc7

[tsergeant, 2017-03-02 05:41:55]

Description was changed from

==========
MD Bookmarks: Implement basis for new data-binding system

This CL adds two new classes to MD Bookmarks:
- bookmarks.Store, a globally readable datastore which can only be
  modified through predefined actions.
- bookmarks.StoreClient, a Polymer behavior for binding front-end Polymer
  elements to the global Store.

These are currently unused in the MD Bookmark Manager, however, they will
form the basis of the new one-way dataflow system for the page.

BUG=697706
==========

to

==========
MD Bookmarks: Implement basis for new data-binding system

This CL adds two new classes to MD Bookmarks:
- bookmarks.Store, a globally readable datastore which can only be
  modified through predefined actions.
- bookmarks.StoreClient, a Polymer behavior for binding front-end Polymer
  elements to the global Store.

These are currently unused in the MD Bookmark Manager, however, they will
form the basis of the new one-way dataflow system for the page.

BUG=697706
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[tsergeant, 2017-03-02 05:45:34]

Description was changed from

==========
MD Bookmarks: Implement basis for new data-binding system

This CL adds two new classes to MD Bookmarks:
- bookmarks.Store, a globally readable datastore which can only be
  modified through predefined actions.
- bookmarks.StoreClient, a Polymer behavior for binding front-end Polymer
  elements to the global Store.

These are currently unused in the MD Bookmark Manager, however, they will
form the basis of the new one-way dataflow system for the page.

BUG=697706
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Bookmarks: Implement basis for new data-binding system

This CL adds two new classes to MD Bookmarks:
- bookmarks.Store, a globally readable datastore which can only be
  modified through predefined actions.
- bookmarks.StoreClient, a Polymer behavior for binding front-end
  Polymer elements to the global Store.

These two classes form the basis for a new one-way dataflow system which
we are building for MD Bookmarks. They are currently unused, but
follow-up CLs will build up the remainder of the functionality (see bug
for details).

BUG=697706
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[tsergeant, 2017-03-02 06:01:19]

The CQ bit was checked by tsergeant@chromium.org to run a CQ dry run

[tsergeant, 2017-03-02 06:02:00]

tsergeant@chromium.org changed reviewers:
+ calamity@chromium.org

[commit-bot: I haz the power, 2017-03-02 06:03:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 07:46:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-02 07:46:41]

Dry run: This issue passed the CQ dry run.

[michaelpg, 2017-03-02 09:30:34]

michaelpg@chromium.org changed reviewers:
+ michaelpg@chromium.org

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

Very cool! Hope you don't mind some drive-by comments.

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/browser_...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/browser_...
chrome/browser/browser_resources.grd:257: <include
name="IDR_MD_BOOKMARKS_BOOKMARKS_STORE_JS"
file="resources\md_bookmarks\bookmarks_store.js" type="BINDATA" />
Might be better to leave these out of the shipped binaries until they're used.

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/bookmarks_store.js (right):

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/bookmarks_store.js:1: // Copyright 2016
The Chromium Authors. All rights reserved.
2017 :-)

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/bookmarks_store.js:2: // Use of this
source code is governed by a BSD-style license that can be
:-\ store.js would be a more consistent name (unlike settings, where tons of
files start with "settings_")

maybe `rename s/store/old_store store.*`?

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/bookmarks_store.js:14: /** @type
{BookmarksPageState} */
throughout: !BookmarksPageState for non-null?

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/reducers.js (right):

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/reducers.js:13: 
nit: remove blank line

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/store_client.js (right):

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store_client.js:13: * @implements
{StoreObserver}
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.

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store_client.js:32: * Observe a particular
part of the state tree, updating |localProperty|
nit: Observes

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store_client.js:62: 
nit: remove blank line

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store_client.js:64: var newValue =
observer.storeGetter(newState);
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.

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store_client.js:69: if (oldValue ==
newValue)
nit: mention in one of the function JSDoc comments that this uses object
identity rather than Polymer-style "deep" equality

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/types.js (right):

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/types.js:19:
StoreObserver.prototype.onStateChanged = function(state) {};
nit: newState, as in store_client.js

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

Thanks for reviewing! It's handy to have an extra set of eyes on this.

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/browser_...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/browser_...
chrome/browser/browser_resources.grd:257: <include
name="IDR_MD_BOOKMARKS_BOOKMARKS_STORE_JS"
file="resources\md_bookmarks\bookmarks_store.js" type="BINDATA" />
On 2017/03/02 09:30:35, michaelpg wrote:
> Might be better to leave these out of the shipped binaries until they're used.

We need them in the binary to be able to run the test.

We could mess around with the config to only add the files/run the tests in
Debug, but I'm not sure it's worth it: This will land after branch point, and
then it should only be a week or two before they start being used.

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/bookmarks_store.js (right):

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/bookmarks_store.js:1: // Copyright 2016
The Chromium Authors. All rights reserved.
On 2017/03/02 09:30:35, michaelpg wrote:
> 2017 :-)

Oops, done.

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/bookmarks_store.js:2: // Use of this
source code is governed by a BSD-style license that can be
On 2017/03/02 09:30:35, michaelpg wrote:
> :-\ store.js would be a more consistent name (unlike settings, where tons of
> files start with "settings_")
> 
> maybe `rename s/store/old_store store.*`?

I did previously have the new store live in store.js, and the old one moved to
polymer_store.js. I changed it to this version to reduce the amount of git churn
-- renaming the file like this caused an extra +500/-500 on the diff.

Chris: which version do you prefer?

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/bookmarks_store.js:14: /** @type
{BookmarksPageState} */
On 2017/03/02 09:30:35, michaelpg wrote:
> throughout: !BookmarksPageState for non-null?

This isn't strictly necessary, since BookmarksPageState is already defined as
non-nullable, but I've gone ahead and changed it for consistency/safety.

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/reducers.js (right):

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/reducers.js:13: 
On 2017/03/02 09:30:35, michaelpg wrote:
> nit: remove blank line

Done.

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/store_client.js (right):

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store_client.js:13: * @implements
{StoreObserver}
On 2017/03/02 09:30:36, michaelpg wrote:
> 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.

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?

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store_client.js:32: * Observe a particular
part of the state tree, updating |localProperty|
On 2017/03/02 09:30:36, michaelpg wrote:
> nit: Observes

Done.

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store_client.js:62: 
On 2017/03/02 09:30:36, michaelpg wrote:
> nit: remove blank line

Done.

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store_client.js:64: var newValue =
observer.storeGetter(newState);
On 2017/03/02 09:30:36, michaelpg wrote:
> 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.

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.

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store_client.js:69: if (oldValue ==
newValue)
On 2017/03/02 09:30:36, michaelpg wrote:
> nit: mention in one of the function JSDoc comments that this uses object
> identity rather than Polymer-style "deep" equality

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

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/types.js (right):

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/types.js:19:
StoreObserver.prototype.onStateChanged = function(state) {};
On 2017/03/02 09:30:36, michaelpg wrote:
> nit: newState, as in store_client.js

Done.

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

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/bookmarks_store.js (right):

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/bookmarks_store.js:2: // Use of this
source code is governed by a BSD-style license that can be
On 2017/03/02 23:20:34, tsergeant wrote:
> On 2017/03/02 09:30:35, michaelpg wrote:
> > :-\ store.js would be a more consistent name (unlike settings, where tons of
> > files start with "settings_")
> > 
> > maybe `rename s/store/old_store store.*`?
> 
> I did previously have the new store live in store.js, and the old one moved to
> polymer_store.js. I changed it to this version to reduce the amount of git
churn
> -- renaming the file like this caused an extra +500/-500 on the diff.
> 
> Chris: which version do you prefer?

I don't think it's worth the churn in other parts of the codebase to have a nice
name for such a brief period.. Eventually we'll just be moving this into
store.js anyway, right?

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/store_client.js (right):

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store_client.js:13: * @implements
{StoreObserver}
On 2017/03/02 23:20:35, tsergeant wrote:
> On 2017/03/02 09:30:36, michaelpg wrote:
> > 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.
> 
> 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?

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.

https://codereview.chromium.org/2731473002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/bookmarks_store.js (right):

https://codereview.chromium.org/2731473002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/bookmarks_store.js:60: return;
nit: newline.

Also, when does this happen?

https://codereview.chromium.org/2731473002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/store_client.js (right):

https://codereview.chromium.org/2731473002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store_client.js:43: observe:
function(localProperty, valueGetter) {
Is it possible to check here that localProperty is a Polymer property (or at
least something that's defined on this)? It would be nice to throw something in
the case of a typo etc.

https://codereview.chromium.org/2731473002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store_client.js:62: return;
nit: newline.

When can this happen?

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

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/bookmarks_store.js (right):

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/bookmarks_store.js:2: // Use of this
source code is governed by a BSD-style license that can be
On 2017/03/03 02:29:03, calamity wrote:
> On 2017/03/02 23:20:34, tsergeant wrote:
> > On 2017/03/02 09:30:35, michaelpg wrote:
> > > :-\ store.js would be a more consistent name (unlike settings, where tons
of
> > > files start with "settings_")
> > > 
> > > maybe `rename s/store/old_store store.*`?
> > 
> > I did previously have the new store live in store.js, and the old one moved
to
> > polymer_store.js. I changed it to this version to reduce the amount of git
> churn
> > -- renaming the file like this caused an extra +500/-500 on the diff.
> > 
> > Chris: which version do you prefer?
> 
> I don't think it's worth the churn in other parts of the codebase to have a
nice
> name for such a brief period.. Eventually we'll just be moving this into
> store.js anyway, right?

Yup, store.js is the eventual name. bookmarks_store.js is just a temporary name
for while we have both version in the codebase.

I've discussed this with Chris in person, and we'll keep this temporary name for
simplicity's sake.

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/store_client.js (right):

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store_client.js:13: * @implements
{StoreObserver}
On 2017/03/03 02:29:03, calamity wrote:
> On 2017/03/02 23:20:35, tsergeant wrote:
> > On 2017/03/02 09:30:36, michaelpg wrote:
> > > 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.
> > 
> > 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?
> 
> 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.

Alright, renamed to |watch| and |watches_|

https://codereview.chromium.org/2731473002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/bookmarks_store.js (right):

https://codereview.chromium.org/2731473002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/bookmarks_store.js:60: return;
On 2017/03/03 02:29:03, calamity wrote:
> nit: newline.
> 
> Also, when does this happen?

Mainly in the router, which likes to fire off actions before the page is fully
initialized.

https://codereview.chromium.org/2731473002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/store_client.js (right):

https://codereview.chromium.org/2731473002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store_client.js:43: observe:
function(localProperty, valueGetter) {
On 2017/03/03 02:29:03, calamity wrote:
> Is it possible to check here that localProperty is a Polymer property (or at
> least something that's defined on this)? It would be nice to throw something
in
> the case of a typo etc.

Hmmm. Maybe? Checking if it's a Polymer property of the current element is easy,
so long as it's not defined by a behavior (in which case I think you need to
traverse the entire behavior tree to see how it's defined).

Checking if it's defined on `this` isn't sufficient since Polymer encourages you
to leave properties undefined until they're used.

I'll put in a TODO here to look into this more.

https://codereview.chromium.org/2731473002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store_client.js:62: return;
On 2017/03/03 02:29:03, calamity wrote:
> nit: newline.
> 
> When can this happen?

While initializing <bookmarks-item> or <bookmarks-folder-node>.

The item is created with a specific itemId, which triggers the polymer observer
to call `updateFromStore`. This happens before attached() is called and
this.observers_ is initialized.

To fix this in a nicer way, I've moved observers_ initialization to created(),
which is called before properties are initialized.

[tsergeant, 2017-03-03 05:08:56]

The CQ bit was checked by tsergeant@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-03 05:09:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[calamity, 2017-03-03 05:46:09]

lgtm

https://codereview.chromium.org/2731473002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/store_client.js (right):

https://codereview.chromium.org/2731473002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store_client.js:62: return;
On 2017/03/03 05:08:00, tsergeant wrote:
> On 2017/03/03 02:29:03, calamity wrote:
> > nit: newline.
> > 
> > When can this happen?
> 
> While initializing <bookmarks-item> or <bookmarks-folder-node>.
> 
> The item is created with a specific itemId, which triggers the polymer
observer
> to call `updateFromStore`. This happens before attached() is called and
> this.observers_ is initialized.
> 
> To fix this in a nicer way, I've moved observers_ initialization to created(),
> which is called before properties are initialized.

Wew.

[commit-bot: I haz the power, 2017-03-03 07:21:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-03 07:21:12]

Dry run: This issue passed the CQ dry run.

[michaelpg, 2017-03-03 22:04:57]

Since I've chimed in, LGTM too. I like the changes made to StoreClient.

Do you see this model being useful for other UIs eventually?

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/browser_...
File chrome/browser/browser_resources.grd (right):

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/browser_...
chrome/browser/browser_resources.grd:257: <include
name="IDR_MD_BOOKMARKS_BOOKMARKS_STORE_JS"
file="resources\md_bookmarks\bookmarks_store.js" type="BINDATA" />
On 2017/03/02 23:20:34, tsergeant wrote:
> On 2017/03/02 09:30:35, michaelpg wrote:
> > Might be better to leave these out of the shipped binaries until they're
used.
> 
> We need them in the binary to be able to run the test.
> 
> We could mess around with the config to only add the files/run the tests in
> Debug, but I'm not sure it's worth it: This will land after branch point, and
> then it should only be a week or two before they start being used.

SGTM!

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/bookmarks_store.js (right):

https://codereview.chromium.org/2731473002/diff/20001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/bookmarks_store.js:2: // Use of this
source code is governed by a BSD-style license that can be
On 2017/03/03 05:08:00, tsergeant wrote:
> On 2017/03/03 02:29:03, calamity wrote:
> > On 2017/03/02 23:20:34, tsergeant wrote:
> > > On 2017/03/02 09:30:35, michaelpg wrote:
> > > > :-\ store.js would be a more consistent name (unlike settings, where
tons
> of
> > > > files start with "settings_")
> > > > 
> > > > maybe `rename s/store/old_store store.*`?
> > > 
> > > I did previously have the new store live in store.js, and the old one
moved
> to
> > > polymer_store.js. I changed it to this version to reduce the amount of git
> > churn
> > > -- renaming the file like this caused an extra +500/-500 on the diff.
> > > 
> > > Chris: which version do you prefer?
> > 
> > I don't think it's worth the churn in other parts of the codebase to have a
> nice
> > name for such a brief period.. Eventually we'll just be moving this into
> > store.js anyway, right?
> 
> Yup, store.js is the eventual name. bookmarks_store.js is just a temporary
name
> for while we have both version in the codebase.
> 
> I've discussed this with Chris in person, and we'll keep this temporary name
for
> simplicity's sake.

Also SGTM!

[tsergeant, 2017-03-06 23:43:18]

On 2017/03/03 22:04:57, michaelpg wrote:
> Since I've chimed in, LGTM too. I like the changes made to StoreClient.

Thanks for reviewing!
 
> Do you see this model being useful for other UIs eventually?

I touched on this at the end of the design doc, which is linked from the bug.
Basically, we're currently not planning on making this a general system,
although the core of it has very little which is bookmarks-specific.

The model seems to be most useful when you have a well-defined database which
you want to operate on. Bookmarks fits this well: we have a core database of
Nodes which we want to be able to edit, and then display a particular slice of
that database in the UI.

On the other hand, something like History doesn't really benefit from this
model: The database structure is much simpler, we don't need to modify the
database much, and we always display the whole database at once. Vanilla Polymer
databinding is still sufficient there.

[tsergeant, 2017-03-06 23:43:51]

The CQ bit was checked by tsergeant@chromium.org

[commit-bot: I haz the power, 2017-03-06 23:44:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 02:37:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-07 02:37:11]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[tsergeant, 2017-03-07 22:13:02]

The CQ bit was checked by tsergeant@chromium.org

[commit-bot: I haz the power, 2017-03-07 22:14:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 01:45:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-08 01:45:48]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[tsergeant, 2017-03-08 02:00:03]

The CQ bit was checked by tsergeant@chromium.org

[commit-bot: I haz the power, 2017-03-08 02:00:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 03:29:15]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1488938403585600,
"parent_rev": "e05be844b2562f3c35ada23f782d2c1aea19adf0", "commit_rev":
"0d9346b1e64f41e97cd44a291ae43343bc0edcc7"}

[commit-bot: I haz the power, 2017-03-08 03:30:02]

Description was changed from

==========
MD Bookmarks: Implement basis for new data-binding system

This CL adds two new classes to MD Bookmarks:
- bookmarks.Store, a globally readable datastore which can only be
  modified through predefined actions.
- bookmarks.StoreClient, a Polymer behavior for binding front-end
  Polymer elements to the global Store.

These two classes form the basis for a new one-way dataflow system which
we are building for MD Bookmarks. They are currently unused, but
follow-up CLs will build up the remainder of the functionality (see bug
for details).

BUG=697706
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Bookmarks: Implement basis for new data-binding system

This CL adds two new classes to MD Bookmarks:
- bookmarks.Store, a globally readable datastore which can only be
  modified through predefined actions.
- bookmarks.StoreClient, a Polymer behavior for binding front-end
  Polymer elements to the global Store.

These two classes form the basis for a new one-way dataflow system which
we are building for MD Bookmarks. They are currently unused, but
follow-up CLs will build up the remainder of the functionality (see bug
for details).

BUG=697706
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2731473002
Cr-Commit-Position: refs/heads/master@{#455373}
Committed:
https://chromium.googlesource.com/chromium/src/+/0d9346b1e64f41e97cd44a291ae4...
==========

[commit-bot: I haz the power, 2017-03-08 03:30:03]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/0d9346b1e64f41e97cd44a291ae4...