MD Bookmarks: Integrate new data store with UI elements

MD Bookmarks: Integrate new data store with UI elements

MD Bookmarks is switching to a new data store layer to help with binding
data changes to UI elements. This CL ties the new data store to the
existing UI elements.

Note that the new data store is currently missing a few features:
* Item selection
* Search
* URL routing

These features are broken by this CL (there are no user-visible feature
regressions, since this is all behind a runtime flag), but will be added
back in soon.

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

Review-Url: https://codereview.chromium.org/2735953002
Cr-Commit-Position: refs/heads/master@{#455981}
Committed: https://chromium.googlesource.com/chromium/src/+/a958e2950a29ba6f293d95e3a93c3c0235db2b94

[tsergeant, 2017-03-07 04:57:07]

Description was changed from

==========
MD Bookmarks: Integrate new data store with UI elements

MD Bookmarks is switching to a new data store layer to help with binding
data changes to UI elements. This CL ties the new data store to the
existing UI elements.

Note that the new data store is currently missing a few features:
* Item selection
* Search
* URL routing

These features are broken by this CL (there are no user-visable feature
regressions, since this is all behind a runtime flag), but will be added
back in soon.

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

to

==========
MD Bookmarks: Integrate new data store with UI elements

MD Bookmarks is switching to a new data store layer to help with binding
data changes to UI elements. This CL ties the new data store to the
existing UI elements.

Note that the new data store is currently missing a few features:
* Item selection
* Search
* URL routing

These features are broken by this CL (there are no user-visable feature
regressions, since this is all behind a runtime flag), but will be added
back in soon.

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

[tsergeant, 2017-03-08 23:38:14]

Patchset #2 (id:20001) has been deleted

[tsergeant, 2017-03-09 02:21:15]

Description was changed from

==========
MD Bookmarks: Integrate new data store with UI elements

MD Bookmarks is switching to a new data store layer to help with binding
data changes to UI elements. This CL ties the new data store to the
existing UI elements.

Note that the new data store is currently missing a few features:
* Item selection
* Search
* URL routing

These features are broken by this CL (there are no user-visable feature
regressions, since this is all behind a runtime flag), but will be added
back in soon.

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

to

==========
MD Bookmarks: Integrate new data store with UI elements

MD Bookmarks is switching to a new data store layer to help with binding
data changes to UI elements. This CL ties the new data store to the
existing UI elements.

Note that the new data store is currently missing a few features:
* Item selection
* Search
* URL routing

These features are broken by this CL (there are no user-visible feature
regressions, since this is all behind a runtime flag), but will be added
back in soon.

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

[tsergeant, 2017-03-09 02:21:32]

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

[commit-bot: I haz the power, 2017-03-09 02:21:57]

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

[tsergeant, 2017-03-09 02:35:39]

Patchset #2 (id:40001) has been deleted

[tsergeant, 2017-03-09 02:37:00]

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

[commit-bot: I haz the power, 2017-03-09 02:37:24]

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

[tsergeant, 2017-03-09 02:48:31]

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

[tsergeant, 2017-03-09 02:48:32]

PTAL

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

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

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

Dry run: This issue passed the CQ dry run.

[calamity, 2017-03-09 04:58:57]

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

https://codereview.chromium.org/2735953002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/api_listener.js:30: id,
removeInfo.parentId, removeInfo.index));
I don't feel strongly, but it feels weird that this is expanded but changeInfo
isn't.

https://codereview.chromium.org/2735953002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/api_listener.js:48:
chrome.bookmarks.onImportEnded.addListener(onImportEnded);
*Golf clap*

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

https://codereview.chromium.org/2735953002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/folder_node.js:110: }
It's weird that this essentially does the same thing as above but with a
different control flow...

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

https://codereview.chromium.org/2735953002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.js:20: return [];
Why is this necessary?

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

