MD Settings: Move settings-advanced-page into settings-basic-page

chrome/browser/resources/settings/basic_page/basic_page.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-basic-page' is the settings page containing the basic settings.
+ * 'settings-basic-page' is the settings page containing the actual settings.
  */
 Polymer({
   is: 'settings-basic-page',
 
   behaviors: [SettingsPageVisibility, MainPageBehavior],
 
   properties: {
     /** Preferences state. */
     prefs: {
       type: Object,
       notify: true,
     },
 
     /**
+     * Dictionary defining page visibility.
+     * @type {!GuestModePageVisibility}
+     */
+    pageVisibility: Object,
+
+    advancedToggleExpanded: {
+      type: Boolean,
+      notify: true,
+    },
+
+    /**
+     * Whether a search operation is in progress or previous search results are
+     * being displayed.
+     */
+    inSearchMode: {
+      type: Boolean,
+      value: false,
+    },
+
+    /**
+     * True if a section is fully expanded to hide other sections beneath it.
+     * Not true otherwise (even while animating a section open/closed).

[dpapad, 2016/12/02 19:45:53]

Nit: s/Not true/False

[michaelpg, 2016/12/08 03:31:59]

Done.

+     * @private {boolean}
+     */
+    hasExpandedSection_: {
+      type: Boolean,
+      value: false,
+    },
+
+    /**
      * True if the basic page should currently display the reset profile banner.
      * @private {boolean}
      */
     showResetProfileBanner_: {
       type: Boolean,
       value: function() {
         return loadTimeData.getBoolean('showResetProfileBanner');
       },
     },
+
+    /** @private {!settings.Route|undefined} */
+    currentRoute_: Object,
+  },
+
+  listeners: {
+    'subpage-expand': 'onSubpageExpanded_',
+  },
+
+  attached: function() {

[dpapad, 2016/12/02 19:45:54]

Nit: @override

[michaelpg, 2016/12/08 03:31:59]

Done.

+    this.currentRoute_ = settings.getCurrentRoute();
+  },
+
+  currentRouteChanged: function(newRoute, oldRoute) {

[dpapad, 2016/12/02 19:45:53]

@param annotations missing.

Also, can you mention in a comment from which Polymer behavior do we inherit
this method (guessing MainPageBehavior->RoutableObserverBehavior)? This will
save time to future readers.

[michaelpg, 2016/12/08 03:31:59]

Done. (Can't use @override because it's overriding a behavior.)

+    this.currentRoute_ = newRoute;
+
+    if (settings.Route.ADVANCED.contains(newRoute))
+      this.advancedToggleExpanded = true;
+
+    // When the route changes away from a sub-page, immediately update
+    // hasExpandedSection_ to unhide the other sections.
+    if (oldRoute && !(newRoute.isSubpage() && oldRoute.isSubpage() &&
+          newRoute.section == oldRoute.section)) {

[dpapad, 2016/12/02 19:45:53]

Indentation is 6 from previous line. Shouldn't it be 4 instead?

[michaelpg, 2016/12/08 03:31:59]

6 is a mistake. 8 might be better because it's an extra paren level. But
rewriting it is even better IMHO.

+      this.hasExpandedSection_ = false;
+    }
+
+    MainPageBehaviorImpl.currentRouteChanged.call(this, newRoute, oldRoute);
+  },
+
+  /**
+   * Queues a task to search the basic sections, then another for the advanced
+   * sections.
+   * @param {string} query The text to search for.
+   * @return {!Promise<!settings.SearchRequest>} A signal indicating that
+   *     searching finished.
+   */
+  searchContents: function(query) {
+    var whenSearchDone = settings.getSearchManager().search(
+        query, assert(this.$$('#basicPage')));
+
+    if (this.pageVisibility.advancedSettings !== false) {
+      assert(whenSearchDone === settings.getSearchManager().search(
+          query, assert(this.$$('#advancedPage'))));
+    }
+
+    return whenSearchDone;
   },
 
   onResetDone_: function() {

[dpapad, 2016/12/02 19:45:54]

@private

[michaelpg, 2016/12/08 03:31:59]

already done by something else i think?

     this.showResetProfileBanner_ = false;
   },
+
+  /**
+   * Hides everything but the newly expanded subpage.
+   * @private
+   */
+  onSubpageExpanded_: function() {
+    this.hasExpandedSection_ = true;
+  },
+
+  /**
+   * @param {boolean} inSearchMode
+   * @param {boolean} hasExpandedSection
+   * @return {boolean}
+   * @private
+   */
+  showAdvancedToggle_: function(inSearchMode, hasExpandedSection) {
+    return !inSearchMode && !hasExpandedSection;
+  },
+
+  /**
+   * @param {!settings.Route} currentRoute
+   * @param {boolean} inSearchMode
+   * @param {boolean} hasExpandedSection
+   * @return {boolean} Whether to show the basic page, taking into account
+   *     both routing and search state.
+   * @private
+   */
+  showBasicPage_: function(currentRoute, inSearchMode, hasExpandedSection) {
+    return !hasExpandedSection || settings.Route.BASIC.contains(currentRoute);
+  },
+
+  /**
+   * @param {!settings.Route} currentRoute
+   * @param {boolean} inSearchMode
+   * @param {boolean} hasExpandedSection
+   * @param {boolean} advancedToggleExpanded
+   * @return {boolean} Whether to show the advanced page, taking into account
+   *     both routing and search state.
+   * @private
+   */
+  showAdvancedPage_: function(currentRoute, inSearchMode, hasExpandedSection,
+                              advancedToggleExpanded) {
+    if (hasExpandedSection)

[dpapad, 2016/12/02 19:45:54]

Nit:
return hasExpandedSection ?
    settings.Route.ADVANCED.contains(currentRoute) :
    advancedToggleExpanded || inSearchMode;

[michaelpg, 2016/12/08 03:31:58]

Done.

+      return settings.Route.ADVANCED.contains(currentRoute);
+
+    return advancedToggleExpanded || inSearchMode;
+  },
+
+  /**
+   * @param {(boolean|undefined)} visibility
+   * @return {boolean} True unless visibility is false.
+   * @private
+   */
+  showAdvancedSettings_: function(visibility) {
+    return visibility !== false;
+  },
+
+  /**
+   * @param {boolean} opened Whether the menu is expanded.
+   * @return {string} Which icon to use.
+   * @private
+   */
+  arrowState_: function(opened) {

[dpapad, 2016/12/02 19:45:53]

Nit: How about renaming to|getIcon_|? Seems more explicit and self-documenting.

[michaelpg, 2016/12/08 03:31:59]

Done.

+    return opened ? 'settings:arrow-drop-up' : 'cr:arrow-drop-down';
+  },
 });