[CrOS PhoneHub] Fix issues on chrome://multidevice-internals

(1) Fix issue which set real PhoneHubManager when the page loads, even
    if it was already set. This prevents unnecessary re-initialization.
(2) Fix issue which displayed the Phone Hub UI even when the flag was
    disabled; now, we show a button leading users to the flag.
(3) Clean up some function names and alphabetize files.

Bug: 1106937
Change-Id: Ie11e136f6087d14176081fb6a45a2c587bc6da18
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2402107Reviewed-by: Regan Hsu <hsuregan@chromium.org>
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#805522}

chrome/browser/resources/chromeos/multidevice_internals/BUILD.gn

@@ -32,26 +32,33 @@ js_library("browser_tabs_metadata_form") {
   deps = [ ":types" ]
 }
 
-js_library("phone_status_model_form") {
+js_library("i18n_setup") {
+  deps = [ "//ui/webui/resources/js:load_time_data.m" ]
+}
+
+js_library("multidevice_phonehub_browser_proxy") {
   deps = [
-    ":multidevice_phonehub_browser_proxy",
     ":types",
+    "//ui/webui/resources/js:cr.m",
   ]
 }
 
-js_library("phonehub_tab") {
+js_library("logging_tab") {
   deps = [
-    ":browser_tabs_model_form",
-    ":multidevice_phonehub_browser_proxy",
-    ":phone_status_model_form",
+    ":log_object",
+    ":multidevice_logs_browser_proxy",
     ":types",
+    "//third_party/polymer/v3_0/components-chromium/polymer:polymer_bundled",
+    "//ui/webui/resources/js:cr.m",
+    "//ui/webui/resources/js:web_ui_listener_behavior.m",
   ]
 }
 
-js_library("multidevice_phonehub_browser_proxy") {
+js_library("log_object") {
   deps = [
+    ":multidevice_logs_browser_proxy",
     ":types",
-    "//ui/webui/resources/js:cr.m",
+    "//third_party/polymer/v3_0/components-chromium/polymer:polymer_bundled",
   ]
 }
 
@@ -65,29 +72,28 @@ js_library("multidevice_internals") {
   ]
 }
 
-js_library("logging_tab") {
+js_library("multidevice_logs_browser_proxy") {
   deps = [
-    ":log_object",
-    ":multidevice_logs_browser_proxy",
     ":types",
-    "//third_party/polymer/v3_0/components-chromium/polymer:polymer_bundled",
     "//ui/webui/resources/js:cr.m",
-    "//ui/webui/resources/js:web_ui_listener_behavior.m",
   ]
 }
 
-js_library("log_object") {
+js_library("phone_status_model_form") {
   deps = [
-    ":multidevice_logs_browser_proxy",
+    ":multidevice_phonehub_browser_proxy",
     ":types",
-    "//third_party/polymer/v3_0/components-chromium/polymer:polymer_bundled",
   ]
 }
 
-js_library("multidevice_logs_browser_proxy") {
+js_library("phonehub_tab") {
   deps = [
+    ":browser_tabs_model_form",
+    ":i18n_setup",
+    ":multidevice_phonehub_browser_proxy",
+    ":phone_status_model_form",
     ":types",
-    "//ui/webui/resources/js:cr.m",
+    "//ui/webui/resources/js:load_time_data.m",
   ]
 }
 

chrome/browser/resources/chromeos/multidevice_internals/i18n_setup.js

+// Copyright 2020 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.
+
+import './strings.m.js';
+
+export {loadTimeData} from 'chrome://resources/js/load_time_data.m.js';
\ No newline at end of file

chrome/browser/resources/chromeos/multidevice_internals/multidevice_internals_resources.grd

@@ -23,17 +23,26 @@
       <include name="IDR_MULTIDEVICE_INTERNALS_INDEX_HTML"
                file="index.html"
                type="BINDATA"/>
+      <include name="IDR_MULTIDEVICE_INTERNALS_I18N_SETUP_JS"
+               file="i18n_setup.js"
+               type="BINDATA"/>
       <include name="IDR_MULTIDEVICE_INTERNALS_LOG_OBJECT_JS"
                file="${root_gen_dir}\chrome\browser\resources\chromeos\multidevice_internals\log_object.js"
                use_base_dir="false"
                type="BINDATA"/>
+      <include name="IDR_MULTIDEVICE_INTERNALS_LOGGING_TAB_JS"
+               file="${root_gen_dir}\chrome\browser\resources\chromeos\multidevice_internals\logging_tab.js"
+               use_base_dir="false"
+               type="BINDATA"/>
       <include name="IDR_MULTIDEVICE_INTERNALS_MULTIDEVICE_INTERNALS_JS"
                file="${root_gen_dir}\chrome\browser\resources\chromeos\multidevice_internals\multidevice_internals.js"
                use_base_dir="false"
                type="BINDATA"/>
-      <include name="IDR_MULTIDEVICE_INTERNALS_LOGGING_TAB_JS"
-               file="${root_gen_dir}\chrome\browser\resources\chromeos\multidevice_internals\logging_tab.js"
-               use_base_dir="false"
+      <include name="IDR_MULTIDEVICE_INTERNALS_MULTIDEVICE_LOGS_BROWSER_PROXY_JS"
+               file="multidevice_logs_browser_proxy.js"
+               type="BINDATA"/>
+      <include name="IDR_MULTIDEVICE_INTERNALS_MULTIDEVICE_PHONEHUB_BROWSER_PROXY_JS"
+               file="multidevice_phonehub_browser_proxy.js"
                type="BINDATA"/>
       <include name="IDR_MULTIDEVICE_INTERNALS_PHONEHUB_TAB_JS"
                file="${root_gen_dir}\chrome\browser\resources\chromeos\multidevice_internals\phonehub_tab.js"
@@ -47,12 +56,6 @@
                file="${root_gen_dir}\chrome\browser\resources\chromeos\multidevice_internals\shared_style.js"
                use_base_dir="false"
                type="BINDATA"/>
-      <include name="IDR_MULTIDEVICE_INTERNALS_MULTIDEVICE_LOGS_BROWSER_PROXY_JS"
-               file="multidevice_logs_browser_proxy.js"
-               type="BINDATA"/>
-      <include name="IDR_MULTIDEVICE_INTERNALS_MULTIDEVICE_PHONEHUB_BROWSER_PROXY_JS"
-               file="multidevice_phonehub_browser_proxy.js"
-               type="BINDATA"/>
       <include name="IDR_MULTIDEVICE_INTERNALS_TYPES_JS"
                file="types.js"
                type="BINDATA"/>

chrome/browser/resources/chromeos/multidevice_internals/phonehub_tab.html

@@ -8,37 +8,53 @@
     display: flex;
     flex: 0 0 100%;
   }
+
+  #flagDisabledContainer {
+    align-items: center;
+    display: flex;
+    flex-direction: column;
+  }
 </style>
 
-<div class="cr-row">
-  <div class="cr-padded-text">
-    Toggle on to use fake PhoneHub
-  </div>
-  <cr-toggle checked="{{shouldEnableFakePhoneHubManager_}}">
-  </cr-toggle>
-</div>
-<template is="dom-if" if="[[shouldEnableFakePhoneHubManager_]]" restamp>
+<template is="dom-if" if="[[isPhoneHubEnabled_]]" restamp>
   <div class="cr-row">
     <div class="cr-padded-text">
-      Select feature status
-    </div>
-    <select id="featureStatusList" class="md-select"
-        on-change="onFeatureStatusSelected_">
-      <template is="dom-repeat" items="[[featureStatusList_]]">
-        <option>[[getFeatureStatusName_(item)]]</option>
-      </template>
-    </select>
-    <div class="cr-padded-text" hidden="[[isFeatureEnabledAndConnected_]]">
-      More controls will appear if the feature status is set to
-      <span class="emphasize">ENABLED_AND_CONNECTED</span>.
+      Toggle on to use fake PhoneHub
     </div>
+    <cr-toggle checked="{{shouldEnableFakePhoneHubManager_}}">
+    </cr-toggle>
   </div>
-  <template is="dom-if" if="[[isFeatureEnabledAndConnected_]]" restamp>
+  <template is="dom-if" if="[[shouldEnableFakePhoneHubManager_]]" restamp>
     <div class="cr-row">
-      <phone-status-model-form></phone-status-model-form>
-    </div>
-    <div class="cr-row">
-      <browser-tabs-model-form></browser-tabs-model-form>
+      <div class="cr-padded-text">
+        Select feature status
+      </div>
+      <select id="featureStatusList" class="md-select"
+          on-change="onFeatureStatusSelected_">
+        <template is="dom-repeat" items="[[featureStatusList_]]">
+          <option>[[getFeatureStatusName_(item)]]</option>
+        </template>
+      </select>
+      <div class="cr-padded-text" hidden="[[isFeatureEnabledAndConnected_]]">
+        More controls will appear if the feature status is set to
+        <span class="emphasize">ENABLED_AND_CONNECTED</span>.
+      </div>
     </div>
+    <template is="dom-if" if="[[isFeatureEnabledAndConnected_]]" restamp>
+      <div class="cr-row">
+        <phone-status-model-form></phone-status-model-form>
+      </div>
+      <div class="cr-row">
+        <browser-tabs-model-form></browser-tabs-model-form>
+      </div>
+    </template>
   </template>
 </template>
+<template is="dom-if" if="[[!isPhoneHubEnabled_]]" restamp>
+  <div id="flagDisabledContainer">
+    <p>Phone Hub flag is disabled.</p>
+    <cr-button on-click="onPhoneHubFlagButtonClick_">
+      Enable
+    </cr-button>
+  </div>
+</template>

chrome/browser/resources/chromeos/multidevice_internals/phonehub_tab.js

@@ -2,13 +2,16 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+import 'chrome://resources/cr_elements/cr_button/cr_button.m.js';
 import 'chrome://resources/cr_elements/cr_toggle/cr_toggle.m.js';
 import 'chrome://resources/cr_elements/md_select_css.m.js';
 import 'chrome://resources/cr_elements/shared_style_css.m.js';
 import './browser_tabs_model_form.js';
+import './i18n_setup.js';
 import './phone_status_model_form.js';
 import './shared_style.js';
 
+import {loadTimeData} from 'chrome://resources/js/load_time_data.m.js';
 import {flush, html, Polymer} from 'chrome://resources/polymer/v3_0/polymer/polymer_bundled.min.js';
 import {MultidevicePhoneHubBrowserProxy} from './multidevice_phonehub_browser_proxy.js';
 import {FeatureStatus} from './types.js';
@@ -40,6 +43,13 @@ Polymer({
   _template: html`{__html_template__}`,
 
   properties: {
+    /** @private */
+    isPhoneHubEnabled_: {
+      type: Boolean,
+      value: loadTimeData.getBoolean('isPhoneHubEnabled'),
+      readonly: true,
+    },
+
     /** @private */
     shouldEnableFakePhoneHubManager_: {
       type: Boolean,
@@ -127,4 +137,8 @@ Polymer({
     return featureStatusToStringMap.get(featureStatus);
   },
 
+  /** @private */
+  onPhoneHubFlagButtonClick_() {
+    window.open('chrome://flags/#enable-phone-hub');
+  },
 });

chrome/browser/ui/webui/chromeos/multidevice_internals/multidevice_internals_phone_hub_handler.cc

@@ -92,14 +92,14 @@ MultidevicePhoneHubHandler::MultidevicePhoneHubHandler() = default;
 
 MultidevicePhoneHubHandler::~MultidevicePhoneHubHandler() {
   if (fake_phone_hub_manager_)
-    SetSystemPhoneHubManagerEnabled();
+    EnableRealPhoneHubManager();
 }
 
 void MultidevicePhoneHubHandler::RegisterMessages() {
   web_ui()->RegisterMessageCallback(
       "setFakePhoneHubManagerEnabled",
       base::BindRepeating(
-          &MultidevicePhoneHubHandler::HandleSetFakePhoneHubManagerEnabled,
+          &MultidevicePhoneHubHandler::HandleEnableFakePhoneHubManager,
           base::Unretained(this)));
 
   web_ui()->RegisterMessageCallback(
@@ -118,7 +118,12 @@ void MultidevicePhoneHubHandler::RegisterMessages() {
                           base::Unretained(this)));
 }
 
-void MultidevicePhoneHubHandler::SetSystemPhoneHubManagerEnabled() {
+void MultidevicePhoneHubHandler::EnableRealPhoneHubManager() {
+  // If no FakePhoneHubManager is active, return early. This ensures that we
+  // don't unnecessarily re-initialize the Phone Hub UI.
+  if (!fake_phone_hub_manager_)
+    return;
+
   PA_LOG(VERBOSE) << "Setting real Phone Hub Manager";
   fake_phone_hub_manager_.reset();
   Profile* profile = Profile::FromWebUI(web_ui());
@@ -127,22 +132,23 @@ void MultidevicePhoneHubHandler::SetSystemPhoneHubManagerEnabled() {
   ash::SystemTray::Get()->SetPhoneHubManager(phone_hub_manager);
 }
 
-void MultidevicePhoneHubHandler::SetFakePhoneHubManagerEnabled() {
+void MultidevicePhoneHubHandler::EnableFakePhoneHubManager() {
+  DCHECK(!fake_phone_hub_manager_);
   PA_LOG(VERBOSE) << "Setting fake Phone Hub Manager";
   fake_phone_hub_manager_ = std::make_unique<phonehub::FakePhoneHubManager>();
   ash::SystemTray::Get()->SetPhoneHubManager(fake_phone_hub_manager_.get());
 }
 
-void MultidevicePhoneHubHandler::HandleSetFakePhoneHubManagerEnabled(
+void MultidevicePhoneHubHandler::HandleEnableFakePhoneHubManager(
     const base::ListValue* args) {
   AllowJavascript();
   bool enabled = false;
   CHECK(args->GetBoolean(0, &enabled));
   if (enabled) {
-    SetFakePhoneHubManagerEnabled();
+    EnableFakePhoneHubManager();
     return;
   }
-  SetSystemPhoneHubManagerEnabled();
+  EnableRealPhoneHubManager();
 }
 
 void MultidevicePhoneHubHandler::HandleSetFeatureStatus(

chrome/browser/ui/webui/chromeos/multidevice_internals/multidevice_internals_phone_hub_handler.h

@@ -30,9 +30,9 @@ class MultidevicePhoneHubHandler : public content::WebUIMessageHandler {
   void OnJavascriptDisallowed() override {}
 
  private:
-  void SetSystemPhoneHubManagerEnabled();
-  void SetFakePhoneHubManagerEnabled();
-  void HandleSetFakePhoneHubManagerEnabled(const base::ListValue* args);
+  void EnableRealPhoneHubManager();
+  void EnableFakePhoneHubManager();
+  void HandleEnableFakePhoneHubManager(const base::ListValue* args);
   void HandleSetFeatureStatus(const base::ListValue* args);
   void HandleSetFakePhoneStatus(const base::ListValue* args);
   void HandleSetBrowserTabs(const base::ListValue* args);

chrome/browser/ui/webui/chromeos/multidevice_internals/multidevice_internals_ui.cc

@@ -12,6 +12,7 @@
 #include "chrome/common/webui_url_constants.h"
 #include "chrome/grit/multidevice_internals_resources.h"
 #include "chrome/grit/multidevice_internals_resources_map.h"
+#include "chromeos/constants/chromeos_features.h"
 #include "content/public/browser/web_ui.h"
 #include "content/public/browser/web_ui_data_source.h"
 #include "ui/base/webui/web_ui_util.h"
@@ -27,9 +28,9 @@ constexpr char kMultideviceInternalsGeneratedPath[] =
 
 MultideviceInternalsUI::MultideviceInternalsUI(content::WebUI* web_ui)
     : ui::MojoWebUIController(web_ui, /*enable_chrome_send=*/true) {
-  Profile* profile = Profile::FromWebUI(web_ui);
   content::WebUIDataSource* html_source = content::WebUIDataSource::Create(
       chrome::kChromeUIMultiDeviceInternalsHost);
+  html_source->AddBoolean("isPhoneHubEnabled", features::IsPhoneHubEnabled());
 
   webui::SetupWebUIDataSource(
       html_source,
@@ -37,7 +38,7 @@ MultideviceInternalsUI::MultideviceInternalsUI(content::WebUI* web_ui)
                       kMultideviceInternalsResourcesSize),
       kMultideviceInternalsGeneratedPath, IDR_MULTIDEVICE_INTERNALS_INDEX_HTML);
 
-  content::WebUIDataSource::Add(profile, html_source);
+  content::WebUIDataSource::Add(Profile::FromWebUI(web_ui), html_source);
   web_ui->AddMessageHandler(
       std::make_unique<multidevice::MultideviceLogsHandler>());
   web_ui->AddMessageHandler(