Settings Router Refactor: Migrate to settings.Route.navigateTo calls.

Settings Router Refactor: Migrate to settings.Route.navigateTo calls.

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

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

[tommycli, 2016-07-18 23:31:01]

Description was changed from

==========
Settings Router Refactor: Migrate to settings.Route.navigateTo calls.

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

to

==========
Settings Router Refactor: Migrate to settings.Route.navigateTo calls.

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

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

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

[commit-bot: I haz the power, 2016-07-19 18:15:01]

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-19 18:15:34]

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

[commit-bot: I haz the power, 2016-07-19 18:16:12]

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-19 18:28:03]

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

[commit-bot: I haz the power, 2016-07-19 18:28:55]

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-19 18:54:26]

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

[commit-bot: I haz the power, 2016-07-19 18:57:44]

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-19 20:12:19]

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

[commit-bot: I haz the power, 2016-07-19 20:12:20]

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-19 20:50:20]

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

[commit-bot: I haz the power, 2016-07-19 20:51:01]

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-19 21:33:25]

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

[tommycli, 2016-07-19 21:33:26]

dbeam: PTAL, thanks!

[commit-bot: I haz the power, 2016-07-19 22:08:17]

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

[commit-bot: I haz the power, 2016-07-19 22:08:18]

Dry run: This issue passed the CQ dry run.

[tommycli, 2016-07-20 19:19:39]

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

[commit-bot: I haz the power, 2016-07-20 19:20:13]

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-20 19:34:31]

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

[commit-bot: I haz the power, 2016-07-20 19:34:32]

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

[michaelpg, 2016-07-20 19:48:23]

https://codereview.chromium.org/2156413002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_animated_pages.js
(right):

https://codereview.chromium.org/2156413002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:49:
assert(this.currentRoute.parent != null);
why not assert in navigateTo instead of in individual calls?

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

https://codereview.chromium.org/2156413002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:56:
'settings.navigateTo already defined. There may only be one ' +
using another namespace to enforce a singleton seems a little weird. maybe use
our existing cr singleton interface.

