Fix CreateApplicationWindow to create windows on display_id

Launching terminal via shelf context menu, or launcher is not currently
using the same display as the shelf.  The display_id is available
through most of the call chain, but is needed in a few extra places.

CreateApplicationWindow receives display_id in AppLaunchParams, but was
never setting Screen::SetDisplayForNewWindows which is required to
ensure that WindowSizer will put the new window on the specified
display.

Same for WebAppLaunchManager::OpenApplication.

Updated LaunchSystemWebApp for all callers to pass display_id where it
is available.

Bug: 1083825
Change-Id: I5e7046af772b09a3c5340083ded33e42642acd6d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2208799Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Ben Wells <benwells@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Joel Hockey <joelhockey@chromium.org>
Cr-Commit-Position: refs/heads/master@{#770914}

chrome/browser/chromeos/crostini/crostini_installer.cc

@@ -35,6 +35,7 @@
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/network_service_instance.h"
+#include "ui/display/types/display_constants.h"
 
 using crostini::mojom::InstallerError;
 using crostini::mojom::InstallerState;
@@ -661,8 +662,9 @@ void CrostiniInstaller::OnCrostiniRestartFinished(CrostiniResult result) {
   progress_callback_.Reset();
 
   if (!skip_launching_terminal_for_testing_) {
+    // kInvalidDisplayId will launch terminal on the current active display.
     crostini::LaunchContainerTerminal(
-        profile_, crostini::kCrostiniDefaultVmName,
+        profile_, display::kInvalidDisplayId, crostini::kCrostiniDefaultVmName,
         crostini::kCrostiniDefaultContainerName, std::vector<std::string>());
   }
 }

chrome/browser/chromeos/crostini/crostini_terminal.cc

@@ -79,11 +79,11 @@ GURL GenerateVshInCroshUrl(Profile* profile,
   return GURL(base::JoinString(pieces, "&args[]="));
 }
 
-apps::AppLaunchParams GenerateTerminalAppLaunchParams() {
+apps::AppLaunchParams GenerateTerminalAppLaunchParams(int64_t display_id) {
   apps::AppLaunchParams launch_params(
       /*app_id=*/"", apps::mojom::LaunchContainer::kLaunchContainerWindow,
       WindowOpenDisposition::NEW_WINDOW,
-      apps::mojom::AppLaunchSource::kSourceAppLauncher);
+      apps::mojom::AppLaunchSource::kSourceAppLauncher, display_id);
   launch_params.override_app_name =
       AppNameFromCrostiniAppId(kCrostiniTerminalId);
   return launch_params;
@@ -107,18 +107,22 @@ void ShowContainerTerminal(Profile* profile,
 }
 
 void LaunchContainerTerminal(Profile* profile,
+                             int64_t display_id,
                              const std::string& vm_name,
                              const std::string& container_name,
                              const std::vector<std::string>& terminal_args) {
   GURL vsh_in_crosh_url =
       GenerateVshInCroshUrl(profile, vm_name, container_name, terminal_args);
   if (base::FeatureList::IsEnabled(features::kTerminalSystemApp)) {
-    web_app::LaunchSystemWebApp(profile, web_app::SystemAppType::TERMINAL,
-                                vsh_in_crosh_url);
+    web_app::LaunchSystemWebApp(
+        profile, web_app::SystemAppType::TERMINAL, vsh_in_crosh_url,
+        web_app::CreateSystemWebAppLaunchParams(
+            profile, web_app::SystemAppType::TERMINAL, display_id));
     return;
   }
 
-  apps::AppLaunchParams launch_params = GenerateTerminalAppLaunchParams();
+  apps::AppLaunchParams launch_params =
+      GenerateTerminalAppLaunchParams(display_id);
 
   Browser* browser =
       CreateContainerTerminal(profile, launch_params, vsh_in_crosh_url);
@@ -126,18 +130,21 @@ void LaunchContainerTerminal(Profile* profile,
 }
 
 Browser* LaunchTerminal(Profile* profile,
+                        int64_t display_id,
                         const std::string& vm_name,
                         const std::string& container_name) {
   GURL vsh_in_crosh_url = GenerateVshInCroshUrl(
       profile, vm_name, container_name, std::vector<std::string>());
-  return web_app::LaunchSystemWebApp(profile, web_app::SystemAppType::TERMINAL,
-                                     vsh_in_crosh_url);
+  return web_app::LaunchSystemWebApp(
+      profile, web_app::SystemAppType::TERMINAL, vsh_in_crosh_url,
+      web_app::CreateSystemWebAppLaunchParams(
+          profile, web_app::SystemAppType::TERMINAL, display_id));
 }
 
-Browser* LaunchTerminalSettings(Profile* profile) {
+Browser* LaunchTerminalSettings(Profile* profile, int64_t display_id) {
   DCHECK(base::FeatureList::IsEnabled(features::kTerminalSystemApp));
   auto params = web_app::CreateSystemWebAppLaunchParams(
-      profile, web_app::SystemAppType::TERMINAL);
+      profile, web_app::SystemAppType::TERMINAL, display_id);
   if (!params.has_value()) {
     LOG(WARNING) << "Empty launch params for terminal";
     return nullptr;
@@ -152,7 +159,7 @@ Browser* LaunchTerminalSettings(Profile* profile) {
   return web_app::LaunchSystemWebApp(
       profile, web_app::SystemAppType::TERMINAL,
       GURL(base::StrCat({chrome::kChromeUIUntrustedTerminalURL, path})),
-      *params);
+      std::move(params));
 }
 
 void RecordTerminalSettingsChangesUMAs(Profile* profile) {

chrome/browser/chromeos/crostini/crostini_terminal.h

@@ -10,6 +10,7 @@
 #include "base/optional.h"
 #include "chrome/browser/apps/app_service/app_launch_params.h"
 #include "chrome/browser/chromeos/crostini/crostini_util.h"
+#include "ui/display/types/display_constants.h"
 #include "ui/gfx/geometry/point.h"
 
 class GURL;
@@ -105,7 +106,7 @@ GURL GenerateVshInCroshUrl(Profile* profile,
                            const std::vector<std::string>& terminal_args);
 
 // Generate AppLaunchParams for the Crostini terminal application.
-apps::AppLaunchParams GenerateTerminalAppLaunchParams();
+apps::AppLaunchParams GenerateTerminalAppLaunchParams(int64_t display_id);
 
 // Create the crosh-in-a-window that displays a shell in an container on a VM.
 Browser* CreateContainerTerminal(Profile* profile,
@@ -123,6 +124,7 @@ void ShowContainerTerminal(Profile* profile,
 // container on a VM and passes |terminal_args| as parameters to that shell
 // which will cause them to be executed as program inside that shell.
 void LaunchContainerTerminal(Profile* profile,
+                             int64_t display_id,
                              const std::string& vm_name,
                              const std::string& container_name,
                              const std::vector<std::string>& terminal_args);
@@ -130,11 +132,14 @@ void LaunchContainerTerminal(Profile* profile,
 // Launches the terminal tabbed app.
 Browser* LaunchTerminal(
     Profile* profile,
+    int64_t display_id = display::kInvalidDisplayId,
     const std::string& vm_name = kCrostiniDefaultVmName,
     const std::string& container_name = kCrostiniDefaultContainerName);
 
 // Launches the terminal settings popup window.
-Browser* LaunchTerminalSettings(Profile* profile);
+Browser* LaunchTerminalSettings(
+    Profile* profile,
+    int64_t display_id = display::kInvalidDisplayId);
 
 // Record which terminal settings have been changed by users.
 void RecordTerminalSettingsChangesUMAs(Profile* profile);

chrome/browser/chromeos/crostini/crostini_util.cc

@@ -408,7 +408,8 @@ void LaunchCrostiniAppImpl(
     RecordAppLaunchHistogram(CrostiniAppLaunchAppType::kTerminal);
 
     if (base::FeatureList::IsEnabled(features::kTerminalSystemApp)) {
-      auto* browser = LaunchTerminal(profile, vm_name, container_name);
+      auto* browser =
+          LaunchTerminal(profile, display_id, vm_name, container_name);
       if (browser == nullptr) {
         RecordAppLaunchResultHistogram(crostini::CrostiniResult::UNKNOWN_ERROR);
       } else {
@@ -419,7 +420,8 @@ void LaunchCrostiniAppImpl(
 
     GURL vsh_in_crosh_url = GenerateVshInCroshUrl(
         profile, vm_name, container_name, std::vector<std::string>());
-    apps::AppLaunchParams launch_params = GenerateTerminalAppLaunchParams();
+    apps::AppLaunchParams launch_params =
+        GenerateTerminalAppLaunchParams(display_id);
     // Create the terminal here so it's created in the right display. If the
     // browser creation is delayed into the callback the root window for new
     // windows setting can be changed due to the launcher or shelf dismissal.

chrome/browser/chromeos/dbus/vm_applications_service_provider.cc

@@ -22,6 +22,7 @@
 #include "dbus/bus.h"
 #include "dbus/message.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
+#include "ui/display/types/display_constants.h"
 
 namespace chromeos {
 
@@ -110,8 +111,10 @@ void VmApplicationsServiceProvider::LaunchTerminal(
   Profile* profile = ProfileManager::GetPrimaryUserProfile();
   if (crostini::CrostiniFeatures::Get()->IsEnabled(profile) &&
       request.owner_id() == crostini::CryptohomeIdForProfile(profile)) {
+    // kInvalidDisplayId will launch terminal on the current active display.
     crostini::LaunchContainerTerminal(
-        profile, request.vm_name(), request.container_name(),
+        profile, display::kInvalidDisplayId, request.vm_name(),
+        request.container_name(),
         std::vector<std::string>(request.params().begin(),
                                  request.params().end()));
   }

chrome/browser/ui/DEPS

@@ -10,6 +10,9 @@ include_rules = [
 ]
 
 specific_include_rules = {
+  "application_launch_browsertest\.cc": [
+    "+ash/shell.h",
+  ],
   "browser_finder_chromeos_browsertest\.cc": [
     "+ash/wm/desks/desk.h",
     "+ash/wm/desks/desks_controller.h",

chrome/browser/ui/app_list/app_service/app_service_context_menu.cc

@@ -114,7 +114,8 @@ void AppServiceContextMenu::ExecuteCommand(int command_id, int event_flags) {
 
     case ash::SETTINGS:
       if (app_id() == crostini::GetTerminalId())
-        crostini::LaunchTerminalSettings(profile());
+        crostini::LaunchTerminalSettings(profile(),
+                                         controller()->GetAppListDisplayId());
       break;
 
     case ash::APP_CONTEXT_MENU_NEW_WINDOW:

chrome/browser/ui/ash/launcher/app_service/app_service_shelf_context_menu.cc

@@ -188,7 +188,7 @@ void AppServiceShelfContextMenu::ExecuteCommand(int command_id,
 
     case ash::SETTINGS:
       if (item().id.app_id == crostini::GetTerminalId())
-        crostini::LaunchTerminalSettings(controller()->profile());
+        crostini::LaunchTerminalSettings(controller()->profile(), display_id());
       return;
 
     default:

chrome/browser/ui/extensions/application_launch.cc

@@ -59,6 +59,7 @@
 #include "extensions/common/manifest_handlers/options_page_info.h"
 #include "third_party/blink/public/common/features.h"
 #include "ui/base/window_open_disposition.h"
+#include "ui/display/scoped_display_for_new_windows.h"
 #include "ui/gfx/geometry/rect.h"
 
 #if defined(OS_MACOSX)
@@ -411,6 +412,9 @@ Browser* CreateApplicationWindow(Profile* profile,
         extensions::AppLaunchInfo::GetLaunchHeight(extension));
   }
 
+  // Place new windows on the specified display.
+  display::ScopedDisplayForNewWindows scoped_display(params.display_id);
+
   // TODO(erg): AppLaunchParams should pass through the user_gesture from the
   // extension system here.
   Browser::CreateParams browser_params(

chrome/browser/ui/extensions/application_launch_browsertest.cc

@@ -14,6 +14,18 @@
 #include "content/public/test/browser_test_utils.h"
 #include "url/gurl.h"
 
+#if defined(OS_CHROMEOS)
+#include "ash/shell.h"
+#include "chrome/browser/ui/browser_window.h"
+#include "chrome/browser/ui/extensions/application_launch.h"
+#include "components/services/app_service/public/mojom/types.mojom.h"
+#include "ui/base/base_window.h"
+#include "ui/base/window_open_disposition.h"
+#include "ui/display/screen.h"
+#include "ui/display/test/display_manager_test_api.h"
+#include "ui/gfx/native_widget_types.h"
+#endif  // OS_CHROMEOS
+
 class ApplicationLaunchBrowserTest : public InProcessBrowserTest {
  public:
   ApplicationLaunchBrowserTest() {
@@ -66,3 +78,32 @@ IN_PROC_BROWSER_TEST_F(ApplicationLaunchBrowserTest,
   EXPECT_EQ(2, browser()->tab_strip_model()->count());
   EXPECT_TRUE(app_browser->is_focus_mode());
 }
+
+#if defined(OS_CHROMEOS)
+IN_PROC_BROWSER_TEST_F(ApplicationLaunchBrowserTest, CreateWindowInDisplay) {
+  display::Screen* screen = display::Screen::GetScreen();
+  // Create 2 displays.
+  display::DisplayManager* display_manager =
+      ash::Shell::Get()->display_manager();
+  display::test::DisplayManagerTestApi display_manager_test(display_manager);
+  display_manager_test.UpdateDisplay("800x800,801+0-800x800");
+  int64_t display1 = screen->GetPrimaryDisplay().id();
+  int64_t display2 = display_manager_test.GetSecondaryDisplay().id();
+  EXPECT_EQ(2, screen->GetNumDisplays());
+
+  // Primary display should hold browser() and be the default for new windows.
+  gfx::NativeWindow window = browser()->window()->GetNativeWindow();
+  EXPECT_EQ(display1, screen->GetDisplayNearestWindow(window).id());
+  EXPECT_EQ(display1, screen->GetDisplayForNewWindows().id());
+
+  // Create browser2 on display 2.
+  apps::AppLaunchParams params(
+      "app_id", apps::mojom::LaunchContainer::kLaunchContainerWindow,
+      WindowOpenDisposition::NEW_WINDOW,
+      apps::mojom::AppLaunchSource::kSourceAppLauncher, display2);
+  Browser* browser2 =
+      CreateApplicationWindow(browser()->profile(), params, GURL());
+  gfx::NativeWindow window2 = browser2->window()->GetNativeWindow();
+  EXPECT_EQ(display2, screen->GetDisplayNearestWindow(window2).id());
+}
+#endif  // OS_CHROMEOS

chrome/browser/ui/settings_window_manager_chromeos.cc

@@ -78,7 +78,8 @@ void SettingsWindowManager::ShowChromePageForProfile(Profile* profile,
   if (AreSystemWebAppsEnabled(profile)) {
     bool did_create;
     Browser* browser = web_app::LaunchSystemWebApp(
-        profile, web_app::SystemAppType::SETTINGS, gurl, &did_create);
+        profile, web_app::SystemAppType::SETTINGS, gurl,
+        /*params=*/base::nullopt, &did_create);
     ShowSettingsOnCurrentDesktop(browser);
     // Only notify if we created a new browser.
     if (!did_create || !browser)

chrome/browser/ui/web_applications/app_browser_controller_browsertest.cc

@@ -38,6 +38,7 @@
 #include "extensions/browser/extension_registry.h"
 #include "ui/display/display.h"
 #include "ui/display/screen.h"
+#include "ui/display/types/display_constants.h"
 
 namespace {
 SkColor GetTabColor(Browser* browser) {
@@ -109,7 +110,8 @@ class AppBrowserControllerBrowserTest : public InProcessBrowserTest {
   void InstallAndLaunchMockPopup() {
     test_system_web_app_installation_->WaitForAppInstall();
     auto params = web_app::CreateSystemWebAppLaunchParams(
-        browser()->profile(), test_system_web_app_installation_->GetType());
+        browser()->profile(), test_system_web_app_installation_->GetType(),
+        display::kInvalidDisplayId);
     params->disposition = WindowOpenDisposition::NEW_POPUP;
     app_browser_ = web_app::LaunchSystemWebApp(
         browser()->profile(), test_system_web_app_installation_->GetType(),

chrome/browser/ui/web_applications/system_web_app_ui_utils.cc

@@ -55,7 +55,8 @@ base::Optional<AppId> GetAppIdForSystemWebApp(Profile* profile,
 
 base::Optional<apps::AppLaunchParams> CreateSystemWebAppLaunchParams(
     Profile* profile,
-    SystemAppType app_type) {
+    SystemAppType app_type,
+    int64_t display_id) {
   base::Optional<AppId> app_id = GetAppIdForSystemWebApp(profile, app_type);
   // TODO(calamity): Decide whether to report app launch failure or CHECK fail.
   if (!app_id)
@@ -70,29 +71,13 @@ base::Optional<apps::AppLaunchParams> CreateSystemWebAppLaunchParams(
   // TODO(calamity): Plumb through better launch sources from callsites.
   apps::AppLaunchParams params = apps::CreateAppIdLaunchParamsWithEventFlags(
       app_id.value(), /*event_flags=*/0,
-      apps::mojom::AppLaunchSource::kSourceChromeInternal,
-      display::kInvalidDisplayId, /*fallback_container=*/
+      apps::mojom::AppLaunchSource::kSourceChromeInternal, display_id,
+      /*fallback_container=*/
       ConvertDisplayModeToAppLaunchContainer(display_mode));
 
   return params;
 }
 
-Browser* LaunchSystemWebApp(Profile* profile,
-                            SystemAppType app_type,
-                            const GURL& url,
-                            bool* did_create) {
-  if (did_create)
-    *did_create = false;
-
-  base::Optional<apps::AppLaunchParams> params =
-      CreateSystemWebAppLaunchParams(profile, app_type);
-  if (!params)
-    return nullptr;
-  params->override_url = url;
-
-  return LaunchSystemWebApp(profile, app_type, url, *params, did_create);
-}
-
 namespace {
 base::FilePath GetLaunchDirectory(
     const std::vector<base::FilePath>& launch_files) {
@@ -117,20 +102,28 @@ base::FilePath GetLaunchDirectory(
 Browser* LaunchSystemWebApp(Profile* profile,
                             SystemAppType app_type,
                             const GURL& url,
-                            const apps::AppLaunchParams& params,
+                            base::Optional<apps::AppLaunchParams> params,
                             bool* did_create) {
   auto* provider = WebAppProvider::Get(profile);
   if (!provider)
     return nullptr;
 
-  DCHECK_EQ(params.app_id, *GetAppIdForSystemWebApp(profile, app_type));
+  if (!params) {
+    params = CreateSystemWebAppLaunchParams(profile, app_type,
+                                            display::kInvalidDisplayId);
+  }
+  if (!params)
+    return nullptr;
+  params->override_url = url;
+
+  DCHECK_EQ(params->app_id, *GetAppIdForSystemWebApp(profile, app_type));
 
   // 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;
   Browser::Type browser_type = Browser::TYPE_APP;
-  if (params.disposition == WindowOpenDisposition::NEW_POPUP)
+  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)) {
@@ -145,38 +138,38 @@ Browser* LaunchSystemWebApp(Profile* profile,
 
   if (base::FeatureList::IsEnabled(features::kDesktopPWAsWithoutExtensions)) {
     if (!browser)
-      browser = CreateWebApplicationWindow(profile, params.app_id,
-                                           params.disposition);
+      browser = CreateWebApplicationWindow(profile, params->app_id,
+                                           params->disposition);
 
     // Navigate application window to application's |url| if necessary.
     web_contents = browser->tab_strip_model()->GetWebContentsAt(0);
     if (!web_contents || web_contents->GetURL() != url) {
       web_contents = NavigateWebApplicationWindow(
-          browser, params.app_id, url, WindowOpenDisposition::CURRENT_TAB);
+          browser, params->app_id, url, WindowOpenDisposition::CURRENT_TAB);
     }
   } else {
     if (!browser)
-      browser = CreateApplicationWindow(profile, params, url);
+      browser = CreateApplicationWindow(profile, *params, url);
 
     // Navigate application window to application's |url| if necessary.
     web_contents = browser->tab_strip_model()->GetWebContentsAt(0);
     if (!web_contents || web_contents->GetURL() != url) {
       web_contents = NavigateApplicationWindow(
-          browser, params, url, WindowOpenDisposition::CURRENT_TAB);
+          browser, *params, url, WindowOpenDisposition::CURRENT_TAB);
     }
   }
 
   // Send launch files.
   if (provider->file_handler_manager().IsFileHandlingAPIAvailable(
-          params.app_id)) {
+          params->app_id)) {
     if (provider->system_web_app_manager().AppShouldReceiveLaunchDirectory(
             app_type)) {
       web_launch::WebLaunchFilesHelper::SetLaunchDirectoryAndLaunchPaths(
           web_contents, web_contents->GetURL(),
-          GetLaunchDirectory(params.launch_files), params.launch_files);
+          GetLaunchDirectory(params->launch_files), params->launch_files);
     } else {
       web_launch::WebLaunchFilesHelper::SetLaunchPaths(
-          web_contents, web_contents->GetURL(), params.launch_files);
+          web_contents, web_contents->GetURL(), params->launch_files);
     }
   }
 

chrome/browser/ui/web_applications/system_web_app_ui_utils.h

@@ -28,23 +28,19 @@ base::Optional<AppId> GetAppIdForSystemWebApp(Profile* profile,
 
 base::Optional<apps::AppLaunchParams> CreateSystemWebAppLaunchParams(
     Profile* profile,
-    SystemAppType app_type);
+    SystemAppType app_type,
+    int64_t display_id);
 
 // 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. |did_create| will reflect whether a new window was created if passed.
-//
-// TODO(calamity) Separate this into LaunchSystemWebApp and
-// LaunchSystemWebAppPopup.
-Browser* LaunchSystemWebApp(Profile* profile,
-                            SystemAppType app_type,
-                            const GURL& url = GURL(),
-                            bool* did_create = nullptr);
-Browser* LaunchSystemWebApp(Profile* profile,
-                            SystemAppType app_type,
-                            const GURL& url,
-                            const apps::AppLaunchParams& params,
-                            bool* did_create = nullptr);
+// failed. CreateSystemWebAppLaunchParams() can be used to create |params|.
+// |did_create| will reflect whether a new window was created if passed.
+Browser* LaunchSystemWebApp(
+    Profile* profile,
+    SystemAppType app_type,
+    const GURL& url,
+    base::Optional<apps::AppLaunchParams> params = base::nullopt,
+    bool* did_create = nullptr);
 
 // Returns a browser that is hosting the given system app type and browser type,
 // or nullptr if not found.

chrome/browser/ui/web_applications/web_app_launch_manager.cc

@@ -41,6 +41,7 @@
 #include "third_party/blink/public/mojom/renderer_preferences.mojom.h"
 #include "ui/base/page_transition_types.h"
 #include "ui/base/window_open_disposition.h"
+#include "ui/display/scoped_display_for_new_windows.h"
 #include "url/gurl.h"
 
 namespace web_app {
@@ -124,6 +125,9 @@ content::WebContents* WebAppLaunchManager::OpenApplication(
                 .value_or(provider_->registrar().GetAppLaunchURL(params.app_id))
           : params.override_url;
 
+  // Place new windows on the specified display.
+  display::ScopedDisplayForNewWindows scoped_display(params.display_id);
+
   // System Web Apps go through their own launch path.
   base::Optional<SystemAppType> system_app_type =
       GetSystemWebAppTypeForAppId(profile_, params.app_id);