Remove teleport menu item from Chrome OS settings window frame menu

This fixes a regression introduced by the split settings project.
The window frame menu for settings, when multiple users are logged in,
is not supposed to show the "Teleport" menu item. This broke because
the WebUI URL for the Settings window content changed.

Fix by using SettingsWindowManager to detect if a Browser* is the
settings window, which is the canonical way of doing it.

Bug: 1023043
Test: added to browser_tests
Change-Id: Ib79bcfd6c6206773d12377640f476d3b76652182
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1910760Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: James Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#714374}

chrome/browser/ui/views/frame/system_menu_model_builder.cc

@@ -9,11 +9,9 @@
 #include "build/build_config.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/ui/browser_commands.h"
-#include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/toolbar/app_menu_model.h"
 #include "chrome/browser/ui/web_applications/app_browser_controller.h"
 #include "chrome/common/chrome_switches.h"
-#include "chrome/common/url_constants.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/strings/grit/components_strings.h"
 #include "ui/base/accelerators/accelerator.h"
@@ -25,29 +23,13 @@
 #include "chrome/browser/ui/ash/multi_user/multi_user_util.h"
 #include "chrome/browser/ui/ash/multi_user/multi_user_window_manager_helper.h"
 #include "chrome/browser/ui/browser_window.h"
+#include "chrome/browser/ui/settings_window_manager_chromeos.h"
 #include "components/account_id/account_id.h"
 #include "components/user_manager/user_info.h"
 #include "components/user_manager/user_manager.h"
 #include "ui/base/l10n/l10n_util.h"
 #endif
 
-namespace {
-
-// Given a |browser| that's an app or popup window, checks if it's hosting the
-// settings page.
-bool IsChromeSettingsAppOrPopupWindow(Browser* browser) {
-  DCHECK(browser);
-  content::WebContents* web_contents =
-      browser->tab_strip_model()->GetActiveWebContents();
-  if (!web_contents)
-    return false;
-  const GURL& gurl = web_contents->GetURL();
-  return gurl.SchemeIs(content::kChromeUIScheme) &&
-         gurl.host_piece() == chrome::kChromeUISettingsHost;
-}
-
-}  // namespace
-
 SystemMenuModelBuilder::SystemMenuModelBuilder(
     ui::AcceleratorProvider* provider,
     Browser* browser)