https://codereview.chromium.org/2735953002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store_client.js:46: * @suppress
{checkTypes}
lol.

https://codereview.chromium.org/2735953002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/md_bookmarks/item_test.js (right):

https://codereview.chromium.org/2735953002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/item_test.js:12: nodes: {
Should this be wrapped in testTree?

https://codereview.chromium.org/2735953002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/md_bookmarks/list_test.js (right):

https://codereview.chromium.org/2735953002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/list_test.js:15: createFolder('3', [],
null),
nit: Drop the null.

https://codereview.chromium.org/2735953002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/list_test.js:41: // TODO(tsergeant): Avoid
overwriting these functions directly.
Is this about using the real backend? If so, make this explicit.

https://codereview.chromium.org/2735953002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/md_bookmarks/test_store.js (right):

https://codereview.chromium.org/2735953002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/test_store.js:24:
this.actions_.push(action);
Is this stack for multi-action UI interactions?

https://codereview.chromium.org/2735953002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/test_store.js:35:
this.observers_.forEach((client) => client.onStateChanged(this.data));
This might want to explain why a deep copy may be necessary.

[tsergeant, 2017-03-09 06:27:20]

Patchset #3 (id:80001) has been deleted

[tsergeant, 2017-03-09 06:27:55]

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

https://codereview.chromium.org/2735953002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/api_listener.js:30: id,
removeInfo.parentId, removeInfo.index));
On 2017/03/09 04:58:57, calamity wrote:
> I don't feel strongly, but it feels weird that this is expanded but changeInfo
> isn't.

I thought the same way at first, but now I think it makes sense: For Removed,
the removeInfo is an explicit set of parameters, while for Changed it's a set of
updates that should be made to the Node. It doesn't really make sense to pull
out changeInfo.url when it might not exist at all.

https://codereview.chromium.org/2735953002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/api_listener.js:48:
chrome.bookmarks.onImportEnded.addListener(onImportEnded);
On 2017/03/09 04:58:57, calamity wrote:
> *Golf clap*

Name your methods very carefully in order to preserve this.

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

https://codereview.chromium.org/2735953002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/folder_node.js:110: }
On 2017/03/09 04:58:57, calamity wrote:
> It's weird that this essentially does the same thing as above but with a
> different control flow...

This actually caught me out earlier ('why do we have both of these? is one of
them unused?')

I've changed hasChildFolder_() to call isFolder_()

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

https://codereview.chromium.org/2735953002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.js:20: return [];
On 2017/03/09 04:58:57, calamity wrote:
> Why is this necessary?

This is a little fix to prevent the flash of an empty card during startup.

Without it, displayedList is undefined during initialization and so the logic to
hide the list doesn't trigger:
    hidden$="[[isEmptyList_(displayedList.length)]]"

I've added a comment to the code to explain this.

https://codereview.chromium.org/2735953002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/md_bookmarks/item_test.js (right):

https://codereview.chromium.org/2735953002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/item_test.js:12: nodes: {
On 2017/03/09 04:58:57, calamity wrote:
> Should this be wrapped in testTree?

Done.

https://codereview.chromium.org/2735953002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/md_bookmarks/list_test.js (right):

https://codereview.chromium.org/2735953002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/list_test.js:15: createFolder('3', [],
null),
On 2017/03/09 04:58:57, calamity wrote:
> nit: Drop the null.

"Just 'createFolder'. It's cleaner."

https://codereview.chromium.org/2735953002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/list_test.js:41: // TODO(tsergeant): Avoid
overwriting these functions directly.
On 2017/03/09 04:58:57, calamity wrote:
> Is this about using the real backend? If so, make this explicit.

My concern was with the fact that overwriting chrome.bookmarks.update like this
leaves the method clobbered for the rest of the test file. I was thinking of
BrowserProxy-style solutions to avoid this.

