MD Settings: Lazy load the contents of the "advanced" settings.

MD Settings: Lazy load the contents of the "advanced" settings.

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

Review-Url: https://codereview.chromium.org/2754563002
Cr-Commit-Position: refs/heads/master@{#458527}
Committed: https://chromium.googlesource.com/chromium/src/+/f9e8f9e6d60d8cd26c4239a377419cbb37fe8724

[dpapad, 2017-03-14 21:26:00]

Description was changed from

==========
DO NOT SUBMIT: settings-idle-load approach.

BUG=
==========

to

==========
DO NOT SUBMIT: settings-idle-load approach.

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

[dpapad, 2017-03-15 17:45:58]

Description was changed from

==========
DO NOT SUBMIT: settings-idle-load approach.

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

to

==========
DO NOT SUBMIT: MD Settings: Lazy load, using settings-idle-load approach.

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

[dpapad, 2017-03-16 00:52:18]

Patchset #3 (id:40001) has been deleted

[dpapad, 2017-03-16 17:44:43]

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

[commit-bot: I haz the power, 2017-03-16 17:45:21]

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 17:57:50]

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

[commit-bot: I haz the power, 2017-03-16 17:57:51]

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

[dpapad, 2017-03-16 18:31:39]

Description was changed from

==========
DO NOT SUBMIT: MD Settings: Lazy load, using settings-idle-load approach.

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

to

==========
MD Settings: Lazy load the contents of the "advanced" settings.

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

[dpapad, 2017-03-16 18:35:27]

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

[commit-bot: I haz the power, 2017-03-16 18:36:06]

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

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

[commit-bot: I haz the power, 2017-03-16 19:44:16]

Dry run: This issue passed the CQ dry run.

[dpapad, 2017-03-16 20:47:15]

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

[dpapad, 2017-03-16 20:47:16]

https://codereview.chromium.org/2754563002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/settings/controls/settings_idle_render.js (right):

https://codereview.chromium.org/2754563002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/settings/controls/settings_idle_render.js:15: is:
'settings-idle-load',
I'll rename this file to settings_idle_load.js. Just wanted to sent it out as-is
first, to showcase the diff better.

https://codereview.chromium.org/2754563002/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/settings/advanced_page_browsertest.js (right):

https://codereview.chromium.org/2754563002/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/advanced_page_browsertest.js:52:
assertTrue(!!section);
FYI, changed this because the expect() made no sense here. The test can't
actually continue if it fails (there is a null dereference in the first line of
verifySubpagesHidden).

[Dan Beam, 2017-03-16 20:52:22]

https://codereview.chromium.org/2754563002/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/settings/settings_page_browsertest.js (right):

https://codereview.chromium.org/2754563002/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_page_browsertest.js:64: * @return
{!PolymerElement} The PolymerElement for the page.
!Promise<!PolymerElement>

https://codereview.chromium.org/2754563002/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/settings/settings_subpage_browsertest.js (right):

https://codereview.chromium.org/2754563002/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_subpage_browsertest.js:73:
this.testSubPage.bind(this, this.page, subPage);
this is not the same.  did you mean

this.testSubPage(this.page, subPage);

?

[dpapad, 2017-03-17 00:08:09]

+dschuyler: Can you take a look at the changes in settings_main.js? In order to
make overscroll work with lazy loading, I had to add an extra call after lazy
loading has completed.

+michaelpg: Can you take a look at main_page_behavior.js? I had to make
tryTransitionToSection_ execute again, after the advanced page has rendered.

https://codereview.chromium.org/2754563002/diff/100001/chrome/test/data/webui...
File chrome/test/data/webui/settings/settings_subpage_browsertest.js (right):

https://codereview.chromium.org/2754563002/diff/100001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_subpage_browsertest.js:73:
this.testSubPage.bind(this, this.page, subPage);
On 2017/03/16 at 20:52:22, Dan Beam wrote:
> this is not the same.  did you mean
> 
> this.testSubPage(this.page, subPage);
> 
> ?

