MD Bookmarks: Remove deleted nodes from state tree

MD Bookmarks: Remove deleted nodes from state tree

This clears out deleted nodes (and their descendants) from all areas of
the state tree:
- The node data is removed from the core node map
- The nodes are unselected
- If the selected folder was deleted, show the parent
- Remove any deleted nodes from search results

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

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

[tsergeant, 2017-03-17 02:28:55]

Description was changed from

==========
MD Bookmarks: Remove deleted nodes from state tree

This clears out deleted nodes (and their descendants) from all areas of
the state tree:
- The node data is removed from the core node map
- The nodes are unselected
- If the selected folder was deleted, show the parent
- Remove any deleted nodes from search results

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

to

==========
MD Bookmarks: Remove deleted nodes from state tree

This clears out deleted nodes (and their descendants) from all areas of
the state tree:
- The node data is removed from the core node map
- The nodes are unselected
- If the selected folder was deleted, show the parent
- Remove any deleted nodes from search results

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

[tsergeant, 2017-03-23 05:36:07]

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

[tsergeant, 2017-03-23 05:36:07]

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

https://codereview.chromium.org/2752223004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/actions.js:50: function removeBookmark(id,
parentId, index, nodes) {
My intention is still that actions should be able to access the store state
without being passed it in.

This has some difficulties with Closure (yayyyyy) and testing that I haven't
fully figured out yet. I'll get to it next week, but wanted to send this CL to
you in the meantime anyway.

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

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

https://codereview.chromium.org/2752223004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/reducers.js:53: items:
bookmarks.util.removeNodesFromMap(selectionState.items, deleted),
When this turns into a Set, this won't work anymore and we'll need a separate
removeKeysFromSet() function.

https://codereview.chromium.org/2752223004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/reducers.js:54: anchor: null,
nit: Ideally the anchor remains on the same item it was, but I'm not super
fussed about that I guess.

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

https://codereview.chromium.org/2752223004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store_client.js:77: if (oldValue ==
newValue || newValue == undefined)
What case does this prevent? Anything deleted in the nodeList shouldn't be
showing anywhere already, right?

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

https://codereview.chromium.org/2752223004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/util.js:107: function
removeNodesFromMap(map, nodes) {
Maybe genericize further into removeKeysFromMap? At the moment this is
overloading Nodes in a confusing way. Another alternative is removeIdsFromMap.

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

https://codereview.chromium.org/2752223004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/reducers_test.js:211: action =
bookmarks.actions.selectFolder('3');
This is technically selecting a deleted folder, but since we're just looking at
selection state, we don't care. However, this is a bit confusing between state
is used elsewhere to mean the actual store.data 'state'.

I might just take a sledgehammer to this test and rename all the 'state's to the
piece of the state they represent. WDYT?

https://codereview.chromium.org/2752223004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/reducers_test.js:378: assertDeepEquals(['1',
'2'], state.search.results);
Also delete 1 and ensure 2 vanishes due to being a descendant.

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

https://codereview.chromium.org/2752223004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/util_test.js:25: }
FYI, I needed this for another patch so I moved it to test_util.js

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

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

https://codereview.chromium.org/2752223004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/reducers.js:53: items:
bookmarks.util.removeNodesFromMap(selectionState.items, deleted),
On 2017/04/03 06:54:24, calamity wrote:
> When this turns into a Set, this won't work anymore and we'll need a separate
> removeKeysFromSet() function.

I've rebased onto your Map->Set CL, and added a new removeIdsFromSet() function.

https://codereview.chromium.org/2752223004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/reducers.js:54: anchor: null,
On 2017/04/03 06:54:24, calamity wrote:
> nit: Ideally the anchor remains on the same item it was, but I'm not super
> fussed about that I guess.

My general feeling is that it's not really worth the effort to keep the anchor
correct after the user does something complex like a deletion.

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

https://codereview.chromium.org/2752223004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/store_client.js:77: if (oldValue ==
newValue || newValue == undefined)
On 2017/04/03 06:54:24, calamity wrote:
> What case does this prevent? Anything deleted in the nodeList shouldn't be
> showing anywhere already, right?

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.

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

https://codereview.chromium.org/2752223004/diff/60001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/util.js:107: function
removeNodesFromMap(map, nodes) {
On 2017/04/03 06:54:24, calamity wrote:
> Maybe genericize further into removeKeysFromMap? At the moment this is
> overloading Nodes in a confusing way. Another alternative is removeIdsFromMap.

Done, with removeIdsFromMap (and removeIdsFromSet)

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

https://codereview.chromium.org/2752223004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/reducers_test.js:211: action =
bookmarks.actions.selectFolder('3');
On 2017/04/03 06:54:24, calamity wrote:
> This is technically selecting a deleted folder, but since we're just looking
at
> selection state, we don't care. However, this is a bit confusing between state
> is used elsewhere to mean the actual store.data 'state'.
> 
> I might just take a sledgehammer to this test and rename all the 'state's to
the
> piece of the state they represent. WDYT?

Seems like a reasonable change.

In the meantime, I've added a new sibling folder to select here to make it a bit
less confusing.

https://codereview.chromium.org/2752223004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/reducers_test.js:378: assertDeepEquals(['1',
'2'], state.search.results);
On 2017/04/03 06:54:24, calamity wrote:
> Also delete 1 and ensure 2 vanishes due to being a descendant.

Done, I changed it to delete 2 and check 2 and 3 vanish in one go.

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

https://codereview.chromium.org/2752223004/diff/60001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/util_test.js:25: }
On 2017/04/03 06:54:24, calamity wrote:
> FYI, I needed this for another patch so I moved it to test_util.js

Ack, picked up in the rebase

[calamity, 2017-04-04 08:37:48]

lgtm.

[tsergeant, 2017-04-05 00:39:17]

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

[commit-bot: I haz the power, 2017-04-05 00:40:06]

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-04-05 02:18:20]

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

[commit-bot: I haz the power, 2017-04-05 02:18:20]

Dry run: This issue passed the CQ dry run.

[tsergeant, 2017-04-05 02:33:46]

The CQ bit was checked by tsergeant@chromium.org

[tsergeant, 2017-04-05 02:33:46]

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/2752223004/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2017-04-05 02:34:20]

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-04-05 02:48:24]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1491359626267200,
"parent_rev": "377d975f2cad5c02157f2436c6d7cf2318c82a51", "commit_rev":
"ff66de077810f11a905d267687e29a644902fd87"}

[commit-bot: I haz the power, 2017-04-05 02:49:13]

Description was changed from

==========
MD Bookmarks: Remove deleted nodes from state tree

This clears out deleted nodes (and their descendants) from all areas of
the state tree:
- The node data is removed from the core node map
- The nodes are unselected
- If the selected folder was deleted, show the parent
- Remove any deleted nodes from search results

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

to

==========
MD Bookmarks: Remove deleted nodes from state tree

This clears out deleted nodes (and their descendants) from all areas of
the state tree:
- The node data is removed from the core node map
- The nodes are unselected
- If the selected folder was deleted, show the parent
- Remove any deleted nodes from search results

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

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

[commit-bot: I haz the power, 2017-04-05 02:49:14]

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