[CrOS MultiDevice] Remove page container infrastructure in Settings UI

Settings has strongly urged us to move away from dynamically attaching
and detaching the MultiDevice page from Settings UI. After this push,
I looked into the costs of that and with khorimoto@ decided they were
small enough to warrant the refactor given the rarity of this
functionality in practice. Note that MultiDevice settings flag already
controls whether to allow the page at a higher level so that does not
rely on the dynamic attachment ability provided by the
multidevice-page-container.

An additional incentive is to prevent a few display bugs (see the bugs
merged into 887784 for details) that are very likely to be irrelevant
once this infrastructure is removed.

Screenshots from the cases that cause the MultiDevice page to detach in
the HEAD version:
http://screen/BW2npVh2Ocw
http://screen/pKLechGvcCp

Bug: 887784
Change-Id: Iaefb8bff4ae300c483645e2558b8a95083e2bc47
Reviewed-on: https://chromium-review.googlesource.com/c/1258211
Commit-Queue: Jordy Greenblatt <jordynass@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#596107}

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_page_container.html">
+<link rel="import" href="../multidevice_page/multidevice_page.html">
 </if>
 
 <if expr="not chromeos">
@@ -122,12 +122,8 @@
             if="[[canShowMultideviceSection_(showMultidevice, pageVisibility)]]"
             restamp>
           <settings-section page-title="$i18n{multidevicePageTitle}"
-              section="multidevice"
-              hidden$="[[!doesChromebookSupportMultiDeviceSection_]]">
-            <settings-multidevice-page-container
-                does-chromebook-support-multi-device-features=
-                    "{{doesChromebookSupportMultiDeviceSection_}}">
-            </settings-multidevice-page-container>
+              section="multidevice">
+            <settings-multidevice-page></settings-multidevice-page>
           </settings-section>
         </template>
 </if>

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

@@ -89,16 +89,6 @@ Polymer({
       type: Boolean,
       computed: 'computeShowSecondaryUserBanner_(hasExpandedSection_)',
     },
-
-    /**
-     * Whether the account supports the features controlled in the multidevice
-     * section.
-     * @private {boolean}
-     */
-    doesChromebookSupportMultiDeviceSection_: {
-      type: Boolean,
-      value: false,
-    },
     // </if>
 
     /** @private {!settings.Route|undefined} */

chrome/browser/resources/settings/multidevice_page/BUILD.gn

@@ -12,7 +12,6 @@ js_type_check("closure_compile") {
     ":multidevice_feature_item",
     ":multidevice_feature_toggle",
     ":multidevice_page",
-    ":multidevice_page_container",
     ":multidevice_subpage",
     ":multidevice_tether_item",
   ]
@@ -62,15 +61,6 @@ js_library("multidevice_page") {
     ":multidevice_feature_behavior",
     "../controls:password_prompt_dialog",
     "//ui/webui/resources/js:cr",
-  ]
-}
-
-js_library("multidevice_page_container") {
-  deps = [
-    ":multidevice_browser_proxy",
-    ":multidevice_constants",
-    ":multidevice_feature_behavior",
-    "//ui/webui/resources/js:cr",
     "//ui/webui/resources/js:web_ui_listener_behavior",
   ]
 }

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

@@ -3,6 +3,7 @@
 <link rel="import" href="chrome://resources/cr_elements/cr_toggle/cr_toggle.html">
 <link rel="import" href="chrome://resources/html/cr.html">
 <link rel="import" href="chrome://resources/html/i18n_behavior.html">
+<link rel="import" href="chrome://resources/html/web_ui_listener_behavior.html">
 <link rel="import" href="chrome://resources/polymer/v1_0/iron-icon/iron-icon.html">
 <link rel="import" href="chrome://resources/polymer/v1_0/neon-animation/neon-animatable.html">
 <link rel="import" href="chrome://resources/polymer/v1_0/paper-button/paper-button.html">

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

