[CrOS MultiDevice]: Move settings-section up DOM tree

The settings-basic-page is designed under the assumption that all
settings-section elements are direct children of the
settings-basic-page. As a result, our previous design of the
settings-multidevice-section-container
(now settings-multidevice-page-container) was very fragile because it
put the multidevice settings-section inside the container. See
https://chromium-review.googlesource.com/c/chromium/src/+/1105185 for
context and
https://chromium-review.googlesource.com/c/chromium/src/+/1107168 for
an example of a bug fix required by this problem.

This CL maintains the settings-multidevice-page-container's ability
to hide the entire multidevice settings-section and avoid attaching
the settings-multidevice-page while putting the settings-section where
the settings-basic-page assumes it is.

Manual checks (all successful) included
    (a) the section appears immediately after loading
    (b) the mode_ property of settings-multidevice-page-container
        toggles the section's presence (including the title) multiple
        times
    (c) the section disappears when other subpages are opened (i.e. it
        does not undo the fix from CL 1107168)
    (d) other animations (e.g. opening subpages) run smoothly
    (e) the correct mode is passed to the settings-multidevice-page,
        displaying the appropriate content
    (f) all buttons work as intended after toggling, including the Set
        up" button opening the multidevice setup dialog

Cq-Include-Trybots: luci.chromium.try:closure_compilation
Change-Id: I10470e8c655a55f69d6a14afa4a719d2050e2bda
Reviewed-on: https://chromium-review.googlesource.com/1109310Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Commit-Queue: Jordy Greenblatt <jordynass@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570253}

chrome/browser/resources/settings/basic_page/basic_page.html

@@ -21,7 +21,7 @@
 <link rel="import" href="../crostini_page/crostini_page.html">
 <link rel="import" href="../device_page/device_page.html">
 <link rel="import" href="../internet_page/internet_page.html">
-<link rel="import" href="../multidevice_page/multidevice_section_container.html">
+<link rel="import" href="../multidevice_page/multidevice_page_container.html">
 </if>
 
 <if expr="not chromeos">
@@ -111,10 +111,16 @@
           </settings-section>
         </template>
         <template is="dom-if"
-            if="[[shouldShowMultidevice_(showMultidevice, pageVisibility,
-                hasExpandedSection_, currentRoute_)]]" restamp>
-          <settings-multidevice-section-container prefs="{{prefs}}">
-          </settings-multidevice-section-container>
+            if="[[canShowMultideviceSection_(showMultidevice, pageVisibility)]]"
+            restamp>
+          <settings-section page-title="$i18n{multidevicePageTitle}"
+              section="multidevice"
+              hidden$="[[!doesAccountSupportMultiDeviceSection_]]">
+            <settings-multidevice-page-container prefs="{{prefs}}"
+                does-potential-connected-phone-exist=
+                    "{{doesAccountSupportMultiDeviceSection_}}">
+            </settings-multidevice-page-container>
+          </settings-section>
         </template>
         <template is="dom-if" if="[[showPage_(pageVisibility.bluetooth)]]"
             restamp>

chrome/browser/resources/settings/basic_page/basic_page.js

@@ -85,6 +85,16 @@ Polymer({
       type: Boolean,
       computed: 'computeShowSecondaryUserBanner_(hasExpandedSection_)',
     },
+
+    /**
+     * Whether the account supports the features controlled in the multidevice
+     * section.
+     * @private {boolean}
+     */
+    doesAccountSupportMultiDeviceSection_: {
+      type: Boolean,
+      value: false,
+    },
     // </if>
 
     /** @private {!settings.Route|undefined} */
@@ -233,15 +243,14 @@ Polymer({
   },
 
   /**
-   * @return {boolean} Whether to show the multidevice settings page.
+   * @return {boolean} Whether the account supports the features managed in
+   * this section.
    * @private
    */
-  shouldShowMultidevice_: function() {
+  canShowMultideviceSection_: function() {
     const visibility = /** @type {boolean|undefined} */ (
         this.get('pageVisibility.multidevice'));
-    return this.showMultidevice && this.showPage_(visibility) &&
-        (settings.routes.MULTIDEVICE.contains(this.currentRoute_) ||
-         !this.hasExpandedSection_);
+    return this.showMultidevice && this.showPage_(visibility);
   },
 
   /**

chrome/browser/resources/settings/multidevice_page/multidevice_page_container.html

+<link rel="import" href="chrome://resources/html/polymer.html">
+
+<link rel="import" href="multidevice_page.html">
+
+<dom-module id="settings-multidevice-page-container">
+  <template>
+    <template is="dom-if" if="[[doesPotentialConnectedPhoneExist]]" restamp>
+      <settings-multidevice-page prefs="[[prefs]]" mode="[[mode_]]">
+      </settings-multidevice-page>
+    </template>
+  </template>
+  <script src="multidevice_page_container.js"></script>
+</dom-module>

chrome/browser/resources/settings/multidevice_page/multidevice_page_container.js

+// Copyright 2018 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 Container element that interfaces with the
+ * MultiDeviceBrowserProxy to ensure that if there is not a host device or
+ * potential host device
+ *     (a) the entire multidevice settings-section is hidden and
+ *     (b) the settings-multidevice-page is not attched to the DOM.
+ * It also provides the settings-multidevice-page with the data it needs to
+ * provide the user with the correct infomation and call(s) to action based on
+ * the data retrieved from the browser proxy (i.e. the mode_ property).
+ */
+
+Polymer({
+  is: 'settings-multidevice-page-container',
+
+  properties: {
+    /** SettingsPrefsElement 'prefs' Object reference. See prefs.js. */
+    prefs: {
+      type: Object,
+      notify: true,
+    },
+
+    // TODO(jordynass): Set this based on data retrieved by browser proxy.
+    /** @type {settings.MultiDeviceSettingsMode} */
+    mode_: Number,
+
+    /**
+     * Whether a phone was found on the account that is either connected to the
+     * Chromebook or has the potential to be.
+     * @type {boolean}
+     */
+    doesPotentialConnectedPhoneExist: {
+      notify: true,
+      type: Boolean,
+      computed: 'computeDoesPotentialConnectedPhoneExist(mode_)',
+    },
+  },
+
+  /** @override */
+  ready: function() {
+    this.mode_ = settings.MultiDeviceSettingsMode.NO_HOST_SET;
+  },
+
+  /**
+   * @return {boolean}
+   * @private
+   */
+  computeDoesPotentialConnectedPhoneExist: function() {
+    return this.mode_ != settings.MultiDeviceSettingsMode.NO_ELIGIBLE_HOSTS;
+  },
+});

chrome/browser/resources/settings/multidevice_page/multidevice_section_container.html

-<link rel="import" href="chrome://resources/html/polymer.html">
-
-<link rel="import" href="../settings_page/settings_section.html">
-<link rel="import" href="multidevice_page.html">
-
-<dom-module id="settings-multidevice-section-container">
-  <template>
-    <template is="dom-if" if="[[hostFound_(mode_)]]" restamp>
-      <settings-section page-title="$i18n{multidevicePageTitle}"
-          section="multidevice">
-        <settings-multidevice-page prefs="[[prefs]]" mode="[[mode_]]">
-        </settings-multidevice-page>
-      </settings-section>
-    </template>
-  </template>
-  <script src="multidevice_section_container.js"></script>
-</dom-module>

chrome/browser/resources/settings/multidevice_page/multidevice_section_container.js

-// Copyright 2018 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
- * Container element that interfaces with a Mojo API to ensure that the entire
- * multidevice settings-section is not attached to the DOM if there is not a
- * host device or potential host device. It also provides the page with the
- * data it needs to provide the user with the correct infomation and call(s) to
- * action based on the data retrieved from the Mojo service (i.e. the mode_
- * property).
- */
-
-Polymer({
-  is: 'settings-multidevice-section-container',
-
-  properties: {
-    /** SettingsPrefsElement 'prefs' Object reference. See prefs.js. */
-    prefs: {
-      type: Object,
-      notify: true,
-    },
-
-    // TODO(jordynass): Set this property based on the results of the Mojo call.
-    /** @type {settings.MultiDeviceSettingsMode} */
-    mode_: {
-      type: Number,
-      value: settings.MultiDeviceSettingsMode.NO_HOST_SET,
-    },
-  },
-
-  /** @return {boolean} */
-  hostFound_: function() {
-    return this.mode_ != settings.MultiDeviceSettingsMode.NO_ELIGIBLE_HOSTS;
-  },
-});

chrome/browser/resources/settings/settings_resources.grd

@@ -1305,11 +1305,11 @@
         <structure name="IDR_SETTINGS_MULTIDEVICE_PAGE_JS"
                    file="multidevice_page/multidevice_page.js"
                    type="chrome_html" />
-        <structure name="IDR_SETTINGS_MULTIDEVICE_SECTION_CONTAINER_HTML"
-                   file="multidevice_page/multidevice_section_container.html"
+        <structure name="IDR_SETTINGS_MULTIDEVICE_PAGE_CONTAINER_HTML"
+                   file="multidevice_page/multidevice_page_container.html"
                    type="chrome_html" />
-        <structure name="IDR_SETTINGS_MULTIDEVICE_SECTION_CONTAINER_JS"
-                   file="multidevice_page/multidevice_section_container.js"
+        <structure name="IDR_SETTINGS_MULTIDEVICE_PAGE_CONTAINER_JS"
+                   file="multidevice_page/multidevice_page_container.js"
                    type="chrome_html" />
         <structure name="IDR_SETTINGS_NETWORK_PROXY_SECTION_HTML"
                    file="internet_page/network_proxy_section.html"

chrome/test/data/webui/settings/cr_settings_browsertest.js

@@ -1796,26 +1796,26 @@ TEST_F('CrSettingsSmbPageTest', 'All', function() {
 });
 
 /**
- * Test fixture for the multidevice settings section container.
+ * Test fixture for the multidevice settings page container.
  * @constructor
  * @extends {CrSettingsBrowserTest}
  */
-function CrSettingsMultideviceSectionContainerTest() {}
+function CrSettingsMultidevicePageContainerTest() {}
 
-CrSettingsMultideviceSectionContainerTest.prototype = {
+CrSettingsMultidevicePageContainerTest.prototype = {
   __proto__: CrSettingsBrowserTest.prototype,
 
   /** @override */
   browsePreload:
-      'chrome://settings/multidevice_page/multidevice_section_container.html',
+      'chrome://settings/multidevice_page/multidevice_page_container.html',
 
   /** @override */
   extraLibraries: CrSettingsBrowserTest.prototype.extraLibraries.concat([
-    'multidevice_section_container_tests.js',
+    'multidevice_page_container_tests.js',
   ]),
 };
 
-TEST_F('CrSettingsMultideviceSectionContainerTest', 'All', function() {
+TEST_F('CrSettingsMultidevicePageContainerTest', 'All', function() {
   mocha.run();
 });
 

chrome/test/data/webui/settings/multidevice_section_container_tests.js → chrome/test/data/webui/settings/multidevice_page_container_tests.js

@@ -3,7 +3,7 @@
 // found in the LICENSE file.
 
 suite('Multidevice', function() {
-  let multideviceSectionContainer = null;
+  let multidevicePageContainer = null;
 
   let ALL_MODES;
 
@@ -11,62 +11,54 @@ suite('Multidevice', function() {
 
   setup(function() {
     PolymerTest.clearBody();
-    multideviceSectionContainer =
-        document.createElement('settings-multidevice-section-container');
-    assertTrue(!!multideviceSectionContainer);
+    multidevicePageContainer =
+        document.createElement('settings-multidevice-page-container');
+    assertTrue(!!multidevicePageContainer);
 
     ALL_MODES = Object.values(settings.MultiDeviceSettingsMode);
 
-    document.body.appendChild(multideviceSectionContainer);
+    document.body.appendChild(multidevicePageContainer);
     Polymer.dom.flush();
   });
 
   teardown(function() {
-    multideviceSectionContainer.remove();
+    multidevicePageContainer.remove();
   });
 
   const setMode = function(mode) {
-    multideviceSectionContainer.mode_ = mode;
+    multidevicePageContainer.mode_ = mode;
     Polymer.dom.flush();
   };
 
-  const isSettingsSectionVisible = function() {
-    const settingsSection = multideviceSectionContainer.$$(
-        'settings-section[section="multidevice"]');
-    return !!settingsSection && !settingsSection.hidden;
-  };
+  const getMultidevicePage = () =>
+      multidevicePageContainer.$$('settings-multidevice-page');
 
-  test('mode_ property toggles multidevice section', function() {
-    // Check that the settings-section is visible iff the mode is not
+  test('mode_ property toggles multidevice page', function() {
+    // Check that the settings-page is visible iff the mode is not
     // NO_ELIGIBLE_HOSTS.
     for (let mode of ALL_MODES) {
       setMode(mode);
       assertEquals(
-          isSettingsSectionVisible(),
-          mode != settings.MultiDeviceSettingsMode.NO_ELIGIBLE_HOSTS);
+          !getMultidevicePage(),
+          mode == settings.MultiDeviceSettingsMode.NO_ELIGIBLE_HOSTS);
     }
     // One more loop to ensure we transition in and out of NO_ELIGIBLE_HOSTS
     // mode.
     for (let mode of ALL_MODES) {
       setMode(mode);
       assertEquals(
-          isSettingsSectionVisible(),
-          mode != settings.MultiDeviceSettingsMode.NO_ELIGIBLE_HOSTS);
+          !getMultidevicePage(),
+          mode == settings.MultiDeviceSettingsMode.NO_ELIGIBLE_HOSTS);
     }
   });
 
-  test(
-      'mode_ property passes to settings-multidevice-page if present',
-      function() {
-        for (let mode of ALL_MODES) {
-          setMode(mode);
-          const multidevicePage =
-              multideviceSectionContainer.$$('settings-multidevice-page');
-          if (mode == settings.MultiDeviceSettingsMode.NO_ELIGIBLE_HOSTS)
-            assertEquals(multidevicePage, null);
-          else
-            assertEquals(
-                multideviceSectionContainer.mode_, multidevicePage.mode);
-        }
-      });
+  test('mode_ property passes to multidevice page if present', function() {
+    for (let mode of ALL_MODES) {
+      setMode(mode);
+      if (mode == settings.MultiDeviceSettingsMode.NO_ELIGIBLE_HOSTS)
+        assertEquals(getMultidevicePage(), null);
+      else
+        assertEquals(multidevicePageContainer.mode_, getMultidevicePage().mode);
+    }
+  });
 });