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

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

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 /**
  * @fileoverview
  * 'settings-main' displays the selected settings page.
  */
 Polymer({
   is: 'settings-main',
 
   properties: {
-    /** @private */
-    isAdvancedMenuOpen_: {
-      type: Boolean,
-      value: false,
-    },
-
     /**
      * Preferences state.
      */
     prefs: {
       type: Object,
       notify: true,
     },
 
     /**
      * The current active route.
      * @type {!SettingsRoute}
      */
     currentRoute: {
       type: Object,
       notify: true,
       observer: 'currentRouteChanged_',
     },
 
     /** @private */
-    showAdvancedPage_: {
+    advancedToggleExpanded_: {
       type: Boolean,
       value: false,
     },
 
     /** @private */
-    showAdvancedToggle_: {
-      type: Boolean,
-      value: true,
-    },
+    inSubpage_: Boolean,
 
-    /** @private */
-    showBasicPage_: {
-      type: Boolean,
-      value: true,
-    },
-
-    /** @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: {
       type: Boolean,
       value: false,
       notify: true,
     },
   },
 
   /** @override */
   created: function() {
     /** @private {!PromiseResolver} */
     this.resolver_ = new PromiseResolver;
     settings.main.rendered = this.resolver_.promise;
   },
 
   /** @override */
   ready: function() {
     settings.getSearchManager().setCallback(function(isRunning) {
       this.toolbarSpinnerActive = isRunning;
     }.bind(this));
   },
 
   /** @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: [],
       };
     }.bind(this));
 
     doWhenReady(
         function() {
           var basicPage = this.$$('settings-basic-page');
           return !!basicPage && basicPage.scrollHeight > 0;
         }.bind(this),
         function() {
           this.resolver_.resolve();
         }.bind(this));
   },
 
   /**
    * @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';
   },
 
   /**
    * @param {!SettingsRoute} newRoute
    * @private
    */
   currentRouteChanged_: function(newRoute) {
-    var isSubpage = !!newRoute.subpage.length;
+    this.inSubpage_ = newRoute.subpage.length > 0;
+    this.style.height = this.inSubpage_ ? '100%' : '';
 
-    this.showAboutPage_ = newRoute.page == 'about';
+    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.showAdvancedToggle_ = !this.showAboutPage_ && !isSubpage;
-
-    this.showBasicPage_ = this.showAdvancedToggle_ || newRoute.page == 'basic';
-
-    this.showAdvancedPage_ =
-        (this.isAdvancedMenuOpen_ && this.showAdvancedToggle_) ||
-        newRoute.page == 'advanced';
-
-    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_);
   },
 
   /**
    * Navigates to the default search page (if necessary).
    * @private
    */
   ensureInDefaultSearchPage_: function() {
     if (this.currentRoute.page != 'basic' ||
         this.currentRoute.section != '' ||
         this.currentRoute.subpage.length != 0) {
       this.currentRoute = {page: 'basic', section: '', subpage: [], url: ''};
     }
   },
 
   /**
    * @param {string} query
    */
   searchContents: function(query) {
     this.ensureInDefaultSearchPage_();
 
     // 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')));
     }.bind(this), 0);
   },
 });