MD Bookmarks: Implement search and selection in new data flow system

MD Bookmarks: Implement search in new data flow system

MD Bookmarks has switched to a new data flow system which makes it
easier to bind and modify backend data from the UI. This CL implements
search in this new system.

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

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

[tsergeant, 2017-03-09 03:53:37]

Description was changed from

==========
MD Bookmarks: Implement search and selection in new data flow system

MD Bookmarks has switched to a new data flow system which makes it
easier to bind and modify backend data from the UI. This CL implements
search and item selection in this new system.

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

to

==========
MD Bookmarks: Implement search and selection in new data flow system

MD Bookmarks has switched to a new data flow system which makes it
easier to bind and modify backend data from the UI. This CL implements
search and item selection in this new system.

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

[tsergeant, 2017-03-10 04:39:27]

Patchset #2 (id:20001) has been deleted

[tsergeant, 2017-03-10 04:47:58]

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

[commit-bot: I haz the power, 2017-03-10 04:48:17]

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-10 04:48:17]

Patchset #2 (id:40001) has been deleted

[tsergeant, 2017-03-10 04:52:43]

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

[tsergeant, 2017-03-10 04:52:44]

PTAL, this is the last big patch.

[commit-bot: I haz the power, 2017-03-10 06:06:12]

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

[commit-bot: I haz the power, 2017-03-10 06:06:12]

Dry run: This issue passed the CQ dry run.

[tsergeant, 2017-03-12 23:55:04]

Description was changed from

==========
MD Bookmarks: Implement search and selection in new data flow system

MD Bookmarks has switched to a new data flow system which makes it
easier to bind and modify backend data from the UI. This CL implements
search and item selection in this new system.

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

to

==========
MD Bookmarks: Implement search in new data flow system

MD Bookmarks has switched to a new data flow system which makes it
easier to bind and modify backend data from the UI. This CL implements
search in this new system.

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

[tsergeant, 2017-03-12 23:55:34]

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

[commit-bot: I haz the power, 2017-03-12 23:55:49]

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-12 23:56:24]

Monday morning update: I realised it was silly to lump these two features
together in one CL, when they're almost entirely independent.

This one now just does search, and I'll send out the CL for selection later.

[commit-bot: I haz the power, 2017-03-13 01:16:28]

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

[commit-bot: I haz the power, 2017-03-13 01:16:29]

Dry run: This issue passed the CQ dry run.

[calamity, 2017-03-13 04:50:36]

https://codereview.chromium.org/2740863003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/actions.js (right):

https://codereview.chromium.org/2740863003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/actions.js:80: return {name:
'clear-search'};
I feel like this should be its own explicit action even if it's never invoked
from UI directly. I'd prefer 1 possible Action name per Action maker.

https://codereview.chromium.org/2740863003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/app.js (right):

https://codereview.chromium.org/2740863003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/app.js:45:
this.dispatch(bookmarks.actions.setSearchResults(ids));
If we're going to do search results by id, we need to worry about whether the
node will exist...

What happens when you delete a bookmark showing on the search results page, for
example? Is it possible for bad timing to cause a node id that's been deleted to
arrive in the frontend through search?

I think we should either handle the case where results don't exist in the node
map or make sure that it can never happen.

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

https://codereview.chromium.org/2740863003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/reducers.js:108: Object.assign({}, node,
{children: newChildren}));
Was this supposed to have a TODO to remove the node from the nodemap?

https://codereview.chromium.org/2740863003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/reducers.js:173: return
nodes['0'].children[0];
Add a TODO to make this return to the previously selected folder?

https://codereview.chromium.org/2740863003/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/md_bookmarks/reducers_test.js (right):

https://codereview.chromium.org/2740863003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/reducers_test.js:148: action =
bookmarks.actions.selectFolder('0');
Hmm. Can we select a real folder here. The root isn't selectable in the real
world. We should probably actually throw something if the root folder gets
selected.

https://codereview.chromium.org/2740863003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/reducers_test.js:149: nextState =
bookmarks.reduceAction(initialState, action);
Initialize nextState with initialState so that this line isn't subtly different.
In fact, it might be a good cargo-cultable pattern to just have 'var state;' at
the top level and then have state = initialState in the setup. (Preferably apply
this to all tests that use an entire state.)

https://codereview.chromium.org/2740863003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/reducers_test.js:164: // UI changes once
search results arrive:
nit: Newlines before these comments.

https://codereview.chromium.org/2740863003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/reducers_test.js:180: var selectedState =
bookmarks.reduceAction(searchedState, action);
Cool.

[tsergeant, 2017-03-14 02:40:36]

https://codereview.chromium.org/2740863003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/actions.js (right):

https://codereview.chromium.org/2740863003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/actions.js:80: return {name:
'clear-search'};
On 2017/03/13 04:50:36, calamity wrote:
> I feel like this should be its own explicit action even if it's never invoked
> from UI directly. I'd prefer 1 possible Action name per Action maker.

Done.

https://codereview.chromium.org/2740863003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/app.js (right):

