Settings Router Refactor: Improve chrome://md-settings/foobar behavior.

Settings Router Refactor: Improve chrome://md-settings/foobar behavior.

Correctly reset the URL path to '/' when the user directly navigates
to a non-existent URL.

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

Committed: https://crrev.com/6c7966816ea0d4304c1caba897668ec12964e7eb
Cr-Commit-Position: refs/heads/master@{#409252}

[tommycli, 2016-08-01 23:37:10]

Description was changed from

==========
Settings Router Refactor: Improve chrome://md-settings/foobar behavior.

Correctly reset the URL path to '/' when the user directly navigates
to a non-existent URL.

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

to

==========
Settings Router Refactor: Improve chrome://md-settings/foobar behavior.

Correctly reset the URL path to '/' when the user directly navigates
to a non-existent URL.

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

[tommycli, 2016-08-01 23:38:18]

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

[tommycli, 2016-08-01 23:38:18]

dbeam: PTAL, thanks

https://codereview.chromium.org/2200013002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/route.js (right):

https://codereview.chromium.org/2200013002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/route.js:324:
window.history.pushState(undefined, '', route.path);
It's unused (by both Firefox and Chrome), and MDN recommends setting it an empty
string to be robust to future changes.

[Dan Beam, 2016-08-01 23:39:20]

https://codereview.chromium.org/2200013002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/route.js (right):

https://codereview.chromium.org/2200013002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/route.js:300:
window.history.replaceState(undefined, '', currentRoute_.path);
can we do this only if currentRoute_ is null?

https://codereview.chromium.org/2200013002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/route.js:324:
window.history.pushState(undefined, '', route.path);
On 2016/08/01 23:38:18, tommycli wrote:
> It's unused (by both Firefox and Chrome), and MDN recommends setting it an
empty
> string to be robust to future changes.

Acknowledged.

[tommycli, 2016-08-02 00:00:40]

dbeam: Thanks. It became a bit longer because I didn't want to leak temp vars,
but open to suggestions...

https://codereview.chromium.org/2200013002/diff/1/chrome/browser/resources/se...
File chrome/browser/resources/settings/route.js (right):

https://codereview.chromium.org/2200013002/diff/1/chrome/browser/resources/se...
chrome/browser/resources/settings/route.js:300:
window.history.replaceState(undefined, '', currentRoute_.path);
On 2016/08/01 23:39:19, Dan Beam wrote:
> can we do this only if currentRoute_ is null?

Done.

[Dan Beam, 2016-08-02 00:11:04]

https://codereview.chromium.org/2200013002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/route.js (right):

https://codereview.chromium.org/2200013002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:307: })();
how would this leak global variables?

var currentRoute_ = getRouteForPath(window.location.pathname);
if (!currentRoute_) {
  currentRoute_ = Route.BASIC;
  window.history.replaceState(undefined, '', Route.BASIC.path);
}

does closure complain because this yields ?settings.Route for 1 statement?

[tommycli, 2016-08-02 00:14:29]

https://codereview.chromium.org/2200013002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/settings/route.js (right):

https://codereview.chromium.org/2200013002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:307: })();
On 2016/08/02 00:11:04, Dan Beam wrote:
> how would this leak global variables?
> 
> var currentRoute_ = getRouteForPath(window.location.pathname);
> if (!currentRoute_) {
>   currentRoute_ = Route.BASIC;
>   window.history.replaceState(undefined, '', Route.BASIC.path);
> }
> 
> does closure complain because this yields ?settings.Route for 1 statement?

Yeah, it complains :D

[Dan Beam, 2016-08-02 00:26:59]

lgtm

[Dan Beam, 2016-08-02 00:27:09]

might be worth adding a test for this behavior, though

[tommycli, 2016-08-02 16:12:06]

On 2016/08/02 00:27:09, Dan Beam wrote:
> might be worth adding a test for this behavior, though

Sounds good, I'll add a test.

[tommycli, 2016-08-02 17:23:14]

dbeam: PTAL, I added a test. Thanks!

[Dan Beam, 2016-08-02 17:23:54]

lgtm++

[tommycli, 2016-08-02 17:24:38]

On 2016/08/02 17:23:54, Dan Beam wrote:
> lgtm++

Thanks! sending to CQ

[tommycli, 2016-08-02 17:24:43]

The CQ bit was checked by tommycli@chromium.org

[commit-bot: I haz the power, 2016-08-02 17:25:36]

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

[michaelpg, 2016-08-02 17:27:34]

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

https://codereview.chromium.org/2200013002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:305:
window.history.replaceState(undefined, '', Route.BASIC.path);
just a thought, maybe we should use ADVANCED (but not scroll, just enable the
advanced toggle) to make it slightly more likely they find whatever they were
pointed to?

[tommycli, 2016-08-02 17:33:25]

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

https://codereview.chromium.org/2200013002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:305:
window.history.replaceState(undefined, '', Route.BASIC.path);
On 2016/08/02 17:27:33, michaelpg wrote:
> just a thought, maybe we should use ADVANCED (but not scroll, just enable the
> advanced toggle) to make it slightly more likely they find whatever they were
> pointed to?

blah - that advanced toggle has always seemed lame to me. Is there any chance
UI/PM would be Okay with just effectively having ADVANCED always toggled open?

(and making them all children of BASIC)?

[Dan Beam, 2016-08-02 17:39:13]

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

https://codereview.chromium.org/2200013002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/route.js:305:
window.history.replaceState(undefined, '', Route.BASIC.path);
On 2016/08/02 17:33:25, tommycli wrote:
> On 2016/08/02 17:27:33, michaelpg wrote:
> > just a thought, maybe we should use ADVANCED (but not scroll, just enable
the
> > advanced toggle) to make it slightly more likely they find whatever they
were
> > pointed to?
> 
> blah - that advanced toggle has always seemed lame to me. Is there any chance
> UI/PM would be Okay with just effectively having ADVANCED always toggled open?
> 
> (and making them all children of BASIC)?

I think the goal is to have both Advanced toggles (in-page, in the side nav) not
do any navigation nor scrolling.  They should simple open up more UI, and that
UI should somehow be visible through creative padding.

bettes, tbuckley, dschuyler and I have talked about this and we just hadn't
conveyed this to the larger group / figured out exactly how to do this yet. 
it'll eventually just work like old options, I think, and just reveal the top of
more UI that the user can choose to explore (or something).

[commit-bot: I haz the power, 2016-08-02 18:28:06]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-02 18:30:15]

Description was changed from

==========
Settings Router Refactor: Improve chrome://md-settings/foobar behavior.

Correctly reset the URL path to '/' when the user directly navigates
to a non-existent URL.

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

to

==========
Settings Router Refactor: Improve chrome://md-settings/foobar behavior.

Correctly reset the URL path to '/' when the user directly navigates
to a non-existent URL.

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

Committed: https://crrev.com/6c7966816ea0d4304c1caba897668ec12964e7eb
Cr-Commit-Position: refs/heads/master@{#409252}
==========

[commit-bot: I haz the power, 2016-08-02 18:30:16]

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