However, my follow-up CL (https://codereview.chromium.org/2742583003/) ended up
addressing this in a different way, so I've deleted the TODO.

https://codereview.chromium.org/2735953002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/md_bookmarks/test_store.js (right):

https://codereview.chromium.org/2735953002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/test_store.js:24:
this.actions_.push(action);
On 2017/03/09 04:58:57, calamity wrote:
> Is this stack for multi-action UI interactions?

That's the idea. Nothing ended up needing it, though, so I'll remove it.

https://codereview.chromium.org/2735953002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/test_store.js:35:
this.observers_.forEach((client) => client.onStateChanged(this.data));
On 2017/03/09 04:58:57, calamity wrote:
> This might want to explain why a deep copy may be necessary.

Reworded as a TODO. WDYT?

[tsergeant, 2017-03-09 06:28:10]

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

[commit-bot: I haz the power, 2017-03-09 06:28:30]

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-09 07:50:50]

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

[commit-bot: I haz the power, 2017-03-09 07:50:51]

Dry run: This issue passed the CQ dry run.

[calamity, 2017-03-10 03:34:21]

lgtm.

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

https://codereview.chromium.org/2735953002/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/list.js:20: return [];
On 2017/03/09 06:27:54, tsergeant wrote:
> On 2017/03/09 04:58:57, calamity wrote:
> > Why is this necessary?
> 
> This is a little fix to prevent the flash of an empty card during startup.
> 
> Without it, displayedList is undefined during initialization and so the logic
to
> hide the list doesn't trigger:
>     hidden$="[[isEmptyList_(displayedList.length)]]"
> 
> I've added a comment to the code to explain this.

Ah yeah, I thought it might be. Good riddance. I'm so sick of that flash.

https://codereview.chromium.org/2735953002/diff/60001/chrome/test/data/webui/...
File chrome/test/data/webui/md_bookmarks/test_store.js (right):

https://codereview.chromium.org/2735953002/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/test_store.js:35:
this.observers_.forEach((client) => client.onStateChanged(this.data));
On 2017/03/09 06:27:55, tsergeant wrote:
> On 2017/03/09 04:58:57, calamity wrote:
> > This might want to explain why a deep copy may be necessary.
> 
> Reworded as a TODO. WDYT?

Sounds good.

[tsergeant, 2017-03-10 03:42:36]

The CQ bit was checked by tsergeant@chromium.org

[commit-bot: I haz the power, 2017-03-10 03:42:55]

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-10 03:54:19]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489117356562060,
"parent_rev": "32fbfd186648b908ee1f708b6a95f1308f52e308", "commit_rev":
"a958e2950a29ba6f293d95e3a93c3c0235db2b94"}

[commit-bot: I haz the power, 2017-03-10 03:55:13]

Description was changed from

==========
MD Bookmarks: Integrate new data store with UI elements

MD Bookmarks is switching to a new data store layer to help with binding
data changes to UI elements. This CL ties the new data store to the
existing UI elements.

Note that the new data store is currently missing a few features:
* Item selection
* Search
* URL routing

These features are broken by this CL (there are no user-visible feature
regressions, since this is all behind a runtime flag), but will be added
back in soon.

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

to

==========
MD Bookmarks: Integrate new data store with UI elements

MD Bookmarks is switching to a new data store layer to help with binding
data changes to UI elements. This CL ties the new data store to the
existing UI elements.

Note that the new data store is currently missing a few features:
* Item selection
* Search
* URL routing

These features are broken by this CL (there are no user-visible feature
regressions, since this is all behind a runtime flag), but will be added
back in soon.

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

Review-Url: https://codereview.chromium.org/2735953002
Cr-Commit-Position: refs/heads/master@{#455981}
Committed:
https://chromium.googlesource.com/chromium/src/+/a958e2950a29ba6f293d95e3a93c...
==========

[commit-bot: I haz the power, 2017-03-10 03:55:16]

Committed patchset #3 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/a958e2950a29ba6f293d95e3a93c...