https://codereview.chromium.org/2740863003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/app.js:45:
this.dispatch(bookmarks.actions.setSearchResults(ids));
On 2017/03/13 04:50:36, calamity wrote:
> If we're going to do search results by id, we need to worry about whether the
> node will exist...
> 
> What happens when you delete a bookmark showing on the search results page,
for
> example?

I was working yesterday on a follow-up CL which purges deleted nodes (including
all descendants) from all areas of the state tree, which addressed this concern.

> Is it possible for bad timing to cause a node id that's been deleted to
> arrive in the frontend through search?

The timing requirements for this are pretty specific, and I wasn't able to get
it to repro in JS alone (maybe there's a property of the JS api or the bookmark
mode that prevents this? who knows)

> I think we should either handle the case where results don't exist in the node
> map or make sure that it can never happen.

What kind of thing do you think should happen? Do we assert? Do we try and fail
gracefully in the UI and hide the <bookmark-item> if the node doesn't exist?

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

https://codereview.chromium.org/2740863003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/reducers.js:108: Object.assign({}, node,
{children: newChildren}));
On 2017/03/13 04:50:36, calamity wrote:
> Was this supposed to have a TODO to remove the node from the nodemap?

That TODO lives in the test file (and as above, I'm working on the CL for it
now).

https://codereview.chromium.org/2740863003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/reducers.js:173: return
nodes['0'].children[0];
On 2017/03/13 04:50:36, calamity wrote:
> Add a TODO to make this return to the previously selected folder?

Done.

https://codereview.chromium.org/2740863003/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/md_bookmarks/reducers_test.js (right):

https://codereview.chromium.org/2740863003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/reducers_test.js:148: action =
bookmarks.actions.selectFolder('0');
On 2017/03/13 04:50:36, calamity wrote:
> Hmm. Can we select a real folder here. The root isn't selectable in the real
> world. We should probably actually throw something if the root folder gets
> selected.

Done. I've added an assert to the action to make sure you can't select the root
folder, and updated testTree to add in a root folder automatically.

https://codereview.chromium.org/2740863003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/reducers_test.js:149: nextState =
bookmarks.reduceAction(initialState, action);
On 2017/03/13 04:50:36, calamity wrote:
> Initialize nextState with initialState so that this line isn't subtly
different.
> In fact, it might be a good cargo-cultable pattern to just have 'var state;'
at
> the top level and then have state = initialState in the setup. (Preferably
apply
> this to all tests that use an entire state.)

For this test, I've combined initialState and nextState into a single `state`
variable, since the distinction wasn't necessary.

https://codereview.chromium.org/2740863003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/reducers_test.js:164: // UI changes once
search results arrive:
On 2017/03/13 04:50:36, calamity wrote:
> nit: Newlines before these comments.

Done.

[calamity, 2017-03-14 03:32:19]

lgtm.

https://codereview.chromium.org/2740863003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/app.js (right):

https://codereview.chromium.org/2740863003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/app.js:45:
this.dispatch(bookmarks.actions.setSearchResults(ids));
On 2017/03/14 02:40:36, tsergeant wrote:
> On 2017/03/13 04:50:36, calamity wrote:
> > If we're going to do search results by id, we need to worry about whether
the
> > node will exist...
> > 
> > What happens when you delete a bookmark showing on the search results page,
> for
> > example?
> 
> I was working yesterday on a follow-up CL which purges deleted nodes
(including
> all descendants) from all areas of the state tree, which addressed this
concern.
> 
> > Is it possible for bad timing to cause a node id that's been deleted to
> > arrive in the frontend through search?
> 
> The timing requirements for this are pretty specific, and I wasn't able to get
> it to repro in JS alone (maybe there's a property of the JS api or the
bookmark
> mode that prevents this? who knows)
> 
> > I think we should either handle the case where results don't exist in the
node
> > map or make sure that it can never happen.
> 
> What kind of thing do you think should happen? Do we assert? Do we try and
fail
> gracefully in the UI and hide the <bookmark-item> if the node doesn't exist?

Taking a glance at the API implementation, timing issues don't seem to be a
problem. I think just asserting in item.js is fine, but I would feel better if
we had some way of reporting these assertions if they fail, just so we're aware
of any uptick in issues in the real world. Maybe just adding a histogram that
counts every time an assert gets hit? WDYT?

https://codereview.chromium.org/2740863003/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/md_bookmarks/reducers_test.js (right):

https://codereview.chromium.org/2740863003/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/reducers_test.js:148: action =
bookmarks.actions.selectFolder('0');
On 2017/03/14 02:40:36, tsergeant wrote:
> On 2017/03/13 04:50:36, calamity wrote:
> > Hmm. Can we select a real folder here. The root isn't selectable in the real
> > world. We should probably actually throw something if the root folder gets
> > selected.
> 
> Done. I've added an assert to the action to make sure you can't select the
root
> folder, and updated testTree to add in a root folder automatically.

#Like+1.

https://codereview.chromium.org/2740863003/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/md_bookmarks/reducers_test.js (right):

