MD Settings: Fix Advanced Toggle when on an Advanced section route.

MD Settings: Fix Advanced Toggle when on an Advanced section route.

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

Committed: https://crrev.com/3b8a42d0c7032ffe452a6b4bd020f8be2697fdd5
Cr-Commit-Position: refs/heads/master@{#418447}

[tommycli, 2016-09-01 23:19:18]

Description was changed from

==========
MD Settings: Fix Advanced Toggle when on an Advanced section route.

BUG=643016
==========

to

==========
MD Settings: Fix Advanced Toggle when on an Advanced section route.

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

[tommycli, 2016-09-01 23:28:29]

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

[tommycli, 2016-09-01 23:28:29]

dschuyler: PTAL, thanks

[dschuyler, 2016-09-01 23:36:00]

https://codereview.chromium.org/2307703002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_main/settings_main.js (right):

https://codereview.chromium.org/2307703002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:155:
this.advancedToggleExpanded_ = settings.Route.ADVANCED.contains(newRoute);
This looks like it may collapse advanced if the
url is for basic page. Does it?

[tommycli, 2016-09-02 00:36:21]

thanks! I added a test for the error you spotted

https://codereview.chromium.org/2307703002/diff/20001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_main/settings_main.js (right):

https://codereview.chromium.org/2307703002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:155:
this.advancedToggleExpanded_ = settings.Route.ADVANCED.contains(newRoute);
On 2016/09/01 23:36:00, dschuyler wrote:
> This looks like it may collapse advanced if the
> url is for basic page. Does it?

Done. And added a test

[dschuyler, 2016-09-03 00:33:32]

On 2016/09/02 00:36:21, tommycli wrote:
> thanks! I added a test for the error you spotted
> 
>
https://codereview.chromium.org/2307703002/diff/20001/chrome/browser/resource...
> File chrome/browser/resources/settings/settings_main/settings_main.js (right):
> 
>
https://codereview.chromium.org/2307703002/diff/20001/chrome/browser/resource...
> chrome/browser/resources/settings/settings_main/settings_main.js:155:
> this.advancedToggleExpanded_ = settings.Route.ADVANCED.contains(newRoute);
> On 2016/09/01 23:36:00, dschuyler wrote:
> > This looks like it may collapse advanced if the
> > url is for basic page. Does it?
> 
> Done. And added a test

Note: I just demoed an issue to Tommy with this CL, it can be seen by:
open chrome://md-settings
Click Manage search engines
Open side nav
Select Reset

Without this CL, the page will change to the Reset settings.
With this CL it will show the top of the page (People settings).

[tommycli, 2016-09-13 22:35:40]

dschuyler: PTAL, this fixes the bad behavior from before.

[dschuyler, 2016-09-14 00:11:02]

LGTM with just an indent nit.

https://codereview.chromium.org/2307703002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_main/settings_main.js (right):

https://codereview.chromium.org/2307703002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:185:
this.advancedToggleExpanded_,
nit: the indent looks incorrect. Is it indented from 
the "this"? Would it be ok to indent four from "advanced"?

(I guess another option would be to indent line 182 off
of "settings" so that they are similar, but I'd prefer
to indent from the start of line in both cases).

[tommycli, 2016-09-14 00:12:23]

https://codereview.chromium.org/2307703002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_main/settings_main.js (right):

https://codereview.chromium.org/2307703002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/settings_main/settings_main.js:185:
this.advancedToggleExpanded_,
On 2016/09/14 00:11:02, dschuyler wrote:
> nit: the indent looks incorrect. Is it indented from 
> the "this"? Would it be ok to indent four from "advanced"?
> 
> (I guess another option would be to indent line 182 off
> of "settings" so that they are similar, but I'd prefer
> to indent from the start of line in both cases).

Done.

[tommycli, 2016-09-14 00:12:58]

The CQ bit was checked by tommycli@chromium.org

[tommycli, 2016-09-14 00:12:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/2307703002/#ps100001
(title: "fix formatting")

[commit-bot: I haz the power, 2016-09-14 00:13:38]

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-09-14 01:34:11]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-14 01:36:15]

Description was changed from

==========
MD Settings: Fix Advanced Toggle when on an Advanced section route.

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

to

==========
MD Settings: Fix Advanced Toggle when on an Advanced section route.

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

Committed: https://crrev.com/3b8a42d0c7032ffe452a6b4bd020f8be2697fdd5
Cr-Commit-Position: refs/heads/master@{#418447}
==========

[commit-bot: I haz the power, 2016-09-14 01:36:16]

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