https://codereview.chromium.org/2156413002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:68: var
matchingKeys = Object.keys(settings.Route).filter(function(key) {
.find

https://codereview.chromium.org/2156413002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:75:
assert(matchingKeys.length == 1, 'Duplicate routes for: ' + path);
rely on testing for this instead

https://codereview.chromium.org/2156413002/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/device_page_tests.js (right):

https://codereview.chromium.org/2156413002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/device_page_tests.js:120: settings.navigateTo =
function(route) {
Can you provide your own stub instead?

https://codereview.chromium.org/2156413002/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/site_list_tests.js (right):

https://codereview.chromium.org/2156413002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_list_tests.js:254: settings.navigateTo =
function(route) {};
same

[tommycli, 2016-07-20 20:32:05]

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

[commit-bot: I haz the power, 2016-07-20 20:32:41]

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-20 20:33:29]

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

[tommycli, 2016-07-20 20:33:31]

michaelpg: Thanks! (I added you to the reviewer list btw)

https://codereview.chromium.org/2156413002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_animated_pages.js
(right):

https://codereview.chromium.org/2156413002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:49:
assert(this.currentRoute.parent != null);
On 2016/07/20 19:48:23, michaelpg wrote:
> why not assert in navigateTo instead of in individual calls?

Done.

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

https://codereview.chromium.org/2156413002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:56:
'settings.navigateTo already defined. There may only be one ' +
On 2016/07/20 19:48:23, michaelpg wrote:
> using another namespace to enforce a singleton seems a little weird. maybe use
> our existing cr singleton interface.

Well... the intent is to really prevent the global settings.navigateTo from
getting set twice. I removed the assert, since it seems to cause more confusion.
It seems pretty harmless to let a newer settings-router displace an older one.

https://codereview.chromium.org/2156413002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:68: var
matchingKeys = Object.keys(settings.Route).filter(function(key) {
On 2016/07/20 19:48:23, michaelpg wrote:
> .find

Done.

https://codereview.chromium.org/2156413002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_router.js:75:
assert(matchingKeys.length == 1, 'Duplicate routes for: ' + path);
On 2016/07/20 19:48:23, michaelpg wrote:
> rely on testing for this instead

Done.

https://codereview.chromium.org/2156413002/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/device_page_tests.js (right):

https://codereview.chromium.org/2156413002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/device_page_tests.js:120: settings.navigateTo =
function(route) {
On 2016/07/20 19:48:23, michaelpg wrote:
> Can you provide your own stub instead?

Per offline discussion, we are going to address the testing use cases more
throughly once the router refactor is further along.

https://codereview.chromium.org/2156413002/diff/80001/chrome/test/data/webui/...
File chrome/test/data/webui/settings/site_list_tests.js (right):

https://codereview.chromium.org/2156413002/diff/80001/chrome/test/data/webui/...
chrome/test/data/webui/settings/site_list_tests.js:254: settings.navigateTo =
function(route) {};
On 2016/07/20 19:48:23, michaelpg wrote:
> same

Acknowledged.

[commit-bot: I haz the power, 2016-07-20 21:41:43]

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

[commit-bot: I haz the power, 2016-07-20 21:41:45]

Dry run: This issue passed the CQ dry run.

[tommycli, 2016-07-20 23:37:56]

dbeam: ping, thx

[Dan Beam, 2016-07-21 00:05:42]

as far as I can tell, all of the .html files that correspond with your .js
change:

* need <link rel="import" href="/routes.html"> if they use settings.Route
* probably need a <link rel="import" href="/settings_router.html"> if they use
settings.navigateTo() and move this code there

additionally, we probably need to update a whole bunch of
compiled_resources2.gyp files?  any target for a .js file that uses
settings.Route needs a dependency on 1-2 targets now, right?

https://codereview.chromium.org/2156413002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/route.js (right):

https://codereview.chromium.org/2156413002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:274: var navigateTo = null;
how is this useful?  why wouldn't this just live in settings_router.js?

[tommycli, 2016-07-21 00:12:10]

On 2016/07/21 00:05:42, Dan Beam wrote:
> as far as I can tell, all of the .html files that correspond with your .js
> change:
> 
> * need <link rel="import" href="/routes.html"> if they use settings.Route
> * probably need a <link rel="import" href="/settings_router.html"> if they use
> settings.navigateTo() and move this code there
> 
> additionally, we probably need to update a whole bunch of
> compiled_resources2.gyp files?  any target for a .js file that uses
> settings.Route needs a dependency on 1-2 targets now, right?
> 
>
https://codereview.chromium.org/2156413002/diff/120001/chrome/browser/resourc...
> File chrome/browser/resources/settings/route.js (right):
> 
>
https://codereview.chromium.org/2156413002/diff/120001/chrome/browser/resourc...
> chrome/browser/resources/settings/route.js:274: var navigateTo = null;
> how is this useful?  why wouldn't this just live in settings_router.js?

Yes, I think everyone who uses settings.navigateTo (which I hope to keep a
global) will need a compiled_resources2.gyp dependency on route.js if we follow
IYWU to the letter.

Does that sounds reasonable to you?

[Dan Beam, 2016-07-21 00:53:32]

On 2016/07/21 00:12:10, tommycli wrote:
> On 2016/07/21 00:05:42, Dan Beam wrote:
> > as far as I can tell, all of the .html files that correspond with your .js
> > change:
> > 
> > * need <link rel="import" href="/routes.html"> if they use settings.Route
> > * probably need a <link rel="import" href="/settings_router.html"> if they
use
> > settings.navigateTo() and move this code there
> > 
> > additionally, we probably need to update a whole bunch of
> > compiled_resources2.gyp files?  any target for a .js file that uses
> > settings.Route needs a dependency on 1-2 targets now, right?
> > 
> >
>
https://codereview.chromium.org/2156413002/diff/120001/chrome/browser/resourc...
> > File chrome/browser/resources/settings/route.js (right):
> > 
> >
>
https://codereview.chromium.org/2156413002/diff/120001/chrome/browser/resourc...
> > chrome/browser/resources/settings/route.js:274: var navigateTo = null;
> > how is this useful?  why wouldn't this just live in settings_router.js?
> 
> Yes, I think everyone who uses settings.navigateTo (which I hope to keep a
> global) will need a compiled_resources2.gyp dependency on route.js if we
follow
> IYWU to the letter.
> 
> Does that sounds reasonable to you?

you're basically doing a forward declaration in JS.  why?  what good is this
code without the implementation?  are you just hoping that any test that loads a
specific page never calls settings.navigateTo()?

[michaelpg, 2016-07-21 03:05:03]

On 2016/07/21 00:53:32, Dan Beam wrote:
> On 2016/07/21 00:12:10, tommycli wrote:
> > On 2016/07/21 00:05:42, Dan Beam wrote:
> > > as far as I can tell, all of the .html files that correspond with your .js
> > > change:
> > > 
> > > * need <link rel="import" href="/routes.html"> if they use settings.Route
> > > * probably need a <link rel="import" href="/settings_router.html"> if they
> use
> > > settings.navigateTo() and move this code there
> > > 
> > > additionally, we probably need to update a whole bunch of
> > > compiled_resources2.gyp files?  any target for a .js file that uses
> > > settings.Route needs a dependency on 1-2 targets now, right?
> > > 
> > >
> >
>
https://codereview.chromium.org/2156413002/diff/120001/chrome/browser/resourc...
> > > File chrome/browser/resources/settings/route.js (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2156413002/diff/120001/chrome/browser/resourc...
> > > chrome/browser/resources/settings/route.js:274: var navigateTo = null;
> > > how is this useful?  why wouldn't this just live in settings_router.js?
> > 
> > Yes, I think everyone who uses settings.navigateTo (which I hope to keep a
> > global) will need a compiled_resources2.gyp dependency on route.js if we
> follow
> > IYWU to the letter.

it's not just about where navigateTo is "declared"; navigateTo can't be used
without knowing the constants in route.js. There isn't really an alternative to
following IYWU "to the letter" -- if we try to be clever and put it in
settings.html, then it will blow up in a bunch of tests and we'll have to work
around that too. We had this problem with loadTimeData before we settled on
everyone importing i18n_setup.html. Same with polymer.html.

> > 
> > Does that sounds reasonable to you?
> 
> you're basically doing a forward declaration in JS.  why?  what good is this
> code without the implementation?  are you just hoping that any test that loads
a
> specific page never calls settings.navigateTo()?

[tommycli, 2016-07-21 17:33:19]

On 2016/07/21 00:53:32, Dan Beam wrote:
> On 2016/07/21 00:12:10, tommycli wrote:
> > On 2016/07/21 00:05:42, Dan Beam wrote:
> > > as far as I can tell, all of the .html files that correspond with your .js
> > > change:
> > > 
> > > * need <link rel="import" href="/routes.html"> if they use settings.Route
> > > * probably need a <link rel="import" href="/settings_router.html"> if they
> use
> > > settings.navigateTo() and move this code there
> > > 
> > > additionally, we probably need to update a whole bunch of
> > > compiled_resources2.gyp files?  any target for a .js file that uses
> > > settings.Route needs a dependency on 1-2 targets now, right?
> > > 
> > >
> >
>
https://codereview.chromium.org/2156413002/diff/120001/chrome/browser/resourc...
> > > File chrome/browser/resources/settings/route.js (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2156413002/diff/120001/chrome/browser/resourc...
> > > chrome/browser/resources/settings/route.js:274: var navigateTo = null;
> > > how is this useful?  why wouldn't this just live in settings_router.js?
> > 
> > Yes, I think everyone who uses settings.navigateTo (which I hope to keep a
> > global) will need a compiled_resources2.gyp dependency on route.js if we
> follow
> > IYWU to the letter.
> > 
> > Does that sounds reasonable to you?
> 
> you're basically doing a forward declaration in JS.  why?  what good is this
> code without the implementation?  are you just hoping that any test that loads
a
> specific page never calls settings.navigateTo()?

Hey, the forward declaration aspect is temporary. Eventually, settings-router
will be deleted and the function definition will just live within route.
Currently this is a "forward declaration" to enable us to update all the
callsites to use the typechecked navigateTo function.

In a followup CL, navigateTo will not be tied to a settings-router method, but
instead just be defined within route.js.

[tommycli, 2016-07-21 18:06:10]

one draft I forgot to publish:

https://codereview.chromium.org/2156413002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/route.js (right):

https://codereview.chromium.org/2156413002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:274: var navigateTo = null;
On 2016/07/21 00:05:42, Dan Beam wrote:
> how is this useful?  why wouldn't this just live in settings_router.js?

Goal is to delete in the future settings-router. This global fn is needed for
elements to be able to perform type-checked navigations.

[dschuyler, 2016-07-21 21:58:53]

https://codereview.chromium.org/2156413002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_menu/settings_menu.js (right):

https://codereview.chromium.org/2156413002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.js:88:
settings.navigateTo(settings.Route.BASIC);
This looks like it's missing the dataset.section.
How will this work to get to separate sections?

[Dan Beam, 2016-07-22 01:16:10]

https://codereview.chromium.org/2156413002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/about_page/about_page.js (right):

https://codereview.chromium.org/2156413002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/about_page/about_page.js:253:
settings.navigateTo(settings.Route.DETAILED_BUILD_INFO);
any .js file that uses either settings.Route.* or settings.navigateTo() needs to
update its corresponding compiled_resources2.gyp (if it exists).

as in, chrome/browser/resources/settings/about_page/compiled_resources2.gyp must
add this line:

{
  'targets': [
    {   
      'target_name': 'about_page',
      'dependencies': [
        ...
++      '../compiled_resources2.gyp:route',
        
do this THEN ping this CL

[tommycli, 2016-07-22 18:02:25]

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

[commit-bot: I haz the power, 2016-07-22 18:02:47]

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-22 18:05:49]

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

[commit-bot: I haz the power, 2016-07-22 18:05:51]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)

[tommycli, 2016-07-22 18:09:06]

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

[commit-bot: I haz the power, 2016-07-22 18:09:31]

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-22 19:06:08]

dbeam / dschuyler: Addressed your comments below. Thanks!

https://codereview.chromium.org/2156413002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/about_page/about_page.js (right):

https://codereview.chromium.org/2156413002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/about_page/about_page.js:253:
settings.navigateTo(settings.Route.DETAILED_BUILD_INFO);
On 2016/07/22 01:16:10, Dan Beam wrote:
> any .js file that uses either settings.Route.* or settings.navigateTo() needs
to
> update its corresponding compiled_resources2.gyp (if it exists).
> 
> as in, chrome/browser/resources/settings/about_page/compiled_resources2.gyp
must
> add this line:
> 
> {
>   'targets': [
>     {   
>       'target_name': 'about_page',
>       'dependencies': [
>         ...
> ++      '../compiled_resources2.gyp:route',
>         
> do this THEN ping this CL

Done. I wasn't trying to irritate - I just wanted to make sure the global stub
approach was fine with you before proceeding.

https://codereview.chromium.org/2156413002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_menu/settings_menu.js (right):

https://codereview.chromium.org/2156413002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.js:88:
settings.navigateTo(settings.Route.BASIC);
On 2016/07/21 21:58:53, dschuyler wrote:
> This looks like it's missing the dataset.section.
> How will this work to get to separate sections?

Done. The bound-route-objects approach we discussed yesterday did not work. I
made a workaround for now, and we can try to improve it further later. Does that
seem fine?

[commit-bot: I haz the power, 2016-07-22 19:21:07]

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

[commit-bot: I haz the power, 2016-07-22 19:21:09]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2016-07-23 00:17:12]

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
File
chrome/browser/resources/settings/people_page/quick_unlock_password_detect_behavior.js
(right):

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/quick_unlock_password_detect_behavior.js:22:
if (!this.setModes) {
nit: drop curlies

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/settings/privacy_page/privacy_page.js (right):

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/privacy_page/privacy_page.js:92:
settings.navigateTo(settings.Route.PRIVACY);
nit:

if (Polymer.dom(event).rootTarget.tagName == 'CR-DIALOG')
  settings.navigateTo(settings.Route.PRIVACY);

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/settings/route.js (left):

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:143: r.KEYBARD =
r.DEVICE.createChild('/keyboard-overlay', 'keyboard');
KEYBARRRRRD!

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_main/settings_main.js (right):

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:141:
settings.navigateTo(settings.Route.BASIC);
nit: maybe considering putting

  if (this.currentRoute == newRoute)
    return;

into settings.navigateTo itself

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_menu/settings_menu.js (right):

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.js:90: var
matchingKey = Object.keys(settings.Route).find(function(key) {
Object.values()

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.js:93:
route.subpage.length == 0;
can you add a TODO() about what this code should be if .page or .section doesn't
exist?

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_animated_pages.js
(right):

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:49:
settings.navigateTo(this.currentRoute.parent);
tight (though, like we talked about, maybe a navigateToParent() and leave
.parent private).

oh well, this is fine and more generic for now

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_router.js (left):

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_router.js:19: var
HistoricRoute;
my only regret is that I never got to get my kicks from naming one "66"

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_router.js (right):

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_router.js:52:
this.currentRoute = this.getRouteFor_(window.location.pathname);
so we basically assume this will always work, right?  does this work for invalid
URLs?  i.e. chrome://settings/gobbleDeeGoop

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_router.js:65: var
matchingKey = Object.keys(settings.Route).find(function(key) {
Object.values()

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/site_settings_behavior.js
(right):

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_settings_behavior.js:142:
default:
nit: if this list is exhaustive, can you remove this default and just put
assertNotReached() as the last line in this method (outside of the switch)

I've been talking with tbreisacher@ about potentially making a compiler warning
for not handling every case in an enum on a switch, but having a default: would
disallow that

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_settings_behavior.js:182:
assertNotReached();
same

https://codereview.chromium.org/2156413002/diff/220001/chrome/test/data/webui...
File chrome/test/data/webui/settings/site_list_tests.js (right):

https://codereview.chromium.org/2156413002/diff/220001/chrome/test/data/webui...
chrome/test/data/webui/settings/site_list_tests.js:254: settings.navigateTo =
function(route) {};
i haz a sad :_(

[tommycli, 2016-07-25 16:40:01]

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

[commit-bot: I haz the power, 2016-07-25 16:40:19]

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-25 16:47:21]

dbeam: Thanks! I think the only thing I wasn't able to do below was the
Object.values since closure compiler still did not like it.

Tommy

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
File
chrome/browser/resources/settings/people_page/quick_unlock_password_detect_behavior.js
(right):

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/people_page/quick_unlock_password_detect_behavior.js:22:
if (!this.setModes) {
On 2016/07/23 00:17:11, Dan Beam wrote:
> nit: drop curlies

Done.

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/settings/privacy_page/privacy_page.js (right):

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/privacy_page/privacy_page.js:92:
settings.navigateTo(settings.Route.PRIVACY);
On 2016/07/23 00:17:11, Dan Beam wrote:
> nit:
> 
> if (Polymer.dom(event).rootTarget.tagName == 'CR-DIALOG')
>   settings.navigateTo(settings.Route.PRIVACY);

Done.

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/settings/route.js (left):

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:143: r.KEYBARD =
r.DEVICE.createChild('/keyboard-overlay', 'keyboard');
On 2016/07/23 00:17:11, Dan Beam wrote:
> KEYBARRRRRD!

Acknowledged.

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_main/settings_main.js (right):

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:141:
settings.navigateTo(settings.Route.BASIC);
On 2016/07/23 00:17:11, Dan Beam wrote:
> nit: maybe considering putting
> 
>   if (this.currentRoute == newRoute)
>     return;
> 
> into settings.navigateTo itself

Done.

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_menu/settings_menu.js (right):

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.js:90: var
matchingKey = Object.keys(settings.Route).find(function(key) {
On 2016/07/23 00:17:12, Dan Beam wrote:
> Object.values()

Closure compiler doesn't accept values(). Even after pulling this morning.

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_menu/settings_menu.js:93:
route.subpage.length == 0;
On 2016/07/23 00:17:12, Dan Beam wrote:
> can you add a TODO() about what this code should be if .page or .section
doesn't
> exist?

Done.

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_animated_pages.js
(right):

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:49:
settings.navigateTo(this.currentRoute.parent);
On 2016/07/23 00:17:12, Dan Beam wrote:
> tight (though, like we talked about, maybe a navigateToParent() and leave
> .parent private).
> 
> oh well, this is fine and more generic for now

Acknowledged.

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_router.js (left):

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_router.js:19: var
HistoricRoute;
On 2016/07/23 00:17:12, Dan Beam wrote:
> my only regret is that I never got to get my kicks from naming one "66"

Acknowledged.

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/settings_router.js (right):

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_router.js:52:
this.currentRoute = this.getRouteFor_(window.location.pathname);
On 2016/07/23 00:17:12, Dan Beam wrote:
> so we basically assume this will always work, right?  does this work for
invalid
> URLs?  i.e. chrome://settings/gobbleDeeGoop

It will give you settings.Route.BASIC for gobbleDeeGoop. Which I think is the
intended behavior...

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/settings_router.js:65: var
matchingKey = Object.keys(settings.Route).find(function(key) {
On 2016/07/23 00:17:12, Dan Beam wrote:
> Object.values()

Closure compiler does not like values. Even after pulling this morning.

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings/site_settings_behavior.js
(right):

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_settings_behavior.js:142:
default:
On 2016/07/23 00:17:12, Dan Beam wrote:
> nit: if this list is exhaustive, can you remove this default and just put
> assertNotReached() as the last line in this method (outside of the switch)
> 
> I've been talking with tbreisacher@ about potentially making a compiler
warning
> for not handling every case in an enum on a switch, but having a default:
would
> disallow that

Done.

https://codereview.chromium.org/2156413002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings/site_settings_behavior.js:182:
assertNotReached();
On 2016/07/23 00:17:12, Dan Beam wrote:
> same

Done.

https://codereview.chromium.org/2156413002/diff/220001/chrome/test/data/webui...
File chrome/test/data/webui/settings/site_list_tests.js (right):

https://codereview.chromium.org/2156413002/diff/220001/chrome/test/data/webui...
chrome/test/data/webui/settings/site_list_tests.js:254: settings.navigateTo =
function(route) {};
On 2016/07/23 00:17:12, Dan Beam wrote:
> i haz a sad :_(

Yeah this is unfortunate. This will be gone when settings-router is gone though.
I added a TODO.

[michaelpg, 2016-07-25 17:28:08]

lgtm... I only looked at the interesting changes though.

https://codereview.chromium.org/2156413002/diff/260001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings_page/site_settings_page.js
(right):

https://codereview.chromium.org/2156413002/diff/260001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings_page/site_settings_page.js:79:
settings.navigateTo(settings.Route.SITE_SETTINGS_HANDLERS);
can this case be merged into the "else" case? I don't see why these need to be
separated if we're using Route objects instead of generating the subpage list.

[commit-bot: I haz the power, 2016-07-25 17:37:01]

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

[commit-bot: I haz the power, 2016-07-25 17:37:03]

Dry run: This issue passed the CQ dry run.

[tommycli, 2016-07-25 17:38:16]

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

[commit-bot: I haz the power, 2016-07-25 17:38:32]

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-25 17:38:34]

michaelpg: Thanks!

https://codereview.chromium.org/2156413002/diff/260001/chrome/browser/resourc...
File chrome/browser/resources/settings/site_settings_page/site_settings_page.js
(right):

https://codereview.chromium.org/2156413002/diff/260001/chrome/browser/resourc...
chrome/browser/resources/settings/site_settings_page/site_settings_page.js:79:
settings.navigateTo(settings.Route.SITE_SETTINGS_HANDLERS);
On 2016/07/25 17:28:08, michaelpg wrote:
> can this case be merged into the "else" case? I don't see why these need to be
> separated if we're using Route objects instead of generating the subpage list.

Done.

[tommycli, 2016-07-25 18:07:43]

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

[commit-bot: I haz the power, 2016-07-25 18:08:03]

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-25 18:56:03]

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

[commit-bot: I haz the power, 2016-07-25 18:56: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, 2016-07-25 19:07:42]

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

[commit-bot: I haz the power, 2016-07-25 19:07:44]

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

[tommycli, 2016-07-25 19:27:04]

Hey - PS 17 has the Object.values, and it seems to fail on closure.

I saw that the closure roll includes the Object.values externs, so I am puzzled.

I have also tried to run closure locally, and my local machine includes the
latest closure changes, but it still fails.

Tommy

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

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

[commit-bot: I haz the power, 2016-07-25 23:16:02]

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

[Dan Beam, 2016-07-25 23:24:02]

14 -> 17 lgtm

also fine with reverting from values(val) to keys(() => obj[key]) for now :/

[tommycli, 2016-07-25 23:26:00]

On 2016/07/25 23:24:02, Dan Beam wrote:
> 14 -> 17 lgtm
> 
> also fine with reverting from values(val) to keys(() => obj[key]) for now :/

Okay thanks! I'm sending in PS19, which is PS16 + TODOs for the Object.keys
issue.

[tommycli, 2016-07-25 23:26:14]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-07-25 23:26:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2156413002/#ps360001
(title: "add todo")

[commit-bot: I haz the power, 2016-07-25 23:26:43]

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-26 00:29:39]

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

[commit-bot: I haz the power, 2016-07-26 00:29:41]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tommycli, 2016-07-26 19:32:19]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2016-07-26 19:32:50]

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-26 20:36:15]

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

[commit-bot: I haz the power, 2016-07-26 20:36:17]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tommycli, 2016-07-26 20:45:48]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-07-26 20:45:49]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org,
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2156413002/#ps380001
(title: "try to fix mac bot failure on languages test")

[commit-bot: I haz the power, 2016-07-26 20:46:33]

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-26 22:07:38]

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

[commit-bot: I haz the power, 2016-07-26 22:07:40]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[tommycli, 2016-07-26 22:41:19]

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

[commit-bot: I haz the power, 2016-07-26 22:42:00]

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-26 23:38:31]

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

[commit-bot: I haz the power, 2016-07-26 23:39:01]

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-27 00:32:24]

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

[commit-bot: I haz the power, 2016-07-27 00:32:26]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[tommycli, 2016-07-27 01:01:27]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-07-27 01:01:28]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org,
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2156413002/#ps440001
(title: "Fix flaky test yet a different way")

[commit-bot: I haz the power, 2016-07-27 01:02:08]

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

[tommycli, 2016-07-27 01:05:05]

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

[tommycli, 2016-07-27 01:05:07]

dschuyler: Per offline discussion, sending this in changing this.async to
setTimeout as temporary fix to flaky test on Mac 10.9

[commit-bot: I haz the power, 2016-07-27 01:39:39]

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

[commit-bot: I haz the power, 2016-07-27 01:39:41]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[michaelpg, 2016-07-27 03:11:35]

https://codereview.chromium.org/2156413002/diff/440001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_main/settings_main.js (right):

https://codereview.chromium.org/2156413002/diff/440001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:132: });
.bind(this)

[michaelpg, 2016-07-27 04:47:18]

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

[commit-bot: I haz the power, 2016-07-27 04:47:26]

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-27 05:15:41]

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

[commit-bot: I haz the power, 2016-07-27 05:15:43]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[michaelpg, 2016-07-27 07:47:39]

On 2016/07/27 01:05:07, tommycli wrote:
> dschuyler: Per offline discussion, sending this in changing this.async to
> setTimeout as temporary fix to flaky test on Mac 10.9

so, this CL actually changes navigation in unexpected ways. there are some
existing bugs with how sections in the Advanced page expand... this CL changes
the buggy behavior to different buggy behavior.

I suspect the language test only fails on mac due to differences in default
browser size on the try bots.

Working with the original before hacky fixes:
https://codereview.chromium.org/download/issue2156413002_380001.diff

to fail the browser test, reduce the window height:
xvfb-run -a -s '-screen 0 1024x500x24' out_linux/Release/browser_tests
--gtest_filter=SettingsLanguagesPageBrowserTest.*

async(fn, 1) and setTimeout(fn.bind(this)) both fix the test;
Polymer.dom.flush() does not. however, all of these fail to fix the Chrome repro
below, so something is still fishy. I interpret this as: Polymer.dom.flush()
definitely prevents certain race conditions we've encountered elsewhere, but
isn't related to this bug; and, using setTimeout just shuffles some tasks around
so it tends to work more (e.g. hides this bug) but not always.

to repro in Chrome:
0. apply your patch:
    git co origin/master && wget -O patch
https://codereview.chromium.org/download/issue2156413002_380001.diff && git
apply patch
1. in settings_page_visibility.js, replace
     var settingsHidePagesByDefaultForTest;
   with
     var settingsHidePagesByDefaultForTest = true;
2. navigate to chrome://md-settings/advanced
3. ensure the window is short enough: document.querySelector('* /deep/
paper-header-panel').clientHeight will need to be below about 590.
4. document.querySelector('* /deep/
settings-advanced-page').set('pageVisibility.languages', true);
5. under Language, click Manage languages; then click Back. the
overscrollHeight_() assertion (settings_main.js:157) fails. (you may have to try
this step twice.)

or, in a console: 

document.querySelector('* /deep/
settings-advanced-page').set('pageVisibility.languages', true);
setTimeout(function() {
document.querySelector('* /deep/ settings-languages-page /deep/
.list-button:last-of-type').click();
setTimeout(function() {
history.back();
setTimeout(function() {
document.querySelector('* /deep/ settings-languages-page /deep/
.list-button:last-of-type').click();
setTimeout(function() {
history.back();
}, 1000);
}, 1000);
}, 1000);
}, 1000);

[tommycli, 2016-07-28 18:19:24]

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

[commit-bot: I haz the power, 2016-07-28 18:19:55]

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-28 18:48:37]

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

[commit-bot: I haz the power, 2016-07-28 18:48:59]

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-28 19:16:55]

On 2016/07/27 07:47:39, michaelpg wrote:
> On 2016/07/27 01:05:07, tommycli wrote:
> > dschuyler: Per offline discussion, sending this in changing this.async to
> > setTimeout as temporary fix to flaky test on Mac 10.9
> 
> so, this CL actually changes navigation in unexpected ways. there are some
> existing bugs with how sections in the Advanced page expand... this CL changes
> the buggy behavior to different buggy behavior.
> 
> I suspect the language test only fails on mac due to differences in default
> browser size on the try bots.
> 
> Working with the original before hacky fixes:
> https://codereview.chromium.org/download/issue2156413002_380001.diff
> 
> to fail the browser test, reduce the window height:
> xvfb-run -a -s '-screen 0 1024x500x24' out_linux/Release/browser_tests
> --gtest_filter=SettingsLanguagesPageBrowserTest.*
> 
> async(fn, 1) and setTimeout(fn.bind(this)) both fix the test;
> Polymer.dom.flush() does not. however, all of these fail to fix the Chrome
repro
> below, so something is still fishy. I interpret this as: Polymer.dom.flush()
> definitely prevents certain race conditions we've encountered elsewhere, but
> isn't related to this bug; and, using setTimeout just shuffles some tasks
around
> so it tends to work more (e.g. hides this bug) but not always.
> 
> to repro in Chrome:
> 0. apply your patch:
>     git co origin/master && wget -O patch
> https://codereview.chromium.org/download/issue2156413002_380001.diff && git
> apply patch
> 1. in settings_page_visibility.js, replace
>      var settingsHidePagesByDefaultForTest;
>    with
>      var settingsHidePagesByDefaultForTest = true;
> 2. navigate to chrome://md-settings/advanced
> 3. ensure the window is short enough: document.querySelector('* /deep/
> paper-header-panel').clientHeight will need to be below about 590.
> 4. document.querySelector('* /deep/
> settings-advanced-page').set('pageVisibility.languages', true);
> 5. under Language, click Manage languages; then click Back. the
> overscrollHeight_() assertion (settings_main.js:157) fails. (you may have to
try
> this step twice.)
> 
> or, in a console: 
> 
> document.querySelector('* /deep/
> settings-advanced-page').set('pageVisibility.languages', true);
> setTimeout(function() {
> document.querySelector('* /deep/ settings-languages-page /deep/
> .list-button:last-of-type').click();
> setTimeout(function() {
> history.back();
> setTimeout(function() {
> document.querySelector('* /deep/ settings-languages-page /deep/
> .list-button:last-of-type').click();
> setTimeout(function() {
> history.back();
> }, 1000);
> }, 1000);
> }, 1000);
> }, 1000);

Hey michaelpg, thanks for the detailed repro instructions. It appears this patch
does uncover a bug.

Per offline discussion, planning on sending it in with the
setTimeout(foo.bind(this)) hack, and using the console.warn plan in case it
flakes other bots.

In the meantime, dschuyler is going to update to use an event-based system that
should solve these issues "for real".

[tommycli, 2016-07-28 20:17:26]

The CQ bit was unchecked by tommycli@chromium.org

[tommycli, 2016-07-28 20:17:26]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-07-28 20:17:27]

The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org,
michaelpg@chromium.org
Link to the patchset: https://codereview.chromium.org/2156413002/#ps480001
(title: "use timing fix")

[commit-bot: I haz the power, 2016-07-28 20:18:05]

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-28 20:38:09]

Committed patchset #25 (id:480001)

[commit-bot: I haz the power, 2016-07-28 20:41:48]

Description was changed from

==========
Settings Router Refactor: Migrate to settings.Route.navigateTo calls.

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

to

==========
Settings Router Refactor: Migrate to settings.Route.navigateTo calls.

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

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

[commit-bot: I haz the power, 2016-07-28 20:41:50]

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