[System Web Apps] Add SettingsWindowManager test.

Bug: 836128
Change-Id: I10a8ac0af55dbd313419a4effaa9305ef8a890c2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1517213
Commit-Queue: calamity <calamity@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Alexey Baskakov <loyso@chromium.org>
Cr-Commit-Position: refs/heads/master@{#658390}

chrome/browser/ui/settings_window_manager_browsertest_chromeos.cc

@@ -13,6 +13,9 @@
 #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/web_applications/system_web_app_manager.h"
+#include "chrome/browser/web_applications/web_app_provider.h"
+#include "chrome/common/chrome_features.h"
 #include "chrome/common/url_constants.h"
 #include "chrome/common/webui_url_constants.h"
 #include "chrome/test/base/in_process_browser_test.h"
@@ -44,12 +47,33 @@ class SettingsWindowTestObserver
 
 }  // namespace
 
-class SettingsWindowManagerTest : public InProcessBrowserTest {
+class SettingsWindowManagerTest : public InProcessBrowserTest,
+                                  public ::testing::WithParamInterface<bool> {
  public:
   SettingsWindowManagerTest()
       : settings_manager_(chrome::SettingsWindowManager::GetInstance()) {
     settings_manager_->AddObserver(&observer_);
+    if (EnableSystemWebApps())
+      scoped_feature_list_.InitAndEnableFeature(features::kSystemWebApps);
+    else
+      scoped_feature_list_.InitAndDisableFeature(features::kSystemWebApps);
   }
+
+  void SetUpOnMainThread() override {
+    if (!EnableSystemWebApps())
+      return;
+
+    // Wait for the Settings System Web App to install.
+    base::RunLoop run_loop;
+    web_app::WebAppProvider::Get(browser()->profile())
+        ->system_web_app_manager()
+        .on_apps_synchronized()
+        .Post(FROM_HERE, run_loop.QuitClosure());
+    run_loop.Run();
+  }
+
+  bool EnableSystemWebApps() { return GetParam(); }
+
   ~SettingsWindowManagerTest() override {
     settings_manager_->RemoveObserver(&observer_);
   }
@@ -74,11 +98,12 @@ class SettingsWindowManagerTest : public InProcessBrowserTest {
  protected:
   chrome::SettingsWindowManager* settings_manager_;
   SettingsWindowTestObserver observer_;
+  base::test::ScopedFeatureList scoped_feature_list_;
 
   DISALLOW_COPY_AND_ASSIGN(SettingsWindowManagerTest);
 };
 
-IN_PROC_BROWSER_TEST_F(SettingsWindowManagerTest, OpenSettingsWindow) {
+IN_PROC_BROWSER_TEST_P(SettingsWindowManagerTest, OpenSettingsWindow) {
   // Open a settings window.
   ShowSettingsForProfile(browser()->profile());
   Browser* settings_browser =
@@ -108,7 +133,7 @@ IN_PROC_BROWSER_TEST_F(SettingsWindowManagerTest, OpenSettingsWindow) {
   CloseBrowserSynchronously(settings_browser2);
 }
 
-IN_PROC_BROWSER_TEST_F(SettingsWindowManagerTest, OpenChromePages) {
+IN_PROC_BROWSER_TEST_P(SettingsWindowManagerTest, OpenChromePages) {
   EXPECT_EQ(1u, chrome::GetTotalBrowserCount());
 
   // History should open in the existing browser window.
@@ -139,7 +164,7 @@ IN_PROC_BROWSER_TEST_F(SettingsWindowManagerTest, OpenChromePages) {
   EXPECT_EQ(2u, chrome::GetTotalBrowserCount());
 }
 
-IN_PROC_BROWSER_TEST_F(SettingsWindowManagerTest, SplitSettings) {
+IN_PROC_BROWSER_TEST_P(SettingsWindowManagerTest, SplitSettings) {
   base::test::ScopedFeatureList feature_list;
   feature_list.InitAndEnableFeature(chromeos::features::kSplitSettings);
   EXPECT_EQ(1u, chrome::GetTotalBrowserCount());
@@ -167,3 +192,8 @@ IN_PROC_BROWSER_TEST_F(SettingsWindowManagerTest, SplitSettings) {
   chrome::ShowSettingsSubPage(browser(), chrome::kAutofillSubPage);
   EXPECT_EQ(1u, chrome::GetTotalBrowserCount());
 }
+
+INSTANTIATE_TEST_SUITE_P(
+    /* no prefix */,
+    SettingsWindowManagerTest,
+    ::testing::Bool());

chrome/browser/ui/settings_window_manager_chromeos.cc

@@ -52,9 +52,11 @@ void SettingsWindowManager::ShowChromePageForProfile(Profile* profile,
   // TODO(calamity): Auto-launch the settings app on install if not found, and
   // figure out how to invoke OnNewSettingsWindow() in that case.
   if (web_app::SystemWebAppManager::IsEnabled()) {
+    bool did_create;
     Browser* browser = web_app::LaunchSystemWebApp(
-        profile, web_app::SystemAppType::SETTINGS, gurl);
-    if (!browser)
+        profile, web_app::SystemAppType::SETTINGS, gurl, &did_create);
+    // Only notify if we created a new browser.
+    if (!did_create || !browser)
       return;
 
     for (SettingsWindowManagerObserver& observer : observers_)

chrome/browser/ui/web_applications/system_web_app_ui_utils_chromeos.cc

@@ -5,6 +5,7 @@
 #include "chrome/browser/ui/web_applications/system_web_app_ui_utils_chromeos.h"
 
 #include <string>
+#include <utility>
 
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
@@ -35,7 +36,11 @@ base::Optional<std::string> GetAppIdForSystemWebApp(Profile* profile,
 
 Browser* LaunchSystemWebApp(Profile* profile,
                             SystemAppType app_type,
-                            const GURL& url) {
+                            const GURL& url,
+                            bool* did_create) {
+  if (did_create)
+    *did_create = false;
+
   Browser* browser = FindSystemWebAppBrowser(profile, app_type);
   if (browser) {
     content::WebContents* web_contents =
@@ -63,11 +68,15 @@ Browser* LaunchSystemWebApp(Profile* profile,
       display::kInvalidDisplayId);
   params.override_url = url;
 
-  if (!browser)
+  if (!browser) {
+    if (did_create)
+      *did_create = true;
     browser = CreateApplicationWindow(params, url);
+  }
 
   ShowApplicationWindow(params, url, browser,
                         WindowOpenDisposition::CURRENT_TAB);
+
   return browser;
 }
 
@@ -92,7 +101,8 @@ Browser* FindSystemWebAppBrowser(Profile* profile, SystemAppType app_type) {
         extensions::ExtensionRegistry::Get(browser->profile())
             ->GetExtensionById(GetAppIdFromApplicationName(browser->app_name()),
                                extensions::ExtensionRegistry::EVERYTHING);
-    if (browser_extension == extension)
+
+    if (browser_extension->id() == extension->id())
       return browser;
   }
 

chrome/browser/ui/web_applications/system_web_app_ui_utils_chromeos.h

@@ -5,6 +5,8 @@
 #ifndef CHROME_BROWSER_UI_WEB_APPLICATIONS_SYSTEM_WEB_APP_UI_UTILS_CHROMEOS_H_
 #define CHROME_BROWSER_UI_WEB_APPLICATIONS_SYSTEM_WEB_APP_UI_UTILS_CHROMEOS_H_
 
+#include <utility>
+
 #include "base/optional.h"
 #include "chrome/browser/web_applications/system_web_app_manager.h"
 #include "url/gurl.h"
@@ -20,10 +22,11 @@ base::Optional<std::string> GetAppIdForSystemWebApp(Profile* profile,
 
 // Launches a System App to the given URL, reusing any existing window for the
 // app. Returns the browser for the System App, or nullptr if launch/focus
-// failed.
+// failed. |did_create| will reflect whether a new window was created if passed.
 Browser* LaunchSystemWebApp(Profile* profile,
                             SystemAppType app_type,
-                            const GURL& url = GURL());
+                            const GURL& url = GURL(),
+                            bool* did_create = nullptr);
 
 // Returns a browser that is hosting the given system app type, or nullptr if
 // not found.