system-web-apps: add support to pass launch_files to running Apps

- Clean up ShowApplicationWindow logic, split it into sub functions.
- Remove SetInitialFocus, which is no longer necessary.
- Remove extra HostedAppBrowserController::SetAppPrefsForWebContents
  during app launch. HostedAppBrowserController already does this.
- Set launch files before showing the window.

Fixed: 1023742
Change-Id: I074a917bb0ed557d1a967a04cb3042c9ee29113b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1955247Reviewed-by: Michael Wasserman <msw@chromium.org>
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Joel Hockey <joelhockey@chromium.org>
Commit-Queue: Jiewei Qian  <qjw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#725054}

chrome/browser/chromeos/crostini/crostini_terminal.cc

@@ -76,8 +76,9 @@ void ShowContainerTerminal(Profile* profile,
                            const apps::AppLaunchParams& launch_params,
                            const GURL& vsh_in_crosh_url,
                            Browser* browser) {
-  ShowApplicationWindow(profile, launch_params, vsh_in_crosh_url, browser,
-                        WindowOpenDisposition::NEW_FOREGROUND_TAB);
+  NavigateApplicationWindow(browser, launch_params, vsh_in_crosh_url,
+                            WindowOpenDisposition::NEW_FOREGROUND_TAB);
+  browser->window()->Show();
   browser->window()->GetNativeWindow()->SetProperty(
       kOverrideWindowIconResourceIdKey, IDR_LOGO_CROSTINI_TERMINAL);
 }

chrome/browser/ui/extensions/application_launch.cc

@@ -408,12 +408,11 @@ Browser* CreateApplicationWindow(Profile* profile,
   return new Browser(browser_params);
 }
 
