[MD settings] side nav routing to page sections

[MD settings] side nav routing to page sections

This CL makes the side nav show the correct settings
section when selecting a page in the menu. Several
routes have been added for top-level pages
(non-subpages). A SettingsRoute closure type has been
defined.

BUG=604942, 593989, 584478, 606632
Committed: https://crrev.com/e46c22835a225bc3c6361a45fa3558400eb7b5dd
Cr-Commit-Position: refs/heads/master@{#390802}

[dschuyler, 2016-04-26 01:27:24]

Description was changed from

==========
[MD settings] side nav routing to page sections

This CL makes the side nav show the correct settings
section when selecting a page in the menu. Several
routes have been added for top-level pages
(non-subpages). A SettingsRoute closure type has been
defined.

BUG=604942, 593989
==========

to

==========
[MD settings] side nav routing to page sections

This CL makes the side nav show the correct settings
section when selecting a page in the menu. Several
routes have been added for top-level pages
(non-subpages). A SettingsRoute closure type has been
defined.

BUG=604942, 593989, 584478, 606632
==========

[dschuyler, 2016-04-26 01:27:24]

dschuyler@chromium.org changed reviewers:
+ michaelpg@chromium.org

[dschuyler, 2016-04-26 18:52:44]

Patchset #1 (id:1) has been deleted

[dschuyler, 2016-04-26 19:09:04]

Patchset #1 (id:20001) has been deleted

[dschuyler, 2016-04-26 19:19:24]

On 2016/04/26 19:09:31, dschuyler wrote:

I filed bug 606876 to follow up on URLs for the
passwords and languages sections.

[michaelpg, 2016-04-28 00:33:43]

the problem seems to be that even document.body still has a computed display
property of 'none' when your code is running.

maybe try checking if document.readyState == 'complete'. if false, add an event
listener for 'load' and scroll there.

'load' is gated on resource loading (including images), so it could be delayed
too much if we have to load external images (ie, favicons). but otherwise, AFAIK
we don't have a way of knowing when the page has been laid out.

[michaelpg, 2016-04-29 20:19:04]

This looks good.

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

https://codereview.chromium.org/1916323002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:339: //
TODO(dschuyler): Determine whether this setTimeout can be removed.
I filed Polymer/polymer#3629 so you can link to that here.

https://codereview.chromium.org/1916323002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:342: if
(element)
Looks like this should be an assert.

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

https://codereview.chromium.org/1916323002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:5: 
remove newline

https://codereview.chromium.org/1916323002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:14: var
SettingsRoute;
nice

https://codereview.chromium.org/1916323002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:89: *
@private {!Array<SettingsRoute>}
!SettingsRoute

https://codereview.chromium.org/1916323002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:379: url:
'/pw', /* TODO(dschuyler): find a better url. */
nit: 2 spaces separation and // style inline comments

https://codereview.chromium.org/1916323002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:490: * @param
{SettingsRoute} newRoute Where we're headed.
!

https://codereview.chromium.org/1916323002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:491: * @param
{SettingsRoute} oldRoute Where we've been.
!SettingsRoute|undefined

[dschuyler, 2016-04-29 21:27:47]

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

https://codereview.chromium.org/1916323002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:339: //
TODO(dschuyler): Determine whether this setTimeout can be removed.
On 2016/04/29 20:19:04, michaelpg wrote:
> I filed Polymer/polymer#3629 so you can link to that here.

Done.

https://codereview.chromium.org/1916323002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:342: if
(element)
On 2016/04/29 20:19:04, michaelpg wrote:
> Looks like this should be an assert.

Done. I removed the 'if' and made this one line.

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

https://codereview.chromium.org/1916323002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:5: 
On 2016/04/29 20:19:04, michaelpg wrote:
> remove newline

Done.

https://codereview.chromium.org/1916323002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:14: var
SettingsRoute;
On 2016/04/29 20:19:04, michaelpg wrote:
> nice

Acknowledged.

https://codereview.chromium.org/1916323002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:89: *
@private {!Array<SettingsRoute>}
On 2016/04/29 20:19:04, michaelpg wrote:
> !SettingsRoute

Done.

https://codereview.chromium.org/1916323002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:379: url:
'/pw', /* TODO(dschuyler): find a better url. */
On 2016/04/29 20:19:04, michaelpg wrote:
> nit: 2 spaces separation and // style inline comments

Done.

https://codereview.chromium.org/1916323002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:490: * @param
{SettingsRoute} newRoute Where we're headed.
On 2016/04/29 20:19:04, michaelpg wrote:
> !

Done.

https://codereview.chromium.org/1916323002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:491: * @param
{SettingsRoute} oldRoute Where we've been.
On 2016/04/29 20:19:04, michaelpg wrote:
> !SettingsRoute|undefined

Done.

[michaelpg, 2016-04-29 23:25:53]

lgtm

[dschuyler, 2016-04-29 23:26:55]

The CQ bit was checked by dschuyler@chromium.org

[commit-bot: I haz the power, 2016-04-29 23:27:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1916323002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1916323002/80001

[commit-bot: I haz the power, 2016-04-29 23:32:32]

Description was changed from

==========
[MD settings] side nav routing to page sections

This CL makes the side nav show the correct settings
section when selecting a page in the menu. Several
routes have been added for top-level pages
(non-subpages). A SettingsRoute closure type has been
defined.

BUG=604942, 593989, 584478, 606632
==========

to

==========
[MD settings] side nav routing to page sections

This CL makes the side nav show the correct settings
section when selecting a page in the menu. Several
routes have been added for top-level pages
(non-subpages). A SettingsRoute closure type has been
defined.

BUG=604942, 593989, 584478, 606632
==========

[commit-bot: I haz the power, 2016-04-29 23:32:33]

Committed patchset #3 (id:80001)

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

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

[commit-bot: I haz the power, 2016-04-30 18:03:29]

Description was changed from

==========
[MD settings] side nav routing to page sections

This CL makes the side nav show the correct settings
section when selecting a page in the menu. Several
routes have been added for top-level pages
(non-subpages). A SettingsRoute closure type has been
defined.

BUG=604942, 593989, 584478, 606632
==========

to

==========
[MD settings] side nav routing to page sections

This CL makes the side nav show the correct settings
section when selecting a page in the menu. Several
routes have been added for top-level pages
(non-subpages). A SettingsRoute closure type has been
defined.

BUG=604942, 593989, 584478, 606632

Committed: https://crrev.com/e46c22835a225bc3c6361a45fa3558400eb7b5dd
Cr-Commit-Position: refs/heads/master@{#390802}
==========