Settings Router Refactor: Migrate canonical route list to new-style.

Settings Router Refactor: Migrate canonical route list to new-style.

This links the router element to the new-style route list.

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

Committed: https://crrev.com/8acb9d5f2f71c3df8e64b13ee321a9d194e89785
Cr-Commit-Position: refs/heads/master@{#405947}

[tommycli, 2016-07-15 16:29:43]

Description was changed from

==========
Settings Router Refactor: Migrate canonical route list to new-style.

This links the router element to the new-style route list.

BUG=608115
==========

to

==========
Settings Router Refactor: Migrate canonical route list to new-style.

This links the router element to the new-style route list.

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

[tommycli, 2016-07-15 16:29:49]

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

[commit-bot: I haz the power, 2016-07-15 16:30:37]

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, 2016-07-15 17:04:45]

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

[commit-bot: I haz the power, 2016-07-15 17:04:46]

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

[tommycli, 2016-07-15 17:23:46]

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

[commit-bot: I haz the power, 2016-07-15 17:24:26]

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

[tommycli, 2016-07-15 17:29:45]

Patchset #1 (id:1) has been deleted

[tommycli, 2016-07-15 18:03:19]

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

[commit-bot: I haz the power, 2016-07-15 18:04:13]

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

[tommycli, 2016-07-15 18:04:18]

tommycli@chromium.org changed reviewers:
+ dbeam@chromium.org, michaelpg@chromium.org

[tommycli, 2016-07-15 18:04:19]

michaelpg, dbeam: PTAL

Patchset 1 contains both lists and a browsertest to make sure they are
equivalent. (This is just to verify that we migrated correctly, no need to
commit this.)

Patchset 2 deletes the old list and makes settings-router use the new list.

Thanks!

[commit-bot: I haz the power, 2016-07-15 20:37:34]

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

[commit-bot: I haz the power, 2016-07-15 20:37:35]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2016-07-15 23:28:27]

lgtm

https://codereview.chromium.org/2153113003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/route.js (right):

https://codereview.chromium.org/2153113003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:29: *                      leading
slash.
i think

  * @param {string} param Multi-line description ...
  *     wraps at 4 \s instead.

is more common

https://codereview.chromium.org/2153113003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:35: assert(path.length > 0);
maybe assert(path) instead?

https://codereview.chromium.org/2153113003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:37: var newUrl = path[0] == '/' ?
path : this.url + '/' + path;
can you add a comment to what this is doing?  also, should we use URL() for
this?

https://codereview.chromium.org/2153113003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:96: // Root pages
nit: end with .

https://codereview.chromium.org/2153113003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_router.js (right):

https://codereview.chromium.org/2153113003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:99:
canonicalRoutes_: Object.keys(settings.Route).map(function(key) {
Object.values(settings.Route)

https://codereview.chromium.org/2153113003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_resources.grd (right):

https://codereview.chromium.org/2153113003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_resources.grd:685: flattenhtml="true"
/>
can you use preprocess="true" instead here?

https://codereview.chromium.org/2153113003/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/route_tests.js (right):

https://codereview.chromium.org/2153113003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/route_tests.js:60: });
Object.values(settings.Route).forEach(function(route) {
  assertFalse(urls.has(route.url), route.url);
  urls.add(route.url);
});

[tommycli, 2016-07-15 23:47:51]

dbeam: thanks!

https://codereview.chromium.org/2153113003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/route.js (right):

https://codereview.chromium.org/2153113003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:29: *                      leading
slash.
On 2016/07/15 23:28:27, Dan Beam wrote:
> i think
> 
>   * @param {string} param Multi-line description ...
>   *     wraps at 4 \s instead.
> 
> is more common

Done.

https://codereview.chromium.org/2153113003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:35: assert(path.length > 0);
On 2016/07/15 23:28:26, Dan Beam wrote:
> maybe assert(path) instead?

Done.

https://codereview.chromium.org/2153113003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:37: var newUrl = path[0] == '/' ?
path : this.url + '/' + path;
On 2016/07/15 23:28:26, Dan Beam wrote:
> can you add a comment to what this is doing?  also, should we use URL() for
> this?

Done. I think using URL doesn't work since that constructor asserts if you don't
provide a protocol, host, etc.

https://codereview.chromium.org/2153113003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:96: // Root pages
On 2016/07/15 23:28:26, Dan Beam wrote:
> nit: end with .

Done.

https://codereview.chromium.org/2153113003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_router.js (right):

https://codereview.chromium.org/2153113003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:99:
canonicalRoutes_: Object.keys(settings.Route).map(function(key) {
On 2016/07/15 23:28:27, Dan Beam wrote:
> Object.values(settings.Route)

Closure compiler complains: ERROR - Property values never defined on Object.

It is present in Canary chrome though...

https://codereview.chromium.org/2153113003/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_resources.grd (right):

https://codereview.chromium.org/2153113003/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_resources.grd:685: flattenhtml="true"
/>
On 2016/07/15 23:28:27, Dan Beam wrote:
> can you use preprocess="true" instead here?

Done.

https://codereview.chromium.org/2153113003/diff/40001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/route_tests.js (right):

https://codereview.chromium.org/2153113003/diff/40001/chrome/test/data/webui/...
chrome/test/data/webui/settings/route_tests.js:60: });
On 2016/07/15 23:28:27, Dan Beam wrote:
> Object.values(settings.Route).forEach(function(route) {
>   assertFalse(urls.has(route.url), route.url);
>   urls.add(route.url);
> });

Done. It works. Thanks. I'm living on the bleeding edge now.

[tommycli, 2016-07-15 23:47:57]

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

[commit-bot: I haz the power, 2016-07-15 23:48:12]

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, 2016-07-16 00:44:39]

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

[commit-bot: I haz the power, 2016-07-16 00:44:39]

Dry run: This issue passed the CQ dry run.

[tommycli, 2016-07-16 19:23:04]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-07-16 19:23:05]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org
Link to the patchset: https://codereview.chromium.org/2153113003/#ps60001
(title: "address comments")

[commit-bot: I haz the power, 2016-07-16 19:23: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, 2016-07-16 19:26:35]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-07-16 19:27:46]

Description was changed from

==========
Settings Router Refactor: Migrate canonical route list to new-style.

This links the router element to the new-style route list.

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

to

==========
Settings Router Refactor: Migrate canonical route list to new-style.

This links the router element to the new-style route list.

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

Committed: https://crrev.com/8acb9d5f2f71c3df8e64b13ee321a9d194e89785
Cr-Commit-Position: refs/heads/master@{#405947}
==========

[commit-bot: I haz the power, 2016-07-16 19:27:47]

Patchset 3 (id:??) landed as
https://crrev.com/8acb9d5f2f71c3df8e64b13ee321a9d194e89785
Cr-Commit-Position: refs/heads/master@{#405947}