@@ -135,14 +117,7 @@ void SystemMenuModelBuilder::BuildSystemMenuForAppOrPopupWindow(
   model->AddSeparator(ui::NORMAL_SEPARATOR);
   model->AddItemWithStringId(IDC_CLOSE_WINDOW, IDS_CLOSE);
 #endif
-
-  // Avoid appending the teleport menu for the settings window.  This window's
-  // presentation is unique: it's a normal browser window with an app-like
-  // frame, which doesn't have a user icon badge.  Thus if teleported it's not
-  // clear what user it applies to. Rather than bother to implement badging just
-  // for this rare case, simply prevent the user from teleporting the window.
-  if (!IsChromeSettingsAppOrPopupWindow(browser()))
-    AppendTeleportMenu(model);
+  AppendTeleportMenu(model);
 }
 
 void SystemMenuModelBuilder::AddFrameToggleItems(ui::SimpleMenuModel* model) {
@@ -158,6 +133,16 @@ void SystemMenuModelBuilder::AppendTeleportMenu(ui::SimpleMenuModel* model) {
 #if defined(OS_CHROMEOS)
   DCHECK(browser()->window());
 
+  // Avoid appending the teleport menu for the settings window.  This window's
+  // presentation is unique: it's a normal browser window with an app-like
+  // frame, which doesn't have a user icon badge.  Thus if teleported it's not
+  // clear what user it applies to. Rather than bother to implement badging just
+  // for this rare case, simply prevent the user from teleporting the window.
+  if (chrome::SettingsWindowManager::GetInstance()->IsSettingsBrowser(
+          browser())) {
+    return;
+  }
+
   // Don't show the menu for incognito windows.
   if (browser()->profile()->IsOffTheRecord())
     return;

chrome/browser/ui/views/frame/system_menu_model_builder_browsertest_chromeos.cc

+// Copyright 2019 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.
+
+#include "chrome/app/chrome_command_ids.h"
+#include "chrome/browser/chromeos/login/login_manager_test.h"
+#include "chrome/browser/chromeos/login/startup_utils.h"
+#include "chrome/browser/chromeos/login/ui/user_adding_screen.h"
+#include "chrome/browser/chromeos/profiles/profile_helper.h"
+#include "chrome/browser/ui/settings_window_manager_chromeos.h"
+#include "chrome/browser/ui/settings_window_manager_observer_chromeos.h"
+#include "chrome/browser/ui/views/frame/browser_view.h"
+#include "chrome/browser/ui/web_applications/system_web_app_ui_utils.h"
+#include "chrome/browser/web_applications/system_web_app_manager.h"
+#include "chrome/browser/web_applications/web_app_provider.h"
+#include "components/account_id/account_id.h"
+#include "components/user_manager/user_manager.h"
+#include "ui/base/models/menu_model.h"
+
+using chrome::SettingsWindowManager;
+using chrome::SettingsWindowManagerObserver;
+using chromeos::ProfileHelper;
+using user_manager::UserManager;
+
+namespace {
+
+class SystemMenuModelBuilderMultiUserTest
+    : public chromeos::LoginManagerTest,
+      public SettingsWindowManagerObserver {
+ public:
+  SystemMenuModelBuilderMultiUserTest()
+      : LoginManagerTest(/*should_launch_browser=*/false,
+                         /*should_initialize_webui=*/false),
+        account_id1_(
+            AccountId::FromUserEmailGaiaId("user1@gmail.com", "1111111111")),
+        account_id2_(
+            AccountId::FromUserEmailGaiaId("user2@gmail.com", "2222222222")) {}
+  ~SystemMenuModelBuilderMultiUserTest() override = default;
+
+  // SettingsWindowManagerObserver:
+  void OnNewSettingsWindow(Browser* settings_browser) override {
+    settings_browser_ = settings_browser;
+  }
+
+ protected:
+  const AccountId account_id1_;
+  const AccountId account_id2_;
+  Browser* settings_browser_ = nullptr;
+};
+
+IN_PROC_BROWSER_TEST_F(SystemMenuModelBuilderMultiUserTest,
+                       PRE_MultiUserSettingsWindowFrameMenu) {
+  RegisterUser(account_id1_);
+  RegisterUser(account_id2_);
+  chromeos::StartupUtils::MarkOobeCompleted();
+}
+
+// Regression test for https://crbug.com/1023043
+IN_PROC_BROWSER_TEST_F(SystemMenuModelBuilderMultiUserTest,
+                       MultiUserSettingsWindowFrameMenu) {
+  // Log in 2 users.
+  LoginUser(account_id1_);
+  base::RunLoop().RunUntilIdle();
+  chromeos::UserAddingScreen::Get()->Start();
+  AddUser(account_id2_);
+  base::RunLoop().RunUntilIdle();
+
+  // Install the Settings App.
+  Profile* profile = ProfileHelper::Get()->GetProfileByUser(
+      UserManager::Get()->FindUser(account_id1_));
+  web_app::WebAppProvider::Get(profile)
+      ->system_web_app_manager()
+      .InstallSystemAppsForTesting();
+
+  // Open the settings window and record the |settings_browser_|.
+  auto* manager = SettingsWindowManager::GetInstance();
+  manager->AddObserver(this);
+  manager->ShowOSSettings(profile);
+  manager->RemoveObserver(this);
+  ASSERT_TRUE(settings_browser_);
+
+  // Copy the command ids from the system menu.
+  BrowserView* browser_view =
+      BrowserView::GetBrowserViewForBrowser(settings_browser_);
+  ui::MenuModel* menu = browser_view->frame()->GetSystemMenuModel();
+  std::set<int> commands;
+  for (int i = 0; i < menu->GetItemCount(); i++)
+    commands.insert(menu->GetCommandIdAt(i));
+
+  // Standard WebUI commands are available.
+  EXPECT_TRUE(base::Contains(commands, IDC_BACK));
+  EXPECT_TRUE(base::Contains(commands, IDC_FORWARD));
+  EXPECT_TRUE(base::Contains(commands, IDC_RELOAD));
+
+  // Settings window cannot be teleported.
+  EXPECT_FALSE(base::Contains(commands, IDC_VISIT_DESKTOP_OF_LRU_USER_2));
+  EXPECT_FALSE(base::Contains(commands, IDC_VISIT_DESKTOP_OF_LRU_USER_3));
+}
+
+}  // namespace

chrome/test/BUILD.gn

@@ -2352,6 +2352,7 @@ if (!is_android) {
         "../browser/ui/views/frame/browser_frame_ash_browsertest.cc",
         "../browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc",
         "../browser/ui/views/frame/immersive_mode_controller_ash_browsertest.cc",
+        "../browser/ui/views/frame/system_menu_model_builder_browsertest_chromeos.cc",
         "../browser/ui/views/frame/top_controls_slide_controller_chromeos_browsertest.cc",
         "../browser/ui/views/plugin_vm/plugin_vm_launcher_view_browsertest.cc",
         "../browser/ui/views/web_apps/web_app_ash_interactive_ui_test.cc",