MD Settings: fix infinite setInterval loop

MD Settings: fix infinite setInterval loop

Pages that are not shown (but have been stamped) enter an infinite doWhenReady
loop when transitioning to another page. We shouldn't use the doWhenReady hack
if the page shouldn't be shown anyway.

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

Committed: https://crrev.com/0e52c54ca4ad014ba5ac13451d1da456b06a4b80
Cr-Commit-Position: refs/heads/master@{#416014}

[michaelpg, 2016-09-01 01:58:15]

Description was changed from

==========
MD Settings: fix infinite setInterval loop

Pages that are not shown (but have been stamped) enter an infinite doWhenReady
loop when transitioning to another page. We shouldn't use the doWhenReady hack
if the page shouldn't be shown anyway.

BUG=643022
==========

to

==========
MD Settings: fix infinite setInterval loop

Pages that are not shown (but have been stamped) enter an infinite doWhenReady
loop when transitioning to another page. We shouldn't use the doWhenReady hack
if the page shouldn't be shown anyway.

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

[michaelpg, 2016-09-01 01:59:57]

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

[michaelpg, 2016-09-01 01:59:58]

shotgunning the review to include interested parties

[Dan Beam, 2016-09-01 03:05:15]

lgtm but can haz test?

https://codereview.chromium.org/2302703002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2302703002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:65: //
Height is irrelevant if this page isn't the current page.
nit:  // This is not the page you're looking for.

</kenobi>

[michaelpg, 2016-09-01 18:08:01]

The CQ bit was checked by michaelpg@chromium.org

[commit-bot: I haz the power, 2016-09-01 18:08:40]

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

[michaelpg, 2016-09-01 18:09:42]

On 2016/09/01 03:05:15, Dan Beam wrote:
> lgtm but can haz test?

tests are sorely needed IMO, would greatly reduce breakages in features, flows
and edge cases that are too numerous for humans to look for

but, doWhenReady should go away rather soon as we now know why we needed the
hack. general-purpose transition tests would cover this after that.

> 
>
https://codereview.chromium.org/2302703002/diff/1/chrome/browser/resources/se...
> File chrome/browser/resources/settings/settings_page/main_page_behavior.js
> (right):
> 
>
https://codereview.chromium.org/2302703002/diff/1/chrome/browser/resources/se...
> chrome/browser/resources/settings/settings_page/main_page_behavior.js:65: //
> Height is irrelevant if this page isn't the current page.
> nit:  // This is not the page you're looking for.
> 
> </kenobi>

[tommycli, 2016-09-01 18:14:24]

https://codereview.chromium.org/2302703002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2302703002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:66: if
(!settings.Route[this.route].contains(settings.getCurrentRoute()))
holy shit that's clever.

[dschuyler, 2016-09-01 18:28:42]

https://codereview.chromium.org/2302703002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_main/settings_main.html (right):

https://codereview.chromium.org/2302703002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_main/settings_main.html:62:
prefs="{{prefs}}"
nit: unwrap

https://codereview.chromium.org/2302703002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_main/settings_main.html:81:
prefs="{{prefs}}"
nit: unwrap

https://codereview.chromium.org/2302703002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2302703002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:65: //
Height is irrelevant if this page isn't the current page.
nit: I suggest dropping the comment (rather 
than changing it - though the change is funny).

[commit-bot: I haz the power, 2016-09-01 19:20:39]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-09-01 19:23:47]

Description was changed from

==========
MD Settings: fix infinite setInterval loop

Pages that are not shown (but have been stamped) enter an infinite doWhenReady
loop when transitioning to another page. We shouldn't use the doWhenReady hack
if the page shouldn't be shown anyway.

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

to

==========
MD Settings: fix infinite setInterval loop

Pages that are not shown (but have been stamped) enter an infinite doWhenReady
loop when transitioning to another page. We shouldn't use the doWhenReady hack
if the page shouldn't be shown anyway.

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

Committed: https://crrev.com/0e52c54ca4ad014ba5ac13451d1da456b06a4b80
Cr-Commit-Position: refs/heads/master@{#416014}
==========

[commit-bot: I haz the power, 2016-09-01 19:23:49]

Patchset 1 (id:??) landed as
https://crrev.com/0e52c54ca4ad014ba5ac13451d1da456b06a4b80
Cr-Commit-Position: refs/heads/master@{#416014}

[michaelpg, 2016-09-01 19:34:46]

https://codereview.chromium.org/2302703002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2302703002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:66: if
(!settings.Route[this.route].contains(settings.getCurrentRoute()))
On 2016/09/01 18:14:24, tommycli wrote:
> holy shit that's clever.

I was hoping to just do:

    if (this.domHost.getPage(settings.getCurrentRoute()) != this)

but then we'd have a cyclic dependency between settings_main and
main_page_behavior. There are also other problems, thanks to closure not
understanding the concept of behaviors, that would make it cumbersome.

Anyway, everything will continue being overly complicated until we move most of
MainPageBehavior's responsibilities to settings-main. It's crazy we let two
elements (advanced and basic) control the same scroller.

[dschuyler, 2016-09-01 20:38:02]

https://codereview.chromium.org/2302703002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/settings_page/main_page_behavior.js
(right):

https://codereview.chromium.org/2302703002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/settings_page/main_page_behavior.js:69: return
false;
Please combine these into:

return this.scrollHeight > 0 ||
    !settings.Route[this.route].contains(settings.getCurrentRoute());