MD Settings: refactor some main page properties, fix Advanced toggle

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 b23930bcc55a02cca9df6a3e898753257a2916a0..7a4f12e0a7efd87455d7e45c8c3b748959197652 100644
--- a/chrome/browser/resources/settings/settings_main/settings_main.js
+++ b/chrome/browser/resources/settings/settings_main/settings_main.js
@@ -10,12 +10,6 @@ Polymer({
   is: 'settings-main',
 
   properties: {
-    /** @private */
-    isAdvancedMenuOpen_: {
-      type: Boolean,
-      value: false,
-    },
-
     /**
      * Preferences state.
      */
@@ -35,27 +29,24 @@ Polymer({
     },
 
     /** @private */
-    showAdvancedPage_: {
+    advancedToggleExpanded_: {
       type: Boolean,
       value: false,
     },
 
     /** @private */
-    showAdvancedToggle_: {
-      type: Boolean,
-      value: true,
-    },
-
-    /** @private */
-    showBasicPage_: {
-      type: Boolean,
-      value: true,
-    },
+    inSubpage_: Boolean,
 
-    /** @private */
-    showAboutPage_: {
-      type: Boolean,
-      value: false,
+    /**
+     * Controls which main pages are displayed via dom-ifs.
+     * @type {!{about: boolean, basic: boolean, advanced: boolean}}
+     * @private
+     */
+    showPages_: {
+      type: Object,
+      value: function() {
+        return {about: false, basic: false, advanced: false};
+      },
     },
 
     toolbarSpinnerActive: {
@@ -82,10 +73,9 @@ Polymer({
   /** @override */
   attached: function() {
     document.addEventListener('toggle-advanced-page', function(e) {
-      this.showAdvancedPage_ = e.detail;
-      this.isAdvancedMenuOpen_ = e.detail;
+      this.advancedToggleExpanded_ = e.detail;
       this.currentRoute = {
-        page: this.isAdvancedMenuOpen_ ? 'advanced' : 'basic',
+        page: this.advancedToggleExpanded_ ? 'advanced' : 'basic',
         section: '',
         subpage: [],
       };
@@ -105,7 +95,7 @@ Polymer({
    * @param {boolean} opened Whether the menu is expanded.
    * @return {string} Which icon to use.
    * @private
-   * */
+   */
   arrowState_: function(opened) {
     return opened ? 'settings:arrow-drop-up' : 'cr:arrow-drop-down';
   },
@@ -115,24 +105,27 @@ Polymer({
    * @private
    */
   currentRouteChanged_: function(newRoute) {
-    var isSubpage = !!newRoute.subpage.length;
-
-    this.showAboutPage_ = newRoute.page == 'about';
-
-    this.showAdvancedToggle_ = !this.showAboutPage_ && !isSubpage;
-
-    this.showBasicPage_ = this.showAdvancedToggle_ || newRoute.page == 'basic';
-
-    this.showAdvancedPage_ =
-        (this.isAdvancedMenuOpen_ && this.showAdvancedToggle_) ||
-        newRoute.page == 'advanced';
+    this.inSubpage_ = newRoute.subpage.length > 0;
+    this.style.height = this.inSubpage_ ? '100%' : '';
+
+    if (newRoute.page == 'about') {
+      this.showPages_ = {about: true, basic: false, advanced: false};
+    } else {
+      this.showPages_ = {
+        about: false,
+        basic: newRoute.page == 'basic' || !this.inSubpage_,
+        advanced: newRoute.page == 'advanced' ||
+            (!this.inSubpage_ && this.advancedToggleExpanded_),
+      };
 
-    this.style.height = isSubpage ? '100%' : '';
+      if (this.showPages_.advanced)
+        this.advancedToggleExpanded_ = true;

[dschuyler, 2016/07/21 19:02:05]

When would this be needed (when would it not 
be set by the toggle-advanced-page event)? If
we can, I'd like for this to be set in one 
place rather than two. It looks like a 
toggle-advanced-page event may cause this to be
set twice (once for the event and once for the
current route changed).

[michaelpg, 2016/07/21 19:55:28]

When you navigate to chrome://md-settings/advanced. Either as the first page you
visit, or using back/forward, the toggle needs to flip on its own sometimes.

So, neither the route nor the click handler has all necessary information to
determine this property in one place. (Thus the bug.)

You could argue the route should include this info, but IMO that state should
belong to settings-main as it isn't necessarily tied to navigation.
Why? It's not a complicated member or set to a complicated value.
Maybe the answer is that the user toggling whether the advanced page is shown
should not be considered a navigation event (shouldn't affect the route).

[michaelpg, 2016/07/22 15:12:17]

So this definitely needs to change to fix Advanced bugs, but this CL is probably
not the right place to do it, and we should clarify the expected behavior first.
Sound OK?

+    }
   },
 
   /** @private */
   toggleAdvancedPage_: function() {
-    this.fire('toggle-advanced-page', !this.isAdvancedMenuOpen_);
+    this.fire('toggle-advanced-page', !this.advancedToggleExpanded_);
   },
 
   /**
@@ -156,13 +149,13 @@ Polymer({
     // Trigger rendering of the basic and advanced pages and search once ready.
     // Even if those are already rendered, yield to the message loop before
     // initiating searching.
-    this.showBasicPage_ = true;
+    this.set('showPages_.basic', true);

[dschuyler, 2016/07/21 19:02:05]

With the new struct to set the basic, advanced, about
settings all at once; would it be better to set them
all at once here as well? What is the advantage of 
setting them one at a time?

[michaelpg, 2016/07/21 19:55:28]

You'd have to ask dpapad whether it matters. I don't really want to mess with
this logic right now.

[dpapad, 2016/07/21 22:31:27]

I believe that setting both booleans at once here would be equivalent.

Let me clarify a bit why I suggested packaging the three booleans in a single
object in the first place. Mostly because of readability. Packaging visibility
booleans in an object is a pattern I have encountered in other Polymer code, for
example it is also being done at https://codereview.chromium.org/2106103006 (see
pageVisibility object). It is also something I wanted to do as a cleanup at
https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/certif....

The performance aspect of assigning all booleans at once, is secondary and only
applicable if the previous version of the code was flipping booleans twice
(where the first value was basically thrown away). Looking back at the previous
patch were I suggested packaging the booleans, I don't think there was any
flipping going on. I still think this is cleaner, but not a big deal, feel free
to unpackage the booleans if you don't think is more readable, I am OK with
either way.

[michaelpg, 2016/07/22 15:12:18]

Nah, the struct of booleans is good step forward. Anyway, we'll improve this as
we continue making changes to routing/navigation so it's not set in stone.

     setTimeout(function() {
       settings.getSearchManager().search(
           query, assert(this.$$('settings-basic-page')));
     }.bind(this), 0);
 
-    this.showAdvancedPage_ = true;
+    this.set('showPages_.advanced', true);
     setTimeout(function() {
       settings.getSearchManager().search(
           query, assert(this.$$('settings-advanced-page')));