MD Bookmarks: Update URL router to work in new data binding system

MD Bookmarks: Update URL router to work in new data binding system

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

Review-Url: https://codereview.chromium.org/2745993002
Cr-Commit-Position: refs/heads/master@{#458279}
Committed: https://chromium.googlesource.com/chromium/src/+/39932ca301fa70758c85d4b24c0e634fe693bd9a

[tsergeant, 2017-03-13 03:01:34]

Description was changed from

==========
MD Bookmarks: Update URL router to work in new data binding system

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

to

==========
MD Bookmarks: Update URL router to work in new data binding system

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

[tsergeant, 2017-03-16 00:29:52]

Patchset #3 (id:40001) has been deleted

[tsergeant, 2017-03-16 00:31:27]

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

[commit-bot: I haz the power, 2017-03-16 00:32:38]

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-16 04:08:30]

Patchset #3 (id:60001) has been deleted

[tsergeant, 2017-03-16 04:14:13]

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

[tsergeant, 2017-03-16 04:14:14]

PTAL, this is the last feature of the old Store to be ported.

[tsergeant, 2017-03-16 04:14:20]

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

[commit-bot: I haz the power, 2017-03-16 04:14:35]

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-16 04:54:10]

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

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

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[tsergeant, 2017-03-17 00:42:18]

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

[commit-bot: I haz the power, 2017-03-17 00:42:52]

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-17 01:49:41]

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

[commit-bot: I haz the power, 2017-03-17 01:49:42]

Dry run: This issue passed the CQ dry run.

[calamity, 2017-03-20 06:34:29]

lgtm. We should also do something reasonable when the folder we're in is deleted
(add a TODO to reducers.js) and I think showing an empty, unselected list when
the id doesn't exist is a touch jank. Can we just default back to the Bookmarks
bar? All follow-uppable.

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

https://codereview.chromium.org/2745993002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/toolbar.js:80: if (searchTerm !=
this.searchTerm_)
What makes this necessary?

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

https://codereview.chromium.org/2745993002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/md_bookmarks_browsertest.js:132: 
nit: Excessive newline.

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

https://codereview.chromium.org/2745993002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/router_test.js:56: // Ensure that the route
doesn't change when the search finishes:
nit: Too many dots.

[tsergeant, 2017-03-21 00:25:34]

Patchset #4 (id:100001) has been deleted

[tsergeant, 2017-03-21 00:27:06]

ding ding ding ding. Congrats, you've successfully identified all parts of the
"improve deletion" follow-up CL that's been hinted at for the past week.

In that CL, deleting the selected folder will select the parent of what was
deleted.

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

https://codereview.chromium.org/2745993002/diff/80001/chrome/browser/resource...
chrome/browser/resources/md_bookmarks/toolbar.js:80: if (searchTerm !=
this.searchTerm_)
On 2017/03/20 06:34:28, calamity wrote:
> What makes this necessary?

This is just a small optimisation to avoid firing off an action which does
nothing.

The flow is:

router fires a search action
StoreClient picks up a change to search term
onSearchTermChanged_ updates the search field
onSearchChanged_ sees that change to the search field, fires off an identical
search action.

This check avoids that second action.

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

https://codereview.chromium.org/2745993002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/md_bookmarks_browsertest.js:132: 
On 2017/03/20 06:34:28, calamity wrote:
> nit: Excessive newline.

Done.

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

https://codereview.chromium.org/2745993002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/md_bookmarks/router_test.js:56: // Ensure that the route
doesn't change when the search finishes:
On 2017/03/20 06:34:28, calamity wrote:
> nit: Too many dots.

..........
:::Done:::
˙˙˙˙˙˙˙˙˙˙

[tsergeant, 2017-03-21 00:27:21]

The CQ bit was checked by tsergeant@chromium.org

[tsergeant, 2017-03-21 00:27:21]

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/2745993002/#ps120001
(title: "calamity@ review")

[commit-bot: I haz the power, 2017-03-21 00:28:01]

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-21 02:12:50]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1490056041582020,
"parent_rev": "1c92dd551abe4a1581981b945076719dff7b4422", "commit_rev":
"39932ca301fa70758c85d4b24c0e634fe693bd9a"}

[commit-bot: I haz the power, 2017-03-21 02:13:43]

Description was changed from

==========
MD Bookmarks: Update URL router to work in new data binding system

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

to

==========
MD Bookmarks: Update URL router to work in new data binding system

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

Review-Url: https://codereview.chromium.org/2745993002
Cr-Commit-Position: refs/heads/master@{#458279}
Committed:
https://chromium.googlesource.com/chromium/src/+/39932ca301fa70758c85d4b24c0e...
==========

[commit-bot: I haz the power, 2017-03-21 02:13:44]

Committed patchset #4 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/39932ca301fa70758c85d4b24c0e...