Good catch. Yes, I meant what you wrote above. I'll address this in next patch.

[Dan Beam, 2017-03-17 00:30:06]

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

https://codereview.chromium.org/2754563002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:88:
this.listen(this, 'lazy-loaded', 'onLazyLoaded_');
use listeners: or unlisten in detached()?

https://codereview.chromium.org/2754563002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2754563002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:122:
}.bind(this));
does this work

promise = this.$$('#advancedPageTemplate').get();

https://codereview.chromium.org/2754563002/diff/120001/chrome/test/data/webui...
File chrome/test/data/webui/settings/cr_settings_browsertest.js (right):

https://codereview.chromium.org/2754563002/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/cr_settings_browsertest.js:45: 
too many \n

https://codereview.chromium.org/2754563002/diff/120001/chrome/test/data/webui...
File chrome/test/data/webui/settings/help_page_browsertest.js (right):

https://codereview.chromium.org/2754563002/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/help_page_browsertest.js:29: setUp: function() {
why can't we just make .basicPage null in tests?

then you could do:

return self.getPage('about').then(function(aboutPage) {
  expectFalse(self.basicPage);
  expectTrue(!!self.getSection(page, 'about');
});

https://codereview.chromium.org/2754563002/diff/120001/chrome/test/data/webui...
File chrome/test/data/webui/settings/settings_idle_render_browsertest.js
(right):

https://codereview.chromium.org/2754563002/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_idle_render_browsertest.js:40:
'<template is="settings-idle-load" id="idleTemplate" url="">' +
is there anything real we could put here to simplify prod code to assume url
always exists?

https://codereview.chromium.org/2754563002/diff/120001/chrome/test/data/webui...
File chrome/test/data/webui/settings/settings_page_browsertest.js (right):

https://codereview.chromium.org/2754563002/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_page_browsertest.js:36: page: null,
basicPage

https://codereview.chromium.org/2754563002/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_page_browsertest.js:77: return
Promise.resolve(page);
var page = settingsMain.$$(pageType);

var idleRender = page && page.$$('template[is=settings-idle-load]');
if (i!dleRender)
  return Promise.resolve(page);

return idleRender.get().then(function() {
  Polymer.dom.flush();
  return page;
});

[Dan Beam, 2017-03-17 00:31:44]

https://codereview.chromium.org/2754563002/diff/120001/chrome/test/data/webui...
File chrome/test/data/webui/settings/ensure_lazy_loaded.js (right):

https://codereview.chromium.org/2754563002/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/ensure_lazy_loaded.js:6: 
nit: no \n

https://codereview.chromium.org/2754563002/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/ensure_lazy_loaded.js:9: if
(window.location.href == 'chrome://md-settings/') {
nit: if (location.href == location.origin) {

gets around the whole md-settings vs settings thing

https://codereview.chromium.org/2754563002/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/ensure_lazy_loaded.js:11: return new
Promise(function(resolve, reject) {
nit: mention this needs to be basically exactly the same as in the idle load

[dpapad, 2017-03-17 16:57:29]

Patchset #7 (id:140001) has been deleted

[dpapad, 2017-03-17 20:53:55]

Waiting for green tryjobs.

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

https://codereview.chromium.org/2754563002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:88:
this.listen(this, 'lazy-loaded', 'onLazyLoaded_');
On 2017/03/17 at 00:30:05, Dan Beam wrote:
> use listeners: or unlisten in detached()?

I am pretty sure listen() already takes care of unlistening on detached. See
implementation where "listeners" is also delegating to listen() internally,
https://github.com/Polymer/polymer/blob/1.x/src/standard/events.html#L72.

https://codereview.chromium.org/2754563002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2754563002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:122:
}.bind(this));
On 2017/03/17 at 00:30:05, Dan Beam wrote:
> does this work
> 
> promise = this.$$('#advancedPageTemplate').get();

It does. Done.

https://codereview.chromium.org/2754563002/diff/120001/chrome/test/data/webui...
File chrome/test/data/webui/settings/cr_settings_browsertest.js (right):

https://codereview.chromium.org/2754563002/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/cr_settings_browsertest.js:45: 
On 2017/03/17 at 00:30:05, Dan Beam wrote:
> too many \n

Done.

https://codereview.chromium.org/2754563002/diff/120001/chrome/test/data/webui...
File chrome/test/data/webui/settings/ensure_lazy_loaded.js (right):

https://codereview.chromium.org/2754563002/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/ensure_lazy_loaded.js:6: 
On 2017/03/17 at 00:31:43, Dan Beam wrote:
> nit: no \n

Done.

https://codereview.chromium.org/2754563002/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/ensure_lazy_loaded.js:9: if
(window.location.href == 'chrome://md-settings/') {
On 2017/03/17 at 00:31:43, Dan Beam wrote:
> nit: if (location.href == location.origin) {
> 
> gets around the whole md-settings vs settings thing

Done.

https://codereview.chromium.org/2754563002/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/ensure_lazy_loaded.js:11: return new
Promise(function(resolve, reject) {
On 2017/03/17 at 00:31:43, Dan Beam wrote:
> nit: mention this needs to be basically exactly the same as in the idle load

Done.

https://codereview.chromium.org/2754563002/diff/120001/chrome/test/data/webui...
File chrome/test/data/webui/settings/help_page_browsertest.js (right):

https://codereview.chromium.org/2754563002/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/help_page_browsertest.js:29: setUp: function() {
On 2017/03/17 at 00:30:05, Dan Beam wrote:
> why can't we just make .basicPage null in tests?
> 
> then you could do:
> 
> return self.getPage('about').then(function(aboutPage) {
>   expectFalse(self.basicPage);
>   expectTrue(!!self.getSection(page, 'about');
> });

Not fully understanding the suggestion. Overriding setUp() achieves exactly
that, not initializing 'basicPage', since it does not really exist in the DOM,
when you visit chrome://help.

https://codereview.chromium.org/2754563002/diff/120001/chrome/test/data/webui...
File chrome/test/data/webui/settings/settings_idle_render_browsertest.js
(right):

https://codereview.chromium.org/2754563002/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_idle_render_browsertest.js:40:
'<template is="settings-idle-load" id="idleTemplate" url="">' +
On 2017/03/17 at 00:30:05, Dan Beam wrote:
> is there anything real we could put here to simplify prod code to assume url
always exists?

Done. Found one that works.

https://codereview.chromium.org/2754563002/diff/120001/chrome/test/data/webui...
File chrome/test/data/webui/settings/settings_page_browsertest.js (right):

https://codereview.chromium.org/2754563002/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_page_browsertest.js:36: page: null,
On 2017/03/17 at 00:30:05, Dan Beam wrote:
> basicPage

Done.

https://codereview.chromium.org/2754563002/diff/120001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_page_browsertest.js:77: return
Promise.resolve(page);
On 2017/03/17 at 00:30:05, Dan Beam wrote:
> var page = settingsMain.$$(pageType);
> 
> var idleRender = page && page.$$('template[is=settings-idle-load]');
> if (i!dleRender)
>   return Promise.resolve(page);
> 
> return idleRender.get().then(function() {
>   Polymer.dom.flush();
>   return page;
> });

Done.

[dpapad, 2017-03-17 22:42:28]

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

[commit-bot: I haz the power, 2017-03-17 22:42:57]

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

[Dan Beam, 2017-03-18 00:15:51]

https://codereview.chromium.org/2754563002/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/controls/settings_idle_render.js (right):

https://codereview.chromium.org/2754563002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/controls/settings_idle_render.js:56:
function() {
i think this code could use some new lines.  suggestions follow

https://codereview.chromium.org/2754563002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/controls/settings_idle_render.js:59:
assert(this.ctor);
\n

https://codereview.chromium.org/2754563002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/controls/settings_idle_render.js:60: var
instance = this.stamp({});
\n

https://codereview.chromium.org/2754563002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/controls/settings_idle_render.js:62:
this.child_ = instance.root.firstElementChild;
\n

https://codereview.chromium.org/2754563002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/controls/settings_idle_render.js:64:
resolve(this.child_);
\n

https://codereview.chromium.org/2754563002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/controls/settings_idle_render.js:68:
}.bind(this));
please use `git cl format --js` on this code

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

https://codereview.chromium.org/2754563002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:93:
Polymer.dom.flush();
should we be doing this only in certain cases?

https://codereview.chromium.org/2754563002/diff/180001/chrome/test/data/webui...
File chrome/test/data/webui/settings/help_page_browsertest.js (right):

https://codereview.chromium.org/2754563002/diff/180001/chrome/test/data/webui...
chrome/test/data/webui/settings/help_page_browsertest.js:29: setUp: function() {
please add a note that you're intentionally bypassing the super
(SettingsPageBrowserTest#setUp)

https://codereview.chromium.org/2754563002/diff/180001/chrome/test/data/webui...
File chrome/test/data/webui/settings/settings_idle_render_browsertest.js
(right):

https://codereview.chromium.org/2754563002/diff/180001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_idle_render_browsertest.js:41: '   
url="chrome://resources/html/polymer.html">' +
why this?

[Dan Beam, 2017-03-18 00:36:40]

https://codereview.chromium.org/2754563002/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/controls/settings_idle_render.js (right):

https://codereview.chromium.org/2754563002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/controls/settings_idle_render.js:15: is:
'settings-idle-load',
can you rename this file now?

[Dan Beam, 2017-03-18 00:41:07]

lgtm

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

https://codereview.chromium.org/2754563002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:93:
Polymer.dom.flush();
On 2017/03/18 00:15:51, Dan Beam wrote:
> should we be doing this only in certain cases?

we probably should, but let's just see if anybody notices first

[commit-bot: I haz the power, 2017-03-18 01:51:00]

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

[commit-bot: I haz the power, 2017-03-18 01:51:00]

Dry run: This issue passed the CQ dry run.

[dpapad, 2017-03-20 19:38:11]

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

[commit-bot: I haz the power, 2017-03-20 19:38:33]

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

[dpapad, 2017-03-20 20:34:55]

@dbeam: See again change in main_page_behavior.js and new comment.

https://codereview.chromium.org/2754563002/diff/180001/chrome/browser/resourc...
File chrome/browser/resources/settings/controls/settings_idle_render.js (right):

https://codereview.chromium.org/2754563002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/controls/settings_idle_render.js:59:
assert(this.ctor);
On 2017/03/18 at 00:15:51, Dan Beam wrote:
> \n

Done.

https://codereview.chromium.org/2754563002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/controls/settings_idle_render.js:60: var
instance = this.stamp({});
On 2017/03/18 at 00:15:51, Dan Beam wrote:
> \n

Done.

https://codereview.chromium.org/2754563002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/controls/settings_idle_render.js:62:
this.child_ = instance.root.firstElementChild;
On 2017/03/18 at 00:15:51, Dan Beam wrote:
> \n

Done.

https://codereview.chromium.org/2754563002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/controls/settings_idle_render.js:64:
resolve(this.child_);
On 2017/03/18 at 00:15:51, Dan Beam wrote:
> \n

Done.

https://codereview.chromium.org/2754563002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/controls/settings_idle_render.js:68:
}.bind(this));
On 2017/03/18 at 00:15:51, Dan Beam wrote:
> please use `git cl format --js` on this code

Done.

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

https://codereview.chromium.org/2754563002/diff/180001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_main/settings_main.js:93:
Polymer.dom.flush();
On 2017/03/18 at 00:41:06, Dan Beam wrote:
> On 2017/03/18 00:15:51, Dan Beam wrote:
> > should we be doing this only in certain cases?
> 
> we probably should, but let's just see if anybody notices first

Ok.

https://codereview.chromium.org/2754563002/diff/180001/chrome/test/data/webui...
File chrome/test/data/webui/settings/help_page_browsertest.js (right):

https://codereview.chromium.org/2754563002/diff/180001/chrome/test/data/webui...
chrome/test/data/webui/settings/help_page_browsertest.js:29: setUp: function() {
On 2017/03/18 at 00:15:51, Dan Beam wrote:
> please add a note that you're intentionally bypassing the super
(SettingsPageBrowserTest#setUp)

Done.

https://codereview.chromium.org/2754563002/diff/180001/chrome/test/data/webui...
File chrome/test/data/webui/settings/settings_idle_render_browsertest.js
(right):

https://codereview.chromium.org/2754563002/diff/180001/chrome/test/data/webui...
chrome/test/data/webui/settings/settings_idle_render_browsertest.js:41: '   
url="chrome://resources/html/polymer.html">' +
On 2017/03/18 at 00:15:51, Dan Beam wrote:
> why this?

I tried several, this one was the first one that worked (all others threw
various errors in the console). Furthermore, this one makes sense because it
ends up being a no-op (an HTML import for polymer.html is guaranteed to exist
when this test executes, so importHref returns early).

https://codereview.chromium.org/2754563002/diff/200001/chrome/browser/resourc...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2754563002/diff/200001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:124:
currentRoute != settings.Route.RESET_DIALOG) {
@dbeam: I discovered an issue with navigating (directly or indirectly) to those
dialogs. tryTransitionToSection_ enters endless recursion, those URLs do not
belong in a "currentSection". The added checks here fix the problem.

[dpapad, 2017-03-20 23:43:17]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2017-03-20 23:43:18]

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/2754563002/#ps220001
(title: "Fix typo")

[commit-bot: I haz the power, 2017-03-20 23:44:00]

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

[Dan Beam, 2017-03-21 00:29:26]

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

https://codereview.chromium.org/2754563002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:124:
currentRoute != settings.Route.RESET_DIALOG) {
can you do this without calling out specific routes?  or maybe make a
settings.isNavigableDialog() that takes a route?

[dpapad, 2017-03-21 00:29:54]

The CQ bit was unchecked by dpapad@chromium.org

[dpapad, 2017-03-21 00:54:50]

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

[dpapad, 2017-03-21 00:54:51]

@Tommy: Can you take a look at route.js and route_tests.js?

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

https://codereview.chromium.org/2754563002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:124:
currentRoute != settings.Route.RESET_DIALOG) {
On 2017/03/21 at 00:29:26, Dan Beam wrote:
> can you do this without calling out specific routes?  or maybe make a
settings.isNavigableDialog() that takes a route?

Done.

[Dan Beam, 2017-03-21 01:54:16]

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

https://codereview.chromium.org/2754563002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:124:
currentRoute != settings.Route.RESET_DIALOG) {
On 2017/03/21 00:54:51, dpapad wrote:
> On 2017/03/21 at 00:29:26, Dan Beam wrote:
> > can you do this without calling out specific routes?  or maybe make a
> settings.isNavigableDialog() that takes a route?
> 
> Done.

is it OK that children of BASIC are included in this?

[dpapad, 2017-03-21 05:36:36]

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

https://codereview.chromium.org/2754563002/diff/220001/chrome/browser/resourc...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:124:
currentRoute != settings.Route.RESET_DIALOG) {
On 2017/03/21 at 01:54:16, Dan Beam wrote:
> On 2017/03/21 00:54:51, dpapad wrote:
> > On 2017/03/21 at 00:29:26, Dan Beam wrote:
> > > can you do this without calling out specific routes?  or maybe make a
> > settings.isNavigableDialog() that takes a route?
> > 
> > Done.
> 
> is it OK that children of BASIC are included in this?

Yes should be fine, because the 1st part of the condition already returns false
when currentRoute is a not a child of ADVANCED.

[Dan Beam, 2017-03-21 05:41:52]

lgtm

[Dan Beam, 2017-03-21 07:13:42]

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

[commit-bot: I haz the power, 2017-03-21 07:14: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, 2017-03-21 07:25:27]

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

[commit-bot: I haz the power, 2017-03-21 07:25:28]

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...)

[Dan Beam, 2017-03-21 08:16:12]

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

https://codereview.chromium.org/2754563002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:409: * @param {!Route}
@param {!Route} route

[tommycli, 2017-03-21 16:44:57]

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

https://codereview.chromium.org/2754563002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:109: ]);
the isNavigableDialog thing is fine. I may suggest instead making it a property
of the route itself that defaults ta false... i.e.

if (route.isNavigableDialog) -- just so it's declared in the line right after
the route (so it's impossible to miss putting it in the Set), but it's not a
huge deal either way.

[dpapad, 2017-03-21 17:54:15]

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

https://codereview.chromium.org/2754563002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:109: ]);
On 2017/03/21 at 16:44:57, tommycli wrote:
> the isNavigableDialog thing is fine. I may suggest instead making it a
property of the route itself that defaults ta false... i.e.
> 
> if (route.isNavigableDialog) -- just so it's declared in the line right after
the route (so it's impossible to miss putting it in the Set), but it's not a
huge deal either way.

Done.

https://codereview.chromium.org/2754563002/diff/240001/chrome/browser/resourc...
chrome/browser/resources/settings/route.js:409: * @param {!Route}
On 2017/03/21 at 08:16:12, Dan Beam wrote:
> @param {!Route} route

Done.

[tommycli, 2017-03-21 17:56:29]

On 2017/03/21 17:54:15, dpapad wrote:
>
https://codereview.chromium.org/2754563002/diff/240001/chrome/browser/resourc...
> File chrome/browser/resources/settings/route.js (right):
> 
>
https://codereview.chromium.org/2754563002/diff/240001/chrome/browser/resourc...
> chrome/browser/resources/settings/route.js:109: ]);
> On 2017/03/21 at 16:44:57, tommycli wrote:
> > the isNavigableDialog thing is fine. I may suggest instead making it a
> property of the route itself that defaults ta false... i.e.
> > 
> > if (route.isNavigableDialog) -- just so it's declared in the line right
after
> the route (so it's impossible to miss putting it in the Set), but it's not a
> huge deal either way.
> 
> Done.
> 
>
https://codereview.chromium.org/2754563002/diff/240001/chrome/browser/resourc...
> chrome/browser/resources/settings/route.js:409: * @param {!Route}
> On 2017/03/21 at 08:16:12, Dan Beam wrote:
> > @param {!Route} route
> 
> Done.

lgtm thanks !

[dpapad, 2017-03-21 18:22:13]

The CQ bit was checked by dpapad@chromium.org

[dpapad, 2017-03-21 18:22:13]

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/2754563002/#ps260001
(title: "Address comments.")

[commit-bot: I haz the power, 2017-03-21 18:23:12]

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 20:10:42]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1490120533397280,
"parent_rev": "2f746c5c4260329c7ef5a850ce1b72d78da106a6", "commit_rev":
"f9e8f9e6d60d8cd26c4239a377419cbb37fe8724"}

[commit-bot: I haz the power, 2017-03-21 20:12:16]

Description was changed from

==========
MD Settings: Lazy load the contents of the "advanced" settings.

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

to

==========
MD Settings: Lazy load the contents of the "advanced" settings.

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

Review-Url: https://codereview.chromium.org/2754563002
Cr-Commit-Position: refs/heads/master@{#458527}
Committed:
https://chromium.googlesource.com/chromium/src/+/f9e8f9e6d60d8cd26c4239a37741...
==========

[commit-bot: I haz the power, 2017-03-21 20:12:18]

Committed patchset #12 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/f9e8f9e6d60d8cd26c4239a37741...