MD Bookmarks: Add basic page features to new data-flow system

MD Bookmarks: Add basic page features to new data-flow system

This adds the backend Action/Reducer functions required to support:
- Showing/editing/removing bookmarks
- Selecting which folder is displayed
- Opening/closing folders in the sidebar
Notable missing features are search and selection.

This code is part of the new data-flow system and is currently only used
by tests. It will be tied into UI elements in a follow-up CL; see bug for
further details.

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

Review-Url: https://codereview.chromium.org/2733463002
Cr-Commit-Position: refs/heads/master@{#455655}
Committed: https://chromium.googlesource.com/chromium/src/+/3e522424f710ebb5378f17a1e55ce22b262e0433

[tsergeant, 2017-03-02 23:44:03]

Description was changed from

==========
MD Bookmarks: Add basic page features to new data-flow system

This adds the backend Action/Reducer functions required to support:
- Showing/editing/removing bookmarks
- Selecting which folder is displayed
- Opening/closing folders in the sidebar
Notable missing features are search and selection.

This code is part of the new data-flow system and is currently only used
by tests. It will be tied into UI elements in a follow-up CL; see bug for
further details.

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

to

==========
MD Bookmarks: Add basic page features to new data-flow system

This adds the backend Action/Reducer functions required to support:
- Showing/editing/removing bookmarks
- Selecting which folder is displayed
- Opening/closing folders in the sidebar
Notable missing features are search and selection.

This code is part of the new data-flow system and is currently only used
by tests. It will be tied into UI elements in a follow-up CL; see bug for
further details.

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

[tsergeant, 2017-03-03 05:26:37]

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:26:52]

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-03 05:31:23]

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

[tsergeant, 2017-03-03 05:31:24]

I think this one is ready to look at if you're bored on Monday.

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

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

[commit-bot: I haz the power, 2017-03-03 06:26:27]

Dry run: This issue passed the CQ dry run.

[tsergeant, 2017-03-07 04:58:30]

On 2017/03/03 05:31:24, tsergeant wrote:
> I think this one is ready to look at if you're bored on Monday.

^ ping on this, since the previous CL is now ready to land.

[calamity, 2017-03-07 07:27:35]

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

https://codereview.chromium.org/2733463002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/actions.js:79: changeFolderOpen:
changeFolderOpen,
nit: Sort? I don't feel super strongly.

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

https://codereview.chromium.org/2733463002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/reducers.js:24: return Object.assign({},
nodes, nodeModification);
How does closure understand this?

https://codereview.chromium.org/2733463002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/reducers.js:35: Object.assign({}, node,
action.changeInfo));
While it doesn't understand this.

https://codereview.chromium.org/2733463002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/reducers.js:98: // TODO(tsergeant): It
should not be possible to select a non-folder.
What's the plan for this TODO? Are you just going to
assert(nodes[action.id].children)?

https://codereview.chromium.org/2733463002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/reducers.js:144: var id = action.id;
nit: This var seems superfluous.

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

https://codereview.chromium.org/2733463002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/util.js:15: return
assert(state.nodes[assert(state.selectedFolder)].children);
Does this remove the 'unselected sidebar during search' functionality?

https://codereview.chromium.org/2733463002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/util.js:24: var nodeMap = {};
nit: nodeList?

https://codereview.chromium.org/2733463002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/util.js:25: var queue = [];
Looks more like a stack to me. Jus sayin'.

https://codereview.chromium.org/2733463002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/util.js:35: if (node.children) {
nit: invert and early continue.

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

https://codereview.chromium.org/2733463002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/reducers_test.js:91: test('updates when a
node is edited', function() {
Does this include folders? If so, add an edit test for one.

[tsergeant, 2017-03-08 02:47:11]

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

https://codereview.chromium.org/2733463002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/actions.js:79: changeFolderOpen:
changeFolderOpen,
On 2017/03/07 07:27:35, calamity wrote:
> nit: Sort? I don't feel super strongly.

Done.

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

https://codereview.chromium.org/2733463002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/reducers.js:24: return Object.assign({},
nodes, nodeModification);
On 2017/03/07 07:27:35, calamity wrote:
> How does closure understand this?

I would guess that here Closure sees that Object.assign returns an Object, and
is able to silently convert that to a NodeList, which is a variant of Object
(Object<string, BookmarkNode>).

https://codereview.chromium.org/2733463002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/reducers.js:35: Object.assign({}, node,
action.changeInfo));
On 2017/03/07 07:27:35, calamity wrote:
> While it doesn't understand this.

But here, it isn't able to convert Object to the specific BookmarkNode record
type.

https://codereview.chromium.org/2733463002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/reducers.js:98: // TODO(tsergeant): It
should not be possible to select a non-folder.
On 2017/03/07 07:27:35, calamity wrote:
> What's the plan for this TODO? Are you just going to
> assert(nodes[action.id].children)?

I think that if you try to select a folder that either doesn't exist or isn't an
item, it will return the existing selectedFolder without change.

https://codereview.chromium.org/2733463002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/reducers.js:144: var id = action.id;
On 2017/03/07 07:27:35, calamity wrote:
> nit: This var seems superfluous.

Done.

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

https://codereview.chromium.org/2733463002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/util.js:15: return
assert(state.nodes[assert(state.selectedFolder)].children);
On 2017/03/07 07:27:35, calamity wrote:
> Does this remove the 'unselected sidebar during search' functionality?

The plan is that the sidebar will be unselected during search (identical to how
it is today on master)

That will be coming in the next CL (this one doesn't know anything about
search).

https://codereview.chromium.org/2733463002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/util.js:24: var nodeMap = {};
On 2017/03/07 07:27:35, calamity wrote:
> nit: nodeList?

Done.

https://codereview.chromium.org/2733463002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/util.js:25: var queue = [];
On 2017/03/07 07:27:35, calamity wrote:
> Looks more like a stack to me. Jus sayin'.

Done.

https://codereview.chromium.org/2733463002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/util.js:35: if (node.children) {
On 2017/03/07 07:27:35, calamity wrote:
> nit: invert and early continue.

Done.

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

https://codereview.chromium.org/2733463002/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/reducers_test.js:91: test('updates when a
node is edited', function() {
On 2017/03/07 07:27:35, calamity wrote:
> Does this include folders? If so, add an edit test for one.

Done. I've also added a check to make sure that folders can't be turned into
items by editing their url.

[calamity, 2017-03-08 06:57:19]

lgtm

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

https://codereview.chromium.org/2733463002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/reducers.js:35: Object.assign({}, node,
action.changeInfo));
On 2017/03/08 02:47:10, tsergeant wrote:
> On 2017/03/07 07:27:35, calamity wrote:
> > While it doesn't understand this.
> 
> But here, it isn't able to convert Object to the specific BookmarkNode record
> type.

Ech.

https://codereview.chromium.org/2733463002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/reducers.js:98: // TODO(tsergeant): It
should not be possible to select a non-folder.
On 2017/03/08 02:47:10, tsergeant wrote:
> On 2017/03/07 07:27:35, calamity wrote:
> > What's the plan for this TODO? Are you just going to
> > assert(nodes[action.id].children)?
> 
> I think that if you try to select a folder that either doesn't exist or isn't
an
> item, it will return the existing selectedFolder without change.

I'd prefer throwing an exception over failing silently since it'll make it
easier to catch. Maybe bailing out in the action creation is a good place since
it won't affect the state at all.

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

https://codereview.chromium.org/2733463002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/reducers_test.js:125: // entirely.
TODO timeline?

[tsergeant, 2017-03-08 23:10:55]

The CQ bit was checked by tsergeant@chromium.org

[commit-bot: I haz the power, 2017-03-08 23:11:41]

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

[tsergeant, 2017-03-08 23:17:53]

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

https://codereview.chromium.org/2733463002/diff/40001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/reducers.js:98: // TODO(tsergeant): It
should not be possible to select a non-folder.
On 2017/03/08 06:57:19, calamity wrote:
> On 2017/03/08 02:47:10, tsergeant wrote:
> > On 2017/03/07 07:27:35, calamity wrote:
> > > What's the plan for this TODO? Are you just going to
> > > assert(nodes[action.id].children)?
> > 
> > I think that if you try to select a folder that either doesn't exist or
isn't
> an
> > item, it will return the existing selectedFolder without change.
> 
> I'd prefer throwing an exception over failing silently since it'll make it
> easier to catch. Maybe bailing out in the action creation is a good place
since
> it won't affect the state at all.

Yeah, maybe. I have a feeling we want to avoid asserting in reducers to keep
them pure.

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

https://codereview.chromium.org/2733463002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/reducers_test.js:125: // entirely.
On 2017/03/08 06:57:19, calamity wrote:
> TODO timeline?

Top-priority after the current series of CLs to land the new dataflow system.

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

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

[commit-bot: I haz the power, 2017-03-09 00:09:58]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

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

The CQ bit was checked by tsergeant@chromium.org

[commit-bot: I haz the power, 2017-03-09 02:46:16]

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 03:33:27]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1489027534768480,
"parent_rev": "9ca1914b925d1108499243aeb51dee5d175535af", "commit_rev":
"3e522424f710ebb5378f17a1e55ce22b262e0433"}

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

Description was changed from

==========
MD Bookmarks: Add basic page features to new data-flow system

This adds the backend Action/Reducer functions required to support:
- Showing/editing/removing bookmarks
- Selecting which folder is displayed
- Opening/closing folders in the sidebar
Notable missing features are search and selection.

This code is part of the new data-flow system and is currently only used
by tests. It will be tied into UI elements in a follow-up CL; see bug for
further details.

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

to

==========
MD Bookmarks: Add basic page features to new data-flow system

This adds the backend Action/Reducer functions required to support:
- Showing/editing/removing bookmarks
- Selecting which folder is displayed
- Opening/closing folders in the sidebar
Notable missing features are search and selection.

This code is part of the new data-flow system and is currently only used
by tests. It will be tied into UI elements in a follow-up CL; see bug for
further details.

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

Review-Url: https://codereview.chromium.org/2733463002
Cr-Commit-Position: refs/heads/master@{#455655}
Committed:
https://chromium.googlesource.com/chromium/src/+/3e522424f710ebb5378f17a1e55c...
==========

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

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