https://codereview.chromium.org/2740863003/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/md_bookmarks/reducers_test.js:7: var initialState;
Can we also ditch all the other initialStates in this file? (Not necessarily
now, but sometime.)

https://codereview.chromium.org/2740863003/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/md_bookmarks/reducers_test.js:21:
assertFalse(!!nextState['1']);
nit: reverse these 2 lines.

https://codereview.chromium.org/2740863003/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/md_bookmarks/reducers_test.js:31:
assertTrue(nextState['1']);
May as well also check '2's state here since there's nowhere else that asserts
that closing parents doesn't close children.

https://codereview.chromium.org/2740863003/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/md_bookmarks/test_util.js (right):

https://codereview.chromium.org/2740863003/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/md_bookmarks/test_util.js:17: * @param
{...BookmarkTreeNode} nodes
Fancy.

[tsergeant, 2017-03-14 05:51:11]

https://codereview.chromium.org/2740863003/diff/80001/chrome/browser/resource...
File chrome/browser/resources/md_bookmarks/app.js (right):

https://codereview.chromium.org/2740863003/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/app.js:45:
this.dispatch(bookmarks.actions.setSearchResults(ids));
On 2017/03/14 03:32:18, calamity wrote:
> On 2017/03/14 02:40:36, tsergeant wrote:
> > On 2017/03/13 04:50:36, calamity wrote:
> > > If we're going to do search results by id, we need to worry about whether
> the
> > > node will exist...
> > > 
> > > What happens when you delete a bookmark showing on the search results
page,
> > for
> > > example?
> > 
> > I was working yesterday on a follow-up CL which purges deleted nodes
> (including
> > all descendants) from all areas of the state tree, which addressed this
> concern.
> > 
> > > Is it possible for bad timing to cause a node id that's been deleted to
> > > arrive in the frontend through search?
> > 
> > The timing requirements for this are pretty specific, and I wasn't able to
get
> > it to repro in JS alone (maybe there's a property of the JS api or the
> bookmark
> > mode that prevents this? who knows)
> > 
> > > I think we should either handle the case where results don't exist in the
> node
> > > map or make sure that it can never happen.
> > 
> > What kind of thing do you think should happen? Do we assert? Do we try and
> fail
> > gracefully in the UI and hide the <bookmark-item> if the node doesn't exist?
> 
> Taking a glance at the API implementation, timing issues don't seem to be a
> problem. I think just asserting in item.js is fine, but I would feel better if
> we had some way of reporting these assertions if they fail, just so we're
aware
> of any uptick in issues in the real world. Maybe just adding a histogram that
> counts every time an assert gets hit? WDYT?

As discussed in person, I've added in an assertion to <bookmarks-item> which
checks if the node exists when itemId is assigned. There's also a TODO there to
do something with Histograms if it fails.

https://codereview.chromium.org/2740863003/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/md_bookmarks/reducers_test.js (right):

https://codereview.chromium.org/2740863003/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/md_bookmarks/reducers_test.js:7: var initialState;
On 2017/03/14 03:32:18, calamity wrote:
> Can we also ditch all the other initialStates in this file? (Not necessarily
> now, but sometime.)

TODO added

https://codereview.chromium.org/2740863003/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/md_bookmarks/reducers_test.js:21:
assertFalse(!!nextState['1']);
On 2017/03/14 03:32:18, calamity wrote:
> nit: reverse these 2 lines.

Done.

https://codereview.chromium.org/2740863003/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/md_bookmarks/reducers_test.js:31:
assertTrue(nextState['1']);
On 2017/03/14 03:32:18, calamity wrote:
> May as well also check '2's state here since there's nowhere else that asserts
> that closing parents doesn't close children.

Done.

[tsergeant, 2017-03-14 05:51:17]

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

[commit-bot: I haz the power, 2017-03-14 05:51:41]

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-14 07:07:25]

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

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

Dry run: This issue passed the CQ dry run.

[tsergeant, 2017-03-14 22:32:28]

The CQ bit was checked by tsergeant@chromium.org

[tsergeant, 2017-03-14 22:32:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
calamity@chromium.org
Link to the patchset: https://codereview.chromium.org/2740863003/#ps120001
(title: "Review round 2")

[commit-bot: I haz the power, 2017-03-14 22:34:09]

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-14 22:40:35]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1489530748803260,
"parent_rev": "ae7d79520a3ed23d98647353cfd5f6704c56fd69", "commit_rev":
"bb410f3b8a762cc37f641494e308aec6919d0c60"}

[commit-bot: I haz the power, 2017-03-14 22:41:19]

Description was changed from

==========
MD Bookmarks: Implement search in new data flow system

MD Bookmarks has switched to a new data flow system which makes it
easier to bind and modify backend data from the UI. This CL implements
search in this new system.

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

to

==========
MD Bookmarks: Implement search in new data flow system

MD Bookmarks has switched to a new data flow system which makes it
easier to bind and modify backend data from the UI. This CL implements
search in this new system.

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

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

[commit-bot: I haz the power, 2017-03-14 22:41:20]

Committed patchset #5 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/bb410f3b8a762cc37f641494e308...