[MD settings] refine routing function

[MD settings] refine routing function

This CL adds a way for animated pages to detect which page a subpage
resides within. Previously, the page was assumed to be the same as the
currentRoute page. That's still true at the moment, but when we change
to having the basic and advanced pages shown together, the currentRoute
page may not match up correctly. This CL sets the page more explicitly
so that the user can jump between basic and advanced subpages.

This is a step toward showing basic and advanced settings together.

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

Committed: https://crrev.com/ffbbd3f14353bf8d981ccbddff84a48a8326c936
Cr-Commit-Position: refs/heads/master@{#396615}

[dschuyler, 2016-05-26 22:43:49]

Description was changed from

==========
[MD settings] refine routing function

This CL adds a way for animated pages to detect which page a subpage
resides within. Previously, the page was assumed to be the same as the
currentRoute page. That's still true at the moment, but when we change
to having the basic and advanced pages shown together, the currentRoute
page may not match up correctly. This CL sets the page more explicitly
so that the user can jump between basic and advanced subpages.

This is a step toward showing basic and advanced settings together.

BUG=593989
==========

to

==========
[MD settings] refine routing function

This CL adds a way for animated pages to detect which page a subpage
resides within. Previously, the page was assumed to be the same as the
currentRoute page. That's still true at the moment, but when we change
to having the basic and advanced pages shown together, the currentRoute
page may not match up correctly. This CL sets the page more explicitly
so that the user can jump between basic and advanced subpages.

This is a step toward showing basic and advanced settings together.

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

[dschuyler, 2016-05-26 22:48:29]

Patchset #1 (id:1) has been deleted

[dschuyler, 2016-05-26 22:50:45]

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

[Dan Beam, 2016-05-26 22:58:04]

drive-by

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

https://codereview.chromium.org/2018753002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:152:
var page = node.dataset.page;
putting a `var` here is a little funky to me

https://codereview.chromium.org/2018753002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:153:
node = node.host ? node.host : node.parentNode;
node = node.host || node.parentNode;

[dschuyler, 2016-05-26 23:08:02]

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

https://codereview.chromium.org/2018753002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:152:
var page = node.dataset.page;
On 2016/05/26 22:58:04, Dan Beam wrote:
> putting a `var` here is a little funky to me

Done.

https://codereview.chromium.org/2018753002/diff/20001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:153:
node = node.host ? node.host : node.parentNode;
On 2016/05/26 22:58:04, Dan Beam wrote:
> node = node.host || node.parentNode;

Done.

[dpapad, 2016-05-27 00:44:23]

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

https://codereview.chromium.org/2018753002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:149:
var node = event.currentTarget;
Where is |event| coming from?

https://codereview.chromium.org/2018753002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:154:
node = node.host || node.parentNode;
What is the expected type(s) of |node|? I recognize "parentNode" as
https://developer.mozilla.org/en-US/docs/Web/API/Node/parentNode, but don't
recognize "host". Could you add @type annotatino or a comment?

[dschuyler, 2016-05-27 01:30:49]

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

https://codereview.chromium.org/2018753002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:149:
var node = event.currentTarget;
On 2016/05/27 00:44:22, dpapad wrote:
> Where is |event| coming from?

I commented that it's a global, but there
is still the question of whether this should
be plumbed through from the caller instead
of using the global. (Note that we're not
creating the global for this, it already 
exists).

Done.

https://codereview.chromium.org/2018753002/diff/40001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:154:
node = node.host || node.parentNode;
On 2016/05/27 00:44:22, dpapad wrote:
> What is the expected type(s) of |node|? I recognize "parentNode" as
> https://developer.mozilla.org/en-US/docs/Web/API/Node/parentNode, but don't
> recognize "host". Could you add @type annotatino or a comment?

Done.

[dpapad, 2016-05-27 17:18:45]

Before we decide whether using window.event is OK, I have a more general design
question for this CL.

My understanding of the upcoming changes to Basic and Advanced pages is that
there will be only one top-level page (without taking into account the About
page). Wouldn't the currentRoute page always match the subpage parent? If my
understanding is valid, then I don't think we need to change anything in the
routing function, do we?

[dschuyler, 2016-05-27 20:29:37]

On 2016/05/27 17:18:45, dpapad wrote:
> Before we decide whether using window.event is OK, I have a more general
design
> question for this CL.
> 
> My understanding of the upcoming changes to Basic and Advanced pages is that
> there will be only one top-level page (without taking into account the About
> page). Wouldn't the currentRoute page always match the subpage parent? If my
> understanding is valid, then I don't think we need to change anything in the
> routing function, do we?

dpapad@ and I chatted about this offline. There are several ways to approach 
tracking router information with what should be shown. They all seemed fine 
with none being clearly better. We discussed:
- marking the pages with an attribute, like 
  data-page="basic" (as done in this CL)
- removing the "basic" and "advanced" names from 
  the router and using a flag instead
- not using an attribute or a flag and stepping up 
  through parents in the dom with a heuristic to workout 
  whether the current section is in a collapsible element.

We also discussed whether it was better to have our current three file structure
with a main html file referring to basic and advanced files; vs. making one file
that contains the contents of those files in one longer file. Neither seems so
compelling that it warrants changing from either to the other.

[dpapad, 2016-05-27 20:39:46]

LGTM

Thanks for taking the time to consider alternative approaches. I agree that it
is not clear yet whether flattening the advanced page within the basic page
would simplify things, so leaving as is SGTM.

[dschuyler, 2016-05-27 21:52:59]

The CQ bit was checked by dschuyler@chromium.org

[commit-bot: I haz the power, 2016-05-27 21:53:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2018753002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2018753002/60001

[commit-bot: I haz the power, 2016-05-27 22:10:01]

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

[commit-bot: I haz the power, 2016-05-27 22:10:02]

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

[Dan Beam, 2016-05-27 22:24:06]

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

https://codereview.chromium.org/2018753002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:149:
// The event var is a global reference to the current event.
use window.event then... also, ew

[dschuyler, 2016-05-28 00:09:03]

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

https://codereview.chromium.org/2018753002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:149:
// The event var is a global reference to the current event.
On 2016/05/27 22:24:06, Dan Beam wrote:
> use window.event then... also, ew

Done.

[dschuyler, 2016-05-28 00:39:33]

The CQ bit was checked by dschuyler@chromium.org

[dschuyler, 2016-05-28 00:39:33]

The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org
Link to the patchset: https://codereview.chromium.org/2018753002/#ps80001
(title: "review change")

[commit-bot: I haz the power, 2016-05-28 00:40:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2018753002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2018753002/80001

[commit-bot: I haz the power, 2016-05-28 00:55:38]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-05-28 00:57:09]

Description was changed from

==========
[MD settings] refine routing function

This CL adds a way for animated pages to detect which page a subpage
resides within. Previously, the page was assumed to be the same as the
currentRoute page. That's still true at the moment, but when we change
to having the basic and advanced pages shown together, the currentRoute
page may not match up correctly. This CL sets the page more explicitly
so that the user can jump between basic and advanced subpages.

This is a step toward showing basic and advanced settings together.

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

to

==========
[MD settings] refine routing function

This CL adds a way for animated pages to detect which page a subpage
resides within. Previously, the page was assumed to be the same as the
currentRoute page. That's still true at the moment, but when we change
to having the basic and advanced pages shown together, the currentRoute
page may not match up correctly. This CL sets the page more explicitly
so that the user can jump between basic and advanced subpages.

This is a step toward showing basic and advanced settings together.

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

Committed: https://crrev.com/ffbbd3f14353bf8d981ccbddff84a48a8326c936
Cr-Commit-Position: refs/heads/master@{#396615}
==========

[commit-bot: I haz the power, 2016-05-28 00:57:10]

Patchset 4 (id:??) landed as
https://crrev.com/ffbbd3f14353bf8d981ccbddff84a48a8326c936
Cr-Commit-Position: refs/heads/master@{#396615}