Commit 9946095a authored by Jiewei Qian's avatar Jiewei Qian Committed by Chromium LUCI CQ

ui: replace SettingsWindowObserver for OS Settings tests

SettingsWindowObserver is used to check if the number of OS Settings
browser instances are correct. This can be performed in other ways
without relying on SettingsWindowManager.

SettingsWindowManager predates the SWA platform, which implements many
features wanted by OS Settings, but were otherwise unavailable. After
SWA platform launch, we have implemented these features in SWA platform,
so it's no longer necessary for OS Settings to duplicate these logics.

This is a preparation for implementing a new LaunchSystemWebApp API,
which no longer returns the Browser object running the launched App.

Bug: 1154540
Change-Id: I2c3322d30fbf2c288e03aa3cc139f9f3bbf74920
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2569191
Commit-Queue: Jiewei Qian  <qjw@chromium.org>
Reviewed-by: default avatarJames Cook <jamescook@chromium.org>
Cr-Commit-Position: refs/heads/master@{#833071}
parent 44832e3d
......@@ -10,10 +10,11 @@
#include "chrome/browser/profiles/profile_manager.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_finder.h"
#include "chrome/browser/ui/browser_list.h"
#include "chrome/browser/ui/browser_window.h"
#include "chrome/browser/ui/settings_window_manager_chromeos.h"
#include "chrome/browser/ui/settings_window_manager_observer_chromeos.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "chrome/browser/ui/web_applications/system_web_app_ui_utils.h"
#include "chrome/browser/ui/web_applications/test/web_app_navigation_browsertest.h"
#include "chrome/browser/ui/webui/settings/chromeos/constants/routes.mojom.h"
#include "chrome/browser/web_applications/system_web_app_manager.h"
......@@ -54,13 +55,15 @@ void CreateAndStartUserSession(const AccountId& account_id) {
SessionManager::Get()->SessionStarted();
}
class SettingsTestObserver : public chrome::SettingsWindowManagerObserver {
public:
void OnNewSettingsWindow(Browser* settings_browser) override {
++new_settings_count_;
}
int new_settings_count_ = 0;
};
// Return the number of windows that hosts OS Settings.
size_t GetNumberOfSettingsWindows() {
auto* browser_list = BrowserList::GetInstance();
return std::count_if(browser_list->begin(), browser_list->end(),
[](Browser* browser) {
return web_app::IsBrowserForSystemWebApp(
browser, web_app::SystemAppType::SETTINGS);
});
}
// Give the underlying function a clearer name.
Browser* GetLastActiveBrowser() {
......@@ -188,19 +191,13 @@ void TestOpenSettingFromArc(Browser* browser,
->system_web_app_manager()
.InstallSystemAppsForTesting();
SettingsTestObserver observer;
auto* settings = chrome::SettingsWindowManager::GetInstance();
settings->AddObserver(&observer);
ChromeNewWindowClient::Get()->OpenChromePageFromArc(page);
EXPECT_EQ(expected_setting_window_count, observer.new_settings_count_);
EXPECT_EQ(expected_setting_window_count, GetNumberOfSettingsWindows());
// The right settings are loaded (not just the settings main page).
content::WebContents* contents =
GetLastActiveBrowser()->tab_strip_model()->GetActiveWebContents();
EXPECT_EQ(expected_url, contents->GetVisibleURL());
settings->RemoveObserver(&observer);
}
IN_PROC_BROWSER_TEST_F(ChromeNewWindowClientBrowserTest,
......@@ -220,19 +217,13 @@ IN_PROC_BROWSER_TEST_F(ChromeNewWindowClientBrowserTest,
}
IN_PROC_BROWSER_TEST_F(ChromeNewWindowClientBrowserTest, OpenAboutChromePage) {
SettingsTestObserver observer;
auto* settings = chrome::SettingsWindowManager::GetInstance();
settings->AddObserver(&observer);
// Opening an about: chrome page opens a new tab, and not the Settings window.
ChromeNewWindowClient::Get()->OpenChromePageFromArc(ChromePage::ABOUTHISTORY);
EXPECT_EQ(0, observer.new_settings_count_);
EXPECT_EQ(0, GetNumberOfSettingsWindows());
content::WebContents* contents =
GetLastActiveBrowser()->tab_strip_model()->GetActiveWebContents();
EXPECT_EQ(GURL(chrome::kChromeUIHistoryURL), contents->GetVisibleURL());
settings->RemoveObserver(&observer);
}
void TestOpenChromePage(ChromePage page, const GURL& expected_url) {
......
......@@ -15,7 +15,6 @@
#include "chrome/browser/ui/browser_window.h"
#include "chrome/browser/ui/chrome_pages.h"
#include "chrome/browser/ui/settings_window_manager_chromeos.h"
#include "chrome/browser/ui/settings_window_manager_observer_chromeos.h"
#include "chrome/browser/ui/web_applications/system_web_app_ui_utils.h"
#include "chrome/browser/ui/webui/settings/chromeos/constants/routes.mojom.h"
#include "chrome/browser/web_applications/system_web_app_manager.h"
......@@ -31,35 +30,22 @@
namespace {
class SettingsWindowTestObserver
: public chrome::SettingsWindowManagerObserver {
public:
SettingsWindowTestObserver() = default;
~SettingsWindowTestObserver() override = default;
void OnNewSettingsWindow(Browser* settings_browser) override {
browser_ = settings_browser;
++new_settings_count_;
}
Browser* browser() { return browser_; }
size_t new_settings_count() const { return new_settings_count_; }
private:
Browser* browser_ = nullptr;
size_t new_settings_count_ = 0;
DISALLOW_COPY_AND_ASSIGN(SettingsWindowTestObserver);
};
// Return the number of windows that hosts OS Settings.
size_t GetNumberOfSettingsWindows() {
auto* browser_list = BrowserList::GetInstance();
return std::count_if(browser_list->begin(), browser_list->end(),
[](Browser* browser) {
return web_app::IsBrowserForSystemWebApp(
browser, web_app::SystemAppType::SETTINGS);
});
}
} // namespace
class SettingsWindowManagerTest : public InProcessBrowserTest {
public:
SettingsWindowManagerTest()
: settings_manager_(chrome::SettingsWindowManager::GetInstance()) {
settings_manager_->AddObserver(&observer_);
}
: settings_manager_(chrome::SettingsWindowManager::GetInstance()) {}
void SetUpOnMainThread() override {
// Install the Settings App.
......@@ -68,9 +54,7 @@ class SettingsWindowManagerTest : public InProcessBrowserTest {
.InstallSystemAppsForTesting();
}
~SettingsWindowManagerTest() override {
settings_manager_->RemoveObserver(&observer_);
}
~SettingsWindowManagerTest() override = default;
void ShowSettingsForProfile(Profile* profile) {
settings_manager_->ShowChromePageForProfile(
......@@ -91,7 +75,6 @@ class SettingsWindowManagerTest : public InProcessBrowserTest {
protected:
chrome::SettingsWindowManager* settings_manager_;
SettingsWindowTestObserver observer_;
DISALLOW_COPY_AND_ASSIGN(SettingsWindowManagerTest);
};
......@@ -102,15 +85,13 @@ IN_PROC_BROWSER_TEST_F(SettingsWindowManagerTest, OpenSettingsWindow) {
Browser* settings_browser =
settings_manager_->FindBrowserForProfile(browser()->profile());
ASSERT_TRUE(settings_browser);
// Ensure the observer fired correctly.
EXPECT_EQ(1u, observer_.new_settings_count());
EXPECT_EQ(settings_browser, observer_.browser());
EXPECT_EQ(1u, GetNumberOfSettingsWindows());
// Open the settings again: no new window.
settings_manager_->ShowOSSettings(browser()->profile());
EXPECT_EQ(settings_browser,
settings_manager_->FindBrowserForProfile(browser()->profile()));
EXPECT_EQ(1u, observer_.new_settings_count());
EXPECT_EQ(1u, GetNumberOfSettingsWindows());
// Launching via LaunchService should also de-dupe to the same browser.
web_app::AppId settings_app_id = *web_app::GetAppIdForSystemWebApp(
......@@ -125,7 +106,7 @@ IN_PROC_BROWSER_TEST_F(SettingsWindowManagerTest, OpenSettingsWindow) {
apps::mojom::AppLaunchSource::kSourceCommandLine));
EXPECT_EQ(contents,
settings_browser->tab_strip_model()->GetActiveWebContents());
EXPECT_EQ(1u, observer_.new_settings_count());
EXPECT_EQ(1u, GetNumberOfSettingsWindows());
// Close the settings window.
CloseBrowserSynchronously(settings_browser);
......@@ -136,7 +117,7 @@ IN_PROC_BROWSER_TEST_F(SettingsWindowManagerTest, OpenSettingsWindow) {
Browser* settings_browser2 =
settings_manager_->FindBrowserForProfile(browser()->profile());
ASSERT_TRUE(settings_browser2);
EXPECT_EQ(2u, observer_.new_settings_count());
EXPECT_EQ(1u, GetNumberOfSettingsWindows());
CloseBrowserSynchronously(settings_browser2);
}
......@@ -183,18 +164,19 @@ IN_PROC_BROWSER_TEST_F(SettingsWindowManagerTest, OpenSettings) {
// OS settings opens in a new window.
settings_manager_->ShowOSSettings(browser()->profile());
EXPECT_EQ(1u, observer_.new_settings_count());
EXPECT_EQ(1u, GetNumberOfSettingsWindows());
EXPECT_EQ(2u, chrome::GetTotalBrowserCount());
// The opened Settings window should be the active browser.
content::WebContents* web_contents =
observer_.browser()->tab_strip_model()->GetWebContentsAt(0);
chrome::FindLastActive()->tab_strip_model()->GetWebContentsAt(0);
EXPECT_EQ(chrome::kChromeUIOSSettingsHost, web_contents->GetURL().host());
// Showing an OS sub-page reuses the OS settings window.
settings_manager_->ShowOSSettings(
browser()->profile(),
chromeos::settings::mojom::kBluetoothDevicesSubpagePath);
EXPECT_EQ(1u, observer_.new_settings_count());
EXPECT_EQ(1u, GetNumberOfSettingsWindows());
EXPECT_EQ(2u, chrome::GetTotalBrowserCount());
// Close the settings window.
......
......@@ -8,7 +8,6 @@
#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"
......@@ -19,15 +18,12 @@
#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 {
class SystemMenuModelBuilderMultiUserTest : public chromeos::LoginManagerTest {
public:
SystemMenuModelBuilderMultiUserTest() : LoginManagerTest() {
login_mixin_.AppendRegularUsers(2);
......@@ -36,16 +32,10 @@ class SystemMenuModelBuilderMultiUserTest
}
~SystemMenuModelBuilderMultiUserTest() override = default;
// SettingsWindowManagerObserver:
void OnNewSettingsWindow(Browser* settings_browser) override {
settings_browser_ = settings_browser;
}
protected:
AccountId account_id1_;
AccountId account_id2_;
chromeos::LoginManagerMixin login_mixin_{&mixin_host_};
Browser* settings_browser_ = nullptr;
};
// Regression test for https://crbug.com/1023043
......@@ -65,16 +55,15 @@ IN_PROC_BROWSER_TEST_F(SystemMenuModelBuilderMultiUserTest,
->system_web_app_manager()
.InstallSystemAppsForTesting();
// Open the settings window and record the |settings_browser_|.
// 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_);
auto* settings_browser = manager->FindBrowserForProfile(profile);
ASSERT_TRUE(settings_browser);
// Copy the command ids from the system menu.
BrowserView* browser_view =
BrowserView::GetBrowserViewForBrowser(settings_browser_);
BrowserView::GetBrowserViewForBrowser(settings_browser);
ui::MenuModel* menu = browser_view->frame()->GetSystemMenuModel();
std::set<int> commands;
for (int i = 0; i < menu->GetItemCount(); i++)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment