Settings Router Refactor: Remove route.page legacy property.

chrome/browser/resources/settings/settings_main/settings_main.js

Index: chrome/browser/resources/settings/settings_main/settings_main.js
diff --git a/chrome/browser/resources/settings/settings_main/settings_main.js b/chrome/browser/resources/settings/settings_main/settings_main.js
index 7150161af7458d9391d1995fa79bcc6cdbf432e2..6c1c303137d5a831574a2d56cab74fd785dc64f4 100644
--- a/chrome/browser/resources/settings/settings_main/settings_main.js
+++ b/chrome/browser/resources/settings/settings_main/settings_main.js
@@ -112,13 +112,14 @@ Polymer({
     this.inSubpage_ = newRoute.subpage.length > 0;
     this.style.height = this.inSubpage_ ? '100%' : '';
 
-    if (newRoute.page == 'about') {
+    if (newRoute.inSubtreeOf(settings.Route.ABOUT)) {

[michaelpg, 2016/07/28 23:22:32]

so, what we care about is what is the *root* of newRoute. I think 'page' was
useful for providing that info, even if the name sucks.

I move to create a read-only 'root' property of settings.Route which provides
the root of the route (one of ABOUT, BASIC or ADVANCED). here, we would just
check (newRoute.root == settings.Route.ABOUT), instead of asking if we're equal
to or a descendant of ABOUT.

[tommycli, 2016/07/29 00:03:35]

So... I would create a 'root', except that we also need to know if we are within
a section (to get rid of the section property).

I think the isEqualOrInSubtreeOf concept allows us to cover both cases without
adding more properties.

[michaelpg, 2016/07/29 00:21:12]

in a later CL, you mean? Why do we want to get rid of .section? It's a concept
of its own, that represents something specific in the UI.

Will we have to mandate that any route that's a child of a root route has to
correspond to a section? If so, why pretend section isn't structural? If not,
how do we identify whether something is a section or a sub-page?

I guess I don't know why we have to get rid of the concepts we've been using the
whole time. We can achieve the objectives in
https://docs.google.com/document/d/1nI22YBBNeYnBCB07wR7QL_k6NmGmfPI_G7A8l5-YTUM/
without removing convenient properties.

[michaelpg, 2016/07/29 00:29:14]

just realized I'm conflating .section and .subpage, though presumably we're
removing both properties. I see that checking route.section is the same as
checking route.isEqualOrDescendantOf(sectionRoute). and we can still check if
something is a subpage via route.subpage.length.

Do the guys in your office think we already reached consensus on removing these
properties? If so then let's do it.

Regarding the function itself, I concede route.isEqualOrDescendantOf(ROOT_PAGE)
is equivalent to isRootedAt(ROOT_PAGE) when ROOT_PAGE is a root page, so I guess
we don't need a separate function/property for it... it's not the simplest API
but it keeps it small.

[tommycli, 2016/07/29 18:23:43]

Hey, I thought about the variety of things you said and talked to Dan about
this.

Dan suggested changing this to parent.contains(child). This follows the same
semantics as Node.contains, which is also inclusive.

https://developer.mozilla.org/en-US/docs/Web/API/Node/contains

[michaelpg, 2016/07/29 20:28:53]

"The contains(other) method must return true if other is an inclusive descendant
of the context object"

Well then... can't argue with crazy ;-)

       this.showPages_ = {about: true, basic: false, advanced: false};
     } else {
       this.showPages_ = {
         about: false,
-        basic: newRoute.page == 'basic' || !this.inSubpage_,
-        advanced: newRoute.page == 'advanced' ||
+        basic:
+            newRoute.inSubtreeOf(settings.Route.BASIC) || !this.inSubpage_,
+        advanced: newRoute.inSubtreeOf(settings.Route.ADVANCED) ||
             (!this.inSubpage_ && this.advancedToggleExpanded_),
       };