@@ -11,7 +11,7 @@ cr.exportPath('settings');
 Polymer({
   is: 'settings-multidevice-page',
 
-  behaviors: [MultiDeviceFeatureBehavior],
+  behaviors: [MultiDeviceFeatureBehavior, WebUIListenerBehavior],
 
   properties: {
     /**
@@ -72,8 +72,15 @@ Polymer({
   browserProxy_: null,
 
   /** @override */
-  created: function() {
+  ready: function() {
     this.browserProxy_ = settings.MultiDeviceBrowserProxyImpl.getInstance();
+
+    this.addWebUIListener(
+        'settings.updateMultidevicePageContentData',
+        this.onPageContentDataChanged_.bind(this));
+
+    this.browserProxy_.getPageContentData().then(
+        this.onPageContentDataChanged_.bind(this));
   },
 
   /**
@@ -307,4 +314,12 @@ Polymer({
     this.browserProxy_.removeHostDevice();
     settings.navigateTo(settings.routes.MULTIDEVICE);
   },
+
+  /**
+   * @param {!MultiDevicePageContentData} newData
+   * @private
+   */
+  onPageContentDataChanged_: function(newData) {
+    this.pageContentData = newData;
+  },
 });

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

-<link rel="import" href="chrome://resources/html/polymer.html">
-
-<link rel="import" href="chrome://resources/html/cr.html">
-<link rel="import" href="chrome://resources/html/web_ui_listener_behavior.html">
-<link rel="import" href="multidevice_browser_proxy.html">
-<link rel="import" href="multidevice_constants.html">
-<link rel="import" href="multidevice_feature_behavior.html">
-<link rel="import" href="multidevice_page.html">
-
-<dom-module id="settings-multidevice-page-container">
-  <template>
-    <template is="dom-if"
-        if="[[doesChromebookSupportMultiDeviceFeatures]]"
-        restamp>
-      <settings-multidevice-page page-content-data="[[pageContentData]]">
-      </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).
- */
-
-cr.exportPath('settings');
-
-Polymer({
-  is: 'settings-multidevice-page-container',
-
-  behaviors: [MultiDeviceFeatureBehavior, WebUIListenerBehavior],
-
-  properties: {
-    /**
-     * Whether the Chromebook is capable of enabling Better Together features.
-     * @type {boolean}
-     */
-    doesChromebookSupportMultiDeviceFeatures: {
-      type: Boolean,
-      computed:
-          'computeDoesChromebookSupportMultiDeviceFeatures(pageContentData)',
-      notify: true,
-    },
-  },
-
-  /** @private {?settings.MultiDeviceBrowserProxy} */
-  browserProxy_: null,
-
-  /** @override */
-  ready: function() {
-    this.browserProxy_ = settings.MultiDeviceBrowserProxyImpl.getInstance();
-
-    this.addWebUIListener(
-        'settings.updateMultidevicePageContentData',
-        this.onPageContentDataChanged_.bind(this));
-
-    this.browserProxy_.getPageContentData().then(
-        this.onPageContentDataChanged_.bind(this));
-  },
-
-  /**
-   * @param {!MultiDevicePageContentData} newData
-   * @private
-   */
-  onPageContentDataChanged_: function(newData) {
-    if (!this.isStatusChangeValid_(newData)) {
-      console.error('Invalid status change');
-      return;
-    }
-    this.pageContentData = newData;
-  },
-
-  /**
-   * If the new mode corresponds to no eligible host or unset potential hosts
-   * (i.e. NO_ELIGIBLE_HOSTS or NO_HOST_SET), then newHostDeviceName should be
-   * falsy. Otherwise it should be truthy.
-   * @param {!MultiDevicePageContentData} newData
-   * @private
-   */
-  isStatusChangeValid_: function(newData) {
-    const noHostModes = [
-      settings.MultiDeviceSettingsMode.NO_ELIGIBLE_HOSTS,
-      settings.MultiDeviceSettingsMode.NO_HOST_SET,
-    ];
-    return !newData.hostDeviceName === noHostModes.includes(newData.mode);
-  },
-
-  /**
-   * @return {boolean}
-   * @private
-   */
-  computeDoesChromebookSupportMultiDeviceFeatures: function() {
-    return !!this.pageContentData &&
-        this.isFeatureSupported(
-            settings.MultiDeviceFeature.BETTER_TOGETHER_SUITE);
-  },
-});

chrome/browser/resources/settings/settings_resources.grd

@@ -1357,12 +1357,6 @@
         <structure name="IDR_SETTINGS_MULTIDEVICE_PAGE_JS"
                    file="multidevice_page/multidevice_page.js"
                    type="chrome_html" />
-        <structure name="IDR_SETTINGS_MULTIDEVICE_PAGE_CONTAINER_HTML"
-                   file="multidevice_page/multidevice_page_container.html"
-                   type="chrome_html" />
-        <structure name="IDR_SETTINGS_MULTIDEVICE_PAGE_CONTAINER_JS"
-                   file="multidevice_page/multidevice_page_container.js"
-                   type="chrome_html" />
         <structure name="IDR_SETTINGS_MULTIDEVICE_SUBPAGE_HTML"
                    file="multidevice_page/multidevice_subpage.html"
                    type="chrome_html" />

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

@@ -1935,31 +1935,6 @@ TEST_F('CrSettingsMultideviceFeatureToggleTest', 'All', function() {
   mocha.run();
 });
 
-/**
- * Test fixture for the multidevice settings page container.
- * @constructor
- * @extends {CrSettingsBrowserTest}
- */
-function CrSettingsMultidevicePageContainerTest() {}
-
-CrSettingsMultidevicePageContainerTest.prototype = {
-  __proto__: CrSettingsBrowserTest.prototype,
-
-  /** @override */
-  browsePreload:
-      'chrome://settings/multidevice_page/multidevice_page_container.html',
-
-  /** @override */
-  extraLibraries: CrSettingsBrowserTest.prototype.extraLibraries.concat([
-    '../test_browser_proxy.js',
-    'multidevice_page_container_tests.js',
-  ]),
-};
-
-TEST_F('CrSettingsMultidevicePageContainerTest', 'All', function() {
-  mocha.run();
-});
-
 /**
  * Test fixture for the multidevice settings page.
  * @constructor

chrome/test/data/webui/settings/multidevice_page_container_tests.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.
-
-/**
- * @implements {settings.MultideviceBrowserProxy}
- * Note: showMultiDeviceSetupDialog is not used by the
- * multidevice-page-container element.
- */
-class TestMultideviceBrowserProxy extends TestBrowserProxy {
-  constructor(initialPageContentData) {
-    super([
-      'showMultiDeviceSetupDialog',
-      'getPageContentData',
-    ]);
-    this.data = initialPageContentData;
-  }
-
-  /** @override */
-  getPageContentData() {
-    this.methodCalled('getPageContentData');
-    return Promise.resolve(this.data);
-  }
-}
-
-suite('Multidevice', function() {
-  let multidevicePageContainer = null;
-  let browserProxy = null;
-  let ALL_MODES;
-
-  /**
-   * @param {!settings.MultiDeviceSettingsMode} mode
-   * @param {boolean} isSuiteSupported
-   * @return {!MultiDevicePageContentData}
-   */
-  function getFakePageContentData(mode, isSuiteSupported) {
-    return {
-      mode: mode,
-      hostDeviceName: [
-        settings.MultiDeviceSettingsMode.HOST_SET_WAITING_FOR_SERVER,
-        settings.MultiDeviceSettingsMode.HOST_SET_WAITING_FOR_VERIFICATION,
-        settings.MultiDeviceSettingsMode.HOST_SET_VERIFIED,
-      ].includes(mode) ?
-          'Pixel XL' :
-          undefined,
-      betterTogetherState: isSuiteSupported ?
-          settings.MultiDeviceFeatureState.ENABLED_BY_USER :
-          settings.MultiDeviceFeatureState.NOT_SUPPORTED_BY_CHROMEBOOK,
-    };
-  }
-
-  /**
-   * @param {!settings.MultiDeviceSettingsMode} newMode
-   * @param {boolean} isSuiteSupported
-   */
-  function changePageContent(newMode, isSuiteSupported) {
-    cr.webUIListenerCallback(
-        'settings.updateMultidevicePageContentData',
-        getFakePageContentData(newMode, isSuiteSupported));
-    Polymer.dom.flush();
-  }
-
-  suiteSetup(function() {
-    ALL_MODES = Object.values(settings.MultiDeviceSettingsMode);
-  });
-
-  /** @return {?HTMLElement} */
-  const getMultidevicePage = () =>
-      multidevicePageContainer.$$('settings-multidevice-page');
-
-  // Initializes page container with provided mode and sets the Better Together
-  // suite's feature state based on provided boolean. It returns a promise that
-  // only resolves when the multidevice-page has changed its pageContentData and
-  // the ensuing callbacks back been flushed.
-  /**
-   * @param {!settings.MultiDeviceSettingsMode} mode
-   * @param {boolean} isSuiteSupported
-   * @return {!Promise}
-   */
-  function setInitialPageContent(mode, isSuiteSupported) {
-    if (multidevicePageContainer)
-      multidevicePageContainer.remove();
-    browserProxy = new TestMultideviceBrowserProxy(
-        getFakePageContentData(mode, isSuiteSupported));
-    settings.MultiDeviceBrowserProxyImpl.instance_ = browserProxy;
-
-    PolymerTest.clearBody();
-    multidevicePageContainer =
-        document.createElement('settings-multidevice-page-container');
-    document.body.appendChild(multidevicePageContainer);
-
-    return browserProxy.whenCalled('getPageContentData')
-        .then(Polymer.dom.flush());
-  }
-
-  test(
-      'WebUIListener toggles multidevice page based suite support in all modes',
-      function() {
-        return setInitialPageContent(
-                   settings.MultiDeviceSettingsMode.NO_HOST_SET,
-                   /* isSuiteSupported */ true)
-            .then(() => {
-              for (const mode of ALL_MODES) {
-                // Check that the settings-page is visible iff the suite is
-                // supported.
-                changePageContent(mode, /* isSuiteSupported */ true);
-                assertTrue(!!getMultidevicePage());
-
-                changePageContent(mode, /* isSuiteSupported */ false);
-                assertFalse(!!getMultidevicePage());
-
-                changePageContent(mode, /* isSuiteSupported */ true);
-                assertTrue(!!getMultidevicePage());
-              }
-            });
-      });
-
-  test(
-      'multidevice-page is not attached if suite is not supported', function() {
-        return setInitialPageContent(
-                   settings.MultiDeviceSettingsMode.NO_HOST_SET,
-                   /* isSuiteSupported */ false)
-            .then(() => assertFalse(!!getMultidevicePage()));
-      });
-});

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

@@ -2,69 +2,98 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-/**
- * @implements {settings.MultideviceBrowserProxy}
- * Note: Only showMultiDeviceSetupDialog is used by the multidevice-page
- * element.
- */
-class TestMultideviceBrowserProxy extends TestBrowserProxy {
-  constructor() {
-    super([
-      'showMultiDeviceSetupDialog',
-      'getPageContentData',
-      'setFeatureEnabledState',
-    ]);
+suite('Multidevice', function() {
+  /**
+   * Builds fake pageContentData for the specified mode. If it is a mode
+   * corresponding to a set host, it will set the hostDeviceName to the provided
+   * name or else default to HOST_DEVICE.
+   * @param {settings.MultiDeviceSettingsMode} mode
+   * @param {string=} opt_hostDeviceName Overrides default if |mode| corresponds
+   *     to a set host.
+   * @return {!MultiDevicePageContentData}
+   */
+  function createFakePageContentData(mode, opt_hostDeviceName) {
+    let pageContentData = {mode: mode};
+    if ([
+          settings.MultiDeviceSettingsMode.HOST_SET_WAITING_FOR_SERVER,
+          settings.MultiDeviceSettingsMode.HOST_SET_WAITING_FOR_VERIFICATION,
+          settings.MultiDeviceSettingsMode.HOST_SET_VERIFIED,
+        ].includes(mode)) {
+      pageContentData.hostDeviceName = opt_hostDeviceName || HOST_DEVICE;
+    }
+    return pageContentData;
   }
 
-  /** @override */
-  showMultiDeviceSetupDialog() {
-    this.methodCalled('showMultiDeviceSetupDialog');
-  }
+  /**
+   * @implements {settings.MultideviceBrowserProxy}
+   * Note: Only showMultiDeviceSetupDialog is used by the multidevice-page
+   * element.
+   */
+  class TestMultideviceBrowserProxy extends TestBrowserProxy {
+    constructor() {
+      super([
+        'showMultiDeviceSetupDialog',
+        'getPageContentData',
+        'setFeatureEnabledState',
+      ]);
+      this.data = createFakePageContentData(
+          settings.MultiDeviceSettingsMode.NO_HOST_SET);
+    }
+
+    /** @override */
+    getPageContentData() {
+      this.methodCalled('getPageContentData');
+      return Promise.resolve(this.data);
+    }
+
+    /** @override */
+    showMultiDeviceSetupDialog() {
+      this.methodCalled('showMultiDeviceSetupDialog');
+    }
 
-  /** @override */
-  setFeatureEnabledState(feature, enabled, opt_authToken) {
-    this.methodCalled(
-        'setFeatureEnabledState', [feature, enabled, opt_authToken]);
+    /** @override */
+    setFeatureEnabledState(feature, enabled, opt_authToken) {
+      this.methodCalled(
+          'setFeatureEnabledState', [feature, enabled, opt_authToken]);
+    }
   }
-}
 
-suite('Multidevice', function() {
   let multidevicePage = null;
   let browserProxy = null;
   let ALL_MODES;
   const HOST_DEVICE = 'Pixel XL';
 
+  /**
+   * Sets pageContentData via WebUI Listener and flushes.
+   * @param {!MultiDevicePageContentData}
+   */
+  function setPageContentData(newPageContentData) {
+    cr.webUIListenerCallback(
+        'settings.updateMultidevicePageContentData', newPageContentData);
+    Polymer.dom.flush();
+  }
+
   /**
    * Sets pageContentData to the specified mode. If it is a mode corresponding
    * to a set host, it will set the hostDeviceName to the provided name or else
    * default to HOST_DEVICE.
    * @param {settings.MultiDeviceSettingsMode} newMode
-   * @param {string|undefined} newHostDeviceName Overrides default if there
-   *     newMode corresponds to a set host.
+   * @param {string=} opt_newHostDeviceName Overrides default if |newMode|
+   *     corresponds to a set host.
    */
-  function setPageContentData(newMode, newHostDeviceName) {
-    let newPageContentData = {mode: newMode};
-    if ([
-          settings.MultiDeviceSettingsMode.HOST_SET_WAITING_FOR_SERVER,
-          settings.MultiDeviceSettingsMode.HOST_SET_WAITING_FOR_VERIFICATION,
-          settings.MultiDeviceSettingsMode.HOST_SET_VERIFIED,
-        ].includes(newMode)) {
-      newPageContentData.hostDeviceName = newHostDeviceName || HOST_DEVICE;
-    }
-    multidevicePage.pageContentData = newPageContentData;
-    Polymer.dom.flush();
+  function setHostData(newMode, opt_newHostDeviceName) {
+    setPageContentData(
+        createFakePageContentData(newMode, opt_newHostDeviceName));
   }
 
   function setSuiteState(newState) {
-    multidevicePage.pageContentData = Object.assign(
-        {}, multidevicePage.pageContentData, {betterTogetherState: newState});
-    Polymer.dom.flush();
+    setPageContentData(Object.assign(
+        {}, multidevicePage.pageContentData, {betterTogetherState: newState}));
   }
 
   function setSmartLockState(newState) {
-    multidevicePage.pageContentData = Object.assign(
-        {}, multidevicePage.pageContentData, {smartLockState: newState});
-    Polymer.dom.flush();
+    setPageContentData(Object.assign(
+        {}, multidevicePage.pageContentData, {smartLockState: newState}));
   }
 
   /**
@@ -129,7 +158,7 @@ suite('Multidevice', function() {
   const getSubpage = () => multidevicePage.$$('settings-multidevice-subpage');
 
   test('clicking setup shows multidevice setup dialog', function() {
-    setPageContentData(settings.MultiDeviceSettingsMode.NO_HOST_SET);
+    setHostData(settings.MultiDeviceSettingsMode.NO_HOST_SET);
     const button = multidevicePage.$$('paper-button');
     assertTrue(!!button);
     button.click();
@@ -138,23 +167,23 @@ suite('Multidevice', function() {
 
   test('headings render based on mode and host', function() {
     for (const mode of ALL_MODES) {
-      setPageContentData(mode);
+      setHostData(mode);
       assertEquals(multidevicePage.isHostSet(), getLabel() === HOST_DEVICE);
     }
   });
 
   test('changing host device changes header', function() {
-    setPageContentData(settings.MultiDeviceSettingsMode.HOST_SET_VERIFIED);
+    setHostData(settings.MultiDeviceSettingsMode.HOST_SET_VERIFIED);
     assertEquals(getLabel(), HOST_DEVICE);
     const anotherHost = 'Super Duper ' + HOST_DEVICE;
-    setPageContentData(
+    setHostData(
         settings.MultiDeviceSettingsMode.HOST_SET_VERIFIED, anotherHost);
     assertEquals(getLabel(), anotherHost);
   });
 
   test('item is actionable if and only if a host is set', function() {
     for (const mode of ALL_MODES) {
-      setPageContentData(mode);
+      setHostData(mode);
       assertEquals(
           multidevicePage.isHostSet(),
           !!multidevicePage.$$('#multidevice-item').hasAttribute('actionable'));
@@ -164,7 +193,7 @@ suite('Multidevice', function() {
   test(
       'clicking item with verified host opens subpage with features',
       function() {
-        setPageContentData(settings.MultiDeviceSettingsMode.HOST_SET_VERIFIED);
+        setHostData(settings.MultiDeviceSettingsMode.HOST_SET_VERIFIED);
         assertFalse(!!getSubpage());
         multidevicePage.$$('#multidevice-item').click();
         assertTrue(!!getSubpage());
@@ -174,7 +203,7 @@ suite('Multidevice', function() {
   test(
       'clicking item with unverified set host opens subpage without features',
       function() {
-        setPageContentData(
+        setHostData(
             settings.MultiDeviceSettingsMode.HOST_SET_WAITING_FOR_VERIFICATION,
             HOST_DEVICE);
         assertFalse(!!getSubpage());
@@ -184,7 +213,7 @@ suite('Multidevice', function() {
       });
 
   test('policy prohibited suite shows policy indicator', function() {
-    setPageContentData(settings.MultiDeviceSettingsMode.NO_ELIGIBLE_HOSTS);
+    setHostData(settings.MultiDeviceSettingsMode.NO_ELIGIBLE_HOSTS);
     assertFalse(!!multidevicePage.$$('cr-policy-indicator'));
     // Prohibit suite by policy.
     setSuiteState(settings.MultiDeviceFeatureState.PROHIBITED_BY_POLICY);