Always reuse System Web App popups

Terminal app should only have 1 instance of
the settings page open.  For now, we
will assume that all system web apps
will have only 1 instance of a popup,
we can make this configurable per app
if needed.

Bug: 1056983
Change-Id: Ib6414fc0e313beaddccc19f0bb23cedf6ccb4faa
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2094911
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#749074}

chrome/browser/ui/web_applications/app_browser_controller_browsertest.cc

@@ -14,6 +14,7 @@
 #include "chrome/browser/themes/custom_theme_supplier.h"
 #include "chrome/browser/themes/theme_properties.h"
 #include "chrome/browser/ui/browser_commands.h"
+#include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
 #include "chrome/browser/ui/extensions/application_launch.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
@@ -209,6 +210,15 @@ IN_PROC_BROWSER_TEST_F(AppBrowserControllerBrowserTest,
   EXPECT_FALSE(app_browser_->app_controller()->GetThemeColor().has_value());
 }
 
+IN_PROC_BROWSER_TEST_F(AppBrowserControllerBrowserTest,
+                       ReuseBrowserForSystemAppPopup) {
+  InstallAndLaunchMockPopup();
+  // We should have the original browser for this BrowserTest, plus new popup.
+  EXPECT_EQ(BrowserList::GetInstance()->size(), 2u);
+  InstallAndLaunchMockPopup();
+  EXPECT_EQ(BrowserList::GetInstance()->size(), 2u);
+}
+
 class AppBrowserControllerChromeUntrustedBrowserTest
     : public InProcessBrowserTest {
  public:

chrome/browser/ui/web_applications/system_web_app_ui_utils.cc

@@ -15,7 +15,6 @@
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/apps/app_service/app_launch_params.h"
 #include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_navigator.h"
 #include "chrome/browser/ui/browser_navigator_params.h"
@@ -124,10 +123,16 @@ Browser* LaunchSystemWebApp(Profile* profile,
 
   DCHECK_EQ(params.app_id, *GetAppIdForSystemWebApp(profile, app_type));
 
-  // Make sure we have a browser for app.
+  // Make sure we have a browser for app.  Always reuse an existing browser for
+  // popups, otherwise check app type whether we should use a single window.
+  // TODO(crbug.com/1060423): Allow apps to control whether popups are single.
   Browser* browser = nullptr;
-  if (provider->system_web_app_manager().IsSingleWindow(app_type)) {
-    browser = FindSystemWebAppBrowser(profile, app_type);
+  Browser::Type browser_type = Browser::TYPE_APP;
+  if (params.disposition == WindowOpenDisposition::NEW_POPUP)
+    browser_type = Browser::TYPE_APP_POPUP;
+  if (browser_type == Browser::TYPE_APP_POPUP ||
+      provider->system_web_app_manager().IsSingleWindow(app_type)) {
+    browser = FindSystemWebAppBrowser(profile, app_type, browser_type);
   }
 
   // We create the app window if no existing browser found.
@@ -177,7 +182,9 @@ Browser* LaunchSystemWebApp(Profile* profile,
   return browser;
 }
 
-Browser* FindSystemWebAppBrowser(Profile* profile, SystemAppType app_type) {
+Browser* FindSystemWebAppBrowser(Profile* profile,
+                                 SystemAppType app_type,
+                                 Browser::Type browser_type) {
   // TODO(calamity): Determine whether, during startup, we need to wait for
   // app install and then provide a valid answer here.
   base::Optional<AppId> app_id = GetAppIdForSystemWebApp(profile, app_type);
@@ -191,7 +198,7 @@ Browser* FindSystemWebAppBrowser(Profile* profile, SystemAppType app_type) {
     return nullptr;
 
   for (auto* browser : *BrowserList::GetInstance()) {
-    if (browser->profile() != profile || !browser->deprecated_is_app())
+    if (browser->profile() != profile || browser->type() != browser_type)
       continue;
 
     if (GetAppIdFromApplicationName(browser->app_name()) == app_id.value())

chrome/browser/ui/web_applications/system_web_app_ui_utils.h

@@ -9,11 +9,11 @@
 
 #include "base/optional.h"
 #include "chrome/browser/apps/app_service/app_launch_params.h"
+#include "chrome/browser/ui/browser.h"
 #include "chrome/browser/web_applications/components/web_app_id.h"
 #include "chrome/browser/web_applications/system_web_app_manager.h"
 #include "url/gurl.h"
 
-class Browser;
 class Profile;
 
 namespace web_app {
@@ -46,9 +46,12 @@ Browser* LaunchSystemWebApp(Profile* profile,
                             const apps::AppLaunchParams& params,
                             bool* did_create = nullptr);
 
-// Returns a browser that is hosting the given system app type, or nullptr if
-// not found.
-Browser* FindSystemWebAppBrowser(Profile* profile, SystemAppType app_type);
+// Returns a browser that is hosting the given system app type and browser type,
+// or nullptr if not found.
+Browser* FindSystemWebAppBrowser(
+    Profile* profile,
+    SystemAppType app_type,
+    Browser::Type browser_type = Browser::TYPE_APP);
 
 // Returns true if the |browser| is a system web app.
 bool IsSystemWebApp(Browser* browser);