-WebContents* ShowApplicationWindow(Profile* profile,
-                                   const apps::AppLaunchParams& params,
-                                   const GURL& default_url,
-                                   Browser* browser,
-                                   WindowOpenDisposition disposition) {
-  const Extension* const extension = GetExtension(profile, params);
+WebContents* NavigateApplicationWindow(Browser* browser,
+                                       const apps::AppLaunchParams& params,
+                                       const GURL& default_url,
+                                       WindowOpenDisposition disposition) {
+  const Extension* const extension = GetExtension(browser->profile(), params);
   ui::PageTransition transition =
       (extension ? ui::PAGE_TRANSITION_AUTO_BOOKMARK
                  : ui::PAGE_TRANSITION_AUTO_TOPLEVEL);
@@ -422,7 +421,7 @@ WebContents* ShowApplicationWindow(Profile* profile,
 
   if (extension && extension->from_bookmark()) {
     web_app::FileHandlerManager& file_handler_manager =
-        web_app::WebAppProviderBase::GetProviderBase(profile)
+        web_app::WebAppProviderBase::GetProviderBase(browser->profile())
             ->file_handler_manager();
     url = file_handler_manager
               .GetMatchingFileHandlerURL(params.app_id, params.launch_files)
@@ -435,8 +434,8 @@ WebContents* ShowApplicationWindow(Profile* profile,
 
   WebContents* web_contents = nav_params.navigated_or_inserted_contents;
 
-  extensions::HostedAppBrowserController::SetAppPrefsForWebContents(
-      browser->app_controller(), web_contents);
+  // TODO(https://crbug.com/1032443):
+  // Eventually move this to browser_navigator.cc: CreateTargetContents().
   if (extension && extension->from_bookmark()) {
     web_app::WebAppTabHelper* tab_helper =
         web_app::WebAppTabHelper::FromWebContents(web_contents);
@@ -444,17 +443,6 @@ WebContents* ShowApplicationWindow(Profile* profile,
     tab_helper->SetAppId(extension->id());
   }
 
-  browser->window()->Show();
-
-  // TODO(jcampan): http://crbug.com/8123 we should not need to set the initial
-  //                focus explicitly.
-  web_contents->SetInitialFocus();
-
-  if (base::FeatureList::IsEnabled(blink::features::kFileHandlingAPI)) {
-    web_launch::WebLaunchFilesHelper::SetLaunchPaths(web_contents, url,
-                                                     params.launch_files);
-  }
-
   return web_contents;
 }
 
@@ -462,8 +450,16 @@ WebContents* OpenApplicationWindow(Profile* profile,
                                    const apps::AppLaunchParams& params,
                                    const GURL& url) {
   Browser* browser = CreateApplicationWindow(profile, params, url);
-  return ShowApplicationWindow(profile, params, url, browser,
-                               WindowOpenDisposition::NEW_FOREGROUND_TAB);
+  WebContents* web_contents = NavigateApplicationWindow(
+      browser, params, url, WindowOpenDisposition::NEW_FOREGROUND_TAB);
+
+  if (base::FeatureList::IsEnabled(blink::features::kFileHandlingAPI)) {
+    web_launch::WebLaunchFilesHelper::SetLaunchPaths(
+        web_contents, web_contents->GetURL(), params.launch_files);
+  }
+
+  browser->window()->Show();
+  return web_contents;
 }
 
 void OpenApplicationWithReenablePrompt(Profile* profile,

chrome/browser/ui/extensions/application_launch.h

@@ -35,12 +35,12 @@ Browser* CreateApplicationWindow(Profile* profile,
                                  const apps::AppLaunchParams& params,
                                  const GURL& url);
 
-// Show the application window that's already created.
-content::WebContents* ShowApplicationWindow(Profile* profile,
-                                            const apps::AppLaunchParams& params,
-                                            const GURL& url,
-                                            Browser* browser,
-                                            WindowOpenDisposition disposition);
+// Navigate application window to application url, but do not show it yet.
+content::WebContents* NavigateApplicationWindow(
+    Browser* browser,
+    const apps::AppLaunchParams& params,
+    const GURL& url,
+    WindowOpenDisposition disposition);
 
 // Open the application in a way specified by |params| in a new window.
 content::WebContents* OpenApplicationWindow(Profile* profile,

chrome/browser/ui/extensions/hosted_app_browser_controller.cc

@@ -54,12 +54,9 @@ bool IsSameHostAndPort(const GURL& app_url, const GURL& page_url) {
          app_url.port() == page_url.port();
 }
 
-}  // namespace
-
-// static
-void HostedAppBrowserController::SetAppPrefsForWebContents(
-    web_app::AppBrowserController* controller,
-    content::WebContents* web_contents) {
+// Set preferences that are unique to app windows.
+void SetAppPrefsForWebContents(web_app::AppBrowserController* controller,
+                               content::WebContents* web_contents) {
   web_contents->GetMutableRendererPrefs()->can_accept_load_drops = false;
   web_contents->SyncRendererPrefs();
 
@@ -76,9 +73,8 @@ void HostedAppBrowserController::SetAppPrefsForWebContents(
   web_contents->NotifyPreferencesChanged();
 }
 
-// static
-void HostedAppBrowserController::ClearAppPrefsForWebContents(
-    content::WebContents* web_contents) {
+// Clear preferences that are unique to app windows.
+void ClearAppPrefsForWebContents(content::WebContents* web_contents) {
   web_contents->GetMutableRendererPrefs()->can_accept_load_drops = true;
   web_contents->SyncRendererPrefs();
 
@@ -88,6 +84,8 @@ void HostedAppBrowserController::ClearAppPrefsForWebContents(
   web_contents->NotifyPreferencesChanged();
 }
 
+}  // namespace
+
 HostedAppBrowserController::HostedAppBrowserController(Browser* browser)
     : AppBrowserController(
           browser,
@@ -273,13 +271,12 @@ void HostedAppBrowserController::OnReceivedInitialURL() {
 
 void HostedAppBrowserController::OnTabInserted(content::WebContents* contents) {
   AppBrowserController::OnTabInserted(contents);
-  extensions::HostedAppBrowserController::SetAppPrefsForWebContents(this,
-                                                                    contents);
+  SetAppPrefsForWebContents(this, contents);
 }
 
 void HostedAppBrowserController::OnTabRemoved(content::WebContents* contents) {
   AppBrowserController::OnTabRemoved(contents);
-  extensions::HostedAppBrowserController::ClearAppPrefsForWebContents(contents);
+  ClearAppPrefsForWebContents(contents);
 }
 
 }  // namespace extensions

chrome/browser/ui/extensions/hosted_app_browser_controller.h

@@ -31,14 +31,6 @@ class Extension;
 class HostedAppBrowserController : public web_app::AppBrowserController,
                                    public ExtensionUninstallDialog::Delegate {
  public:
-  // Functions to set preferences that are unique to app windows.
-  static void SetAppPrefsForWebContents(
-      web_app::AppBrowserController* controller,
-      content::WebContents* web_contents);
-
-  // Clear preferences that are unique to app windows.
-  static void ClearAppPrefsForWebContents(content::WebContents* web_contents);
-
   explicit HostedAppBrowserController(Browser* browser);
   ~HostedAppBrowserController() override;
 

chrome/browser/ui/web_applications/system_web_app_ui_utils.cc

@@ -26,11 +26,13 @@
 #include "chrome/browser/web_applications/components/web_app_helpers.h"
 #include "chrome/browser/web_applications/system_web_app_manager.h"
 #include "chrome/browser/web_applications/web_app_provider.h"
+#include "chrome/browser/web_launch/web_launch_files_helper.h"
 #include "chrome/common/extensions/manifest_handlers/app_launch_info.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_ui_data_source.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/extension.h"
+#include "third_party/blink/public/common/features.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/base/template_expressions.h"
@@ -95,34 +97,35 @@ Browser* LaunchSystemWebApp(Profile* profile,
 
   DCHECK_EQ(params.app_id, *GetAppIdForSystemWebApp(profile, app_type));
 
-  if (did_create)
-    *did_create = false;
-
-  // TODO(https://crbug.com/1027030): Better understand and improve SWA launch
-  // behavior. Early exit here skips logic in ShowApplicationWindow.
+  // Make sure we have a browser for app.
   Browser* browser = nullptr;
   if (provider->system_web_app_manager().IsSingleWindow(app_type)) {
     browser = FindSystemWebAppBrowser(profile, app_type);
   }
-
-  if (browser) {
-    content::WebContents* web_contents =
-        browser->tab_strip_model()->GetWebContentsAt(0);
-    if (web_contents && web_contents->GetURL() == url) {
-      browser->window()->Show();
-      return browser;
-    }
-  }
-
   if (!browser) {
+    browser = CreateApplicationWindow(profile, params, url);
     if (did_create)
       *did_create = true;
-    browser = CreateApplicationWindow(profile, params, url);
+  } else {
+    if (did_create)
+      *did_create = false;
   }
 
-  ShowApplicationWindow(profile, params, url, browser,
-                        WindowOpenDisposition::CURRENT_TAB);
+  // Navigate application window to application's |url| if necessary.
+  content::WebContents* web_contents =
+      browser->tab_strip_model()->GetWebContentsAt(0);
+  if (!web_contents || web_contents->GetURL() != url) {
+    web_contents = NavigateApplicationWindow(
+        browser, params, url, WindowOpenDisposition::CURRENT_TAB);
+  }
+
+  // Send launch files.
+  if (base::FeatureList::IsEnabled(blink::features::kFileHandlingAPI)) {
+    web_launch::WebLaunchFilesHelper::SetLaunchPaths(
+        web_contents, web_contents->GetURL(), params.launch_files);
+  }
 
+  browser->window()->Show();
   return browser;
 }
 

chrome/browser/ui/web_applications/web_app_launch_manager.cc

@@ -49,49 +49,6 @@ Browser* CreateWebApplicationWindow(Profile* profile,
   return new Browser(browser_params);
 }
 
-void SetWebAppPrefsForWebContents(content::WebContents* web_contents) {
-  web_contents->GetMutableRendererPrefs()->can_accept_load_drops = false;
-  web_contents->SyncRendererPrefs();
-  web_contents->NotifyPreferencesChanged();
-}
-
-content::WebContents* ShowWebApplicationWindow(
-    const apps::AppLaunchParams& params,
-    const GURL& default_url,
-    Browser* browser,
-    WindowOpenDisposition disposition) {
-  web_app::FileHandlerManager& file_handler_manager =
-      web_app::WebAppProviderBase::GetProviderBase(browser->profile())
-          ->file_handler_manager();
-  const GURL url =
-      file_handler_manager
-          .GetMatchingFileHandlerURL(params.app_id, params.launch_files)
-          .value_or(default_url);
-
-  NavigateParams nav_params(browser, url, ui::PAGE_TRANSITION_AUTO_BOOKMARK);
-  nav_params.disposition = disposition;
-  Navigate(&nav_params);
-
-  content::WebContents* web_contents =
-      nav_params.navigated_or_inserted_contents;
-
-  SetWebAppPrefsForWebContents(web_contents);
-
-  WebAppTabHelper* tab_helper = WebAppTabHelper::FromWebContents(web_contents);
-  DCHECK(tab_helper);
-  tab_helper->SetAppId(params.app_id);
-
-  browser->window()->Show();
-  web_contents->SetInitialFocus();
-
-  if (base::FeatureList::IsEnabled(blink::features::kFileHandlingAPI)) {
-    web_launch::WebLaunchFilesHelper::SetLaunchPaths(web_contents, url,
-                                                     params.launch_files);
-  }
-
-  return web_contents;
-}
-
 }  // namespace
 
 WebAppLaunchManager::WebAppLaunchManager(Profile* profile)
@@ -107,9 +64,15 @@ content::WebContents* WebAppLaunchManager::OpenApplication(
   if (params.container == apps::mojom::LaunchContainer::kLaunchContainerWindow)
     RecordAppWindowLaunch(profile(), params.app_id);
 
-  const GURL url = params.override_url.is_empty()
-                       ? provider_->registrar().GetAppLaunchURL(params.app_id)
-                       : params.override_url;
+  web_app::FileHandlerManager& file_handler_manager =
+      provider_->file_handler_manager();
+
+  const GURL url =
+      params.override_url.is_empty()
+          ? file_handler_manager
+                .GetMatchingFileHandlerURL(params.app_id, params.launch_files)
+                .value_or(provider_->registrar().GetAppLaunchURL(params.app_id))
+          : params.override_url;
 
   // System Web Apps go through their own launch path.
   base::Optional<SystemAppType> system_app_type =
@@ -122,8 +85,26 @@ content::WebContents* WebAppLaunchManager::OpenApplication(
 
   Browser* browser = CreateWebApplicationWindow(profile(), params.app_id);
 
-  content::WebContents* web_contents = ShowWebApplicationWindow(
-      params, url, browser, WindowOpenDisposition::NEW_FOREGROUND_TAB);
+  NavigateParams nav_params(browser, url, ui::PAGE_TRANSITION_AUTO_BOOKMARK);
+  nav_params.disposition = WindowOpenDisposition::NEW_FOREGROUND_TAB;
+  Navigate(&nav_params);
+
+  content::WebContents* web_contents =
+      nav_params.navigated_or_inserted_contents;
+
+  // TODO(https://crbug.com/1032443):
+  // Eventually move this to browser_navigator.cc:
+  // CreateTargetContents().
+  WebAppTabHelper* tab_helper = WebAppTabHelper::FromWebContents(web_contents);
+  DCHECK(tab_helper);
+  tab_helper->SetAppId(params.app_id);
+
+  if (base::FeatureList::IsEnabled(blink::features::kFileHandlingAPI)) {
+    web_launch::WebLaunchFilesHelper::SetLaunchPaths(web_contents, url,
+                                                     params.launch_files);
+  }
+
+  browser->window()->Show();
 
   // TODO(crbug.com/1014328): Populate WebApp metrics instead of Extensions.
 

chrome/browser/web_applications/extensions/system_web_app_manager_browsertest.cc

@@ -239,6 +239,22 @@ content::WebContents* SystemWebAppManagerBrowserTest::LaunchApp(
       ->OpenApplication(params);
 }
 
+content::EvalJsResult EvalJs(content::WebContents* web_contents,
+                             const std::string& script) {
+  // Set world_id = 1 to bypass Content Security Policy restriction.
+  return content::EvalJs(web_contents, script,
+                         content::EXECUTE_SCRIPT_DEFAULT_OPTIONS,
+                         1 /*world_id*/);
+}
+
+::testing::AssertionResult ExecJs(content::WebContents* web_contents,
+                                  const std::string& script) {
+  // Set world_id = 1 to bypass Content Security Policy restriction.
+  return content::ExecJs(web_contents, script,
+                         content::EXECUTE_SCRIPT_DEFAULT_OPTIONS,
+                         1 /*world_id*/);
+}
+
 // Test that System Apps install correctly with a manifest.
 IN_PROC_BROWSER_TEST_F(SystemWebAppManagerBrowserTest, Install) {
   const extensions::Extension* app = GetExtensionForAppBrowser(
@@ -299,30 +315,57 @@ IN_PROC_BROWSER_TEST_F(SystemWebAppManagerBrowserTest,
   ASSERT_TRUE(base::CreateTemporaryFileInDir(temp_directory.GetPath(),
                                              &temp_file_path));
 
-  params.launch_files = {temp_file_path};
-
   const GURL& launch_url = WebAppProvider::Get(browser()->profile())
                                ->registrar()
                                .GetAppLaunchURL(params.app_id);
+
+  // First launch.
+  params.launch_files = {temp_file_path};
   content::TestNavigationObserver navigation_observer(launch_url);
   navigation_observer.StartWatchingNewWebContents();
-
   content::WebContents* web_contents =
       OpenApplication(browser()->profile(), params);
-
   navigation_observer.Wait();
 
-  auto result =
-      content::EvalJs(web_contents,
-                      "new Promise(resolve => {"
-                      "  launchQueue.setConsumer(launchParams => {"
-                      "    resolve(launchParams.files.length);"
-                      "  });"
-                      "});",
-                      content::EXECUTE_SCRIPT_DEFAULT_OPTIONS, 1 /*world_id*/);
-
-  // Files array should be populated with one file.
-  EXPECT_EQ(1, result.value.GetInt());
+  // Set up a Promise that resolves to launchParams, when launchQueue's consumer
+  // callback is called.
+  EXPECT_TRUE(ExecJs(web_contents,
+                     "window.launchParamsPromise = new Promise(resolve => {"
+                     "  window.resolveLaunchParamsPromise = resolve;"
+                     "});"
+                     "launchQueue.setConsumer(launchParams => {"
+                     "  window.resolveLaunchParamsPromise(launchParams);"
+                     "});"));
+
+  // Check launch files are correct.
+  EXPECT_EQ(temp_file_path.BaseName().AsUTF8Unsafe(),
+            EvalJs(web_contents,
+                   "window.launchParamsPromise.then("
+                   "  launchParams => launchParams.files[0].name);"));
+
+  // Reset the Promise to get second launchParams.
+  EXPECT_TRUE(ExecJs(web_contents,
+                     "window.launchParamsPromise = new Promise(resolve => {"
+                     "  window.resolveLaunchParamsPromise = resolve;"
+                     "});"));
+
+  // Second Launch.
+  base::FilePath temp_file_path2;
+  ASSERT_TRUE(base::CreateTemporaryFileInDir(temp_directory.GetPath(),
+                                             &temp_file_path2));
+  params.launch_files = {temp_file_path2};
+  content::WebContents* web_contents2 =
+      OpenApplication(browser()->profile(), params);
+
+  // WebContents* should be the same because we are passing launchParams to the
+  // opened application.
+  EXPECT_EQ(web_contents, web_contents2);
+
+  // Second launch_files are passed to the opened application.
+  EXPECT_EQ(temp_file_path2.BaseName().AsUTF8Unsafe(),
+            EvalJs(web_contents,
+                   "window.launchParamsPromise.then("
+                   "  launchParams => launchParams.files[0].name)"));
 }
 
 }  // namespace web_app