dPWA: Enable file handling "multilaunch" on Lacros.

Bug: 1304003
Change-Id: I160248f2e7029ccff307486b38317f76632fdbac
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3553819
Commit-Queue: Evan Stade <estade@chromium.org>
Reviewed-by: Nancy Wang <nancylingwang@chromium.org>
Reviewed-by: Peter Marshall <petermarshall@chromium.org>
Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Reviewed-by: Erik Chen <erikchen@chromium.org>
Cr-Commit-Position: refs/heads/main@{#988691}

chrome/browser/apps/app_service/app_service_proxy_ash.cc

@@ -387,14 +387,16 @@ bool AppServiceProxyAsh::MaybeShowLaunchPreventionDialog(
 
 void AppServiceProxyAsh::OnLaunched(LaunchCallback callback,
                                     LaunchResult&& launch_result) {
-  bool exists = false;
-  InstanceRegistry().ForOneInstance(
-      launch_result.instance_id,
-      [&exists](const apps::InstanceUpdate& update) { exists = true; });
-  if (exists) {
-    std::move(callback).Run(std::move(launch_result));
+  base::RepeatingCallback<bool(void)> ready_to_run_callback =
+      base::BindRepeating(&AppServiceProxyAsh::CanRunLaunchCallback,
+                          base::Unretained(this), launch_result.instance_ids);
+  base::OnceClosure launch_callback =
+      base::BindOnce(std::move(callback), std::move(launch_result));
+  if (ready_to_run_callback.Run()) {
+    std::move(launch_callback).Run();
   } else {
-    callback_list_[launch_result.instance_id].push_back(std::move(callback));
+    callback_list_.emplace_back(
+        std::make_pair(ready_to_run_callback, std::move(launch_callback)));
   }
 }
 
@@ -547,17 +549,33 @@ void AppServiceProxyAsh::OnInstanceUpdate(const apps::InstanceUpdate& update) {
     return;
   }
 
-  for (auto& callback : callback_list_[update.InstanceId()]) {
-    auto launch_result = LaunchResult();
-    launch_result.instance_id = update.InstanceId();
-    std::move(callback).Run(std::move(launch_result));
-  }
-  callback_list_.erase(update.InstanceId());
+  callback_list_.remove_if([](std::pair<base::RepeatingCallback<bool(void)>,
+                                        base::OnceClosure>& callbacks) {
+    if (callbacks.first.Run()) {
+      std::move(callbacks.second).Run();
+      return true;
+    }
+    return false;
+  });
 }
 
 void AppServiceProxyAsh::OnInstanceRegistryWillBeDestroyed(
     apps::InstanceRegistry* cache) {
   instance_registry_observer_.Reset();
 }
 
+bool AppServiceProxyAsh::CanRunLaunchCallback(
+    const std::vector<base::UnguessableToken>& instance_ids) {
+  for (const base::UnguessableToken& instance_id : instance_ids) {
+    bool exists = false;
+    InstanceRegistry().ForOneInstance(
+        instance_id,
+        [&exists](const apps::InstanceUpdate& update) { exists = true; });
+    if (!exists)
+      return false;
+  }
+
+  return true;
+}
+
 }  // namespace apps

chrome/browser/apps/app_service/app_service_proxy_ash.h

@@ -206,6 +206,10 @@ class AppServiceProxyAsh : public AppServiceProxyBase,
   void OnInstanceRegistryWillBeDestroyed(
       apps::InstanceRegistry* cache) override;
 
+  // Checks if all instance IDs correspond to existing windows.
+  bool CanRunLaunchCallback(
+      const std::vector<base::UnguessableToken>& instance_ids);
+
   SubscriberCrosapi* crosapi_subscriber_ = nullptr;
 
   std::unique_ptr<PublisherHost> publisher_host_;
@@ -241,10 +245,12 @@ class AppServiceProxyAsh : public AppServiceProxyBase,
                           apps::InstanceRegistry::Observer>
       instance_registry_observer_{this};
 
-  // A map to record the launch callbacks for an app instance. Note it is
-  // possible to have multiple launches to launch the same app instance for
-  // web apps, so we record a list of launch callbacks for each instance.
-  std::map<base::UnguessableToken, std::vector<LaunchCallback>> callback_list_;
+  // A list to record outstanding launch callbacks. When the first member
+  // returns true, the second member should be run and the pair can be removed
+  // from the outstanding callback queue.
+  std::list<std::pair<base::RepeatingCallback<bool(void)>, base::OnceClosure>>
+      callback_list_;
+
   base::WeakPtrFactory<AppServiceProxyAsh> weak_ptr_factory_{this};
 };
 

chrome/browser/apps/app_service/app_service_proxy_unittest.cc

@@ -515,10 +515,10 @@ TEST_F(AppServiceProxyTest, LaunchCallback) {
   bool called_2 = false;
   auto instance_id_1 = base::UnguessableToken::Create();
   LaunchResult result_1;
-  result_1.instance_id = instance_id_1;
+  result_1.instance_ids.push_back(instance_id_1);
   auto instance_id_2 = base::UnguessableToken::Create();
   LaunchResult result_2;
-  result_2.instance_id = instance_id_2;
+  result_2.instance_ids.push_back(instance_id_2);
 
   // If the instance is not created yet, the callback will be stored.
   proxy.OnLaunched(
@@ -554,6 +554,28 @@ TEST_F(AppServiceProxyTest, LaunchCallback) {
   EXPECT_EQ(proxy.callback_list_.size(), 1U);
   EXPECT_TRUE(called_1);
   EXPECT_FALSE(called_2);
+
+  // A launch that results in multiple instances.
+  LaunchResult result_multi;
+  auto instance_id_3 = base::UnguessableToken::Create();
+  auto instance_id_4 = base::UnguessableToken::Create();
+  result_multi.instance_ids.push_back(instance_id_3);
+  result_multi.instance_ids.push_back(instance_id_4);
+  bool called_multi = false;
+  proxy.OnLaunched(
+      base::BindOnce([](bool* called,
+                        apps::LaunchResult&& launch_result) { *called = true; },
+                     &called_multi),
+      std::move(result_multi));
+  EXPECT_EQ(proxy.callback_list_.size(), 2U);
+  EXPECT_FALSE(called_multi);
+  proxy.InstanceRegistry().OnInstance(
+      std::make_unique<apps::Instance>("foo", instance_id_3, nullptr));
+  proxy.InstanceRegistry().OnInstance(
+      std::make_unique<apps::Instance>("bar", instance_id_4, nullptr));
+  EXPECT_EQ(proxy.callback_list_.size(), 1U);
+
+  EXPECT_TRUE(called_multi);
 }
 #endif  // BUILDFLAG(IS_CHROMEOS_ASH)
 }  // namespace apps

chrome/browser/apps/app_service/launch_result_type.cc

@@ -12,11 +12,22 @@
 
 namespace apps {
 
+LaunchResult::LaunchResult() = default;
+LaunchResult::~LaunchResult() = default;
+
+LaunchResult::LaunchResult(LaunchResult&& other) = default;
+
 #if BUILDFLAG(IS_CHROMEOS)
 LaunchResult ConvertMojomLaunchResultToLaunchResult(
     crosapi::mojom::LaunchResultPtr mojom_launch_result) {
   auto launch_result = LaunchResult();
-  launch_result.instance_id = std::move(mojom_launch_result->instance_id);
+  if (mojom_launch_result->instance_ids) {
+    for (auto token : *mojom_launch_result->instance_ids)
+      launch_result.instance_ids.push_back(std::move(token));
+  } else {
+    launch_result.instance_ids.push_back(
+        std::move(mojom_launch_result->instance_id));
+  }
   return launch_result;
 }
 

chrome/browser/apps/app_service/launch_result_type.h

@@ -5,6 +5,8 @@
 #ifndef CHROME_BROWSER_APPS_APP_SERVICE_LAUNCH_RESULT_TYPE_H_
 #define CHROME_BROWSER_APPS_APP_SERVICE_LAUNCH_RESULT_TYPE_H_
 
+#include <vector>
+
 #include "base/callback_forward.h"
 #include "base/unguessable_token.h"
 #include "build/build_config.h"
@@ -18,7 +20,15 @@ namespace apps {
 // desktop platforms. So this struct can't be moved to AppPublisher.
 
 struct LaunchResult {
-  base::UnguessableToken instance_id;
+  LaunchResult();
+  ~LaunchResult();
+
+  LaunchResult(LaunchResult&& launch_result);
+  LaunchResult& operator=(const LaunchResult& launch_result) = delete;
+  LaunchResult(const LaunchResult& launch_result) = delete;
+
+  // A single launch event may result in multiple app instances being created.
+  std::vector<base::UnguessableToken> instance_ids;
 };
 
 using LaunchCallback = base::OnceCallback<void(LaunchResult&&)>;

chrome/browser/web_applications/app_service/lacros_web_apps_controller.cc

@@ -185,7 +185,7 @@ void LacrosWebAppsController::ExecuteContextMenuCommand(
   auto* web_contents = publisher_helper().ExecuteContextMenuCommand(
       app_id, id, display::kDefaultDisplayId);
 
-  ReturnLaunchResult(std::move(callback), web_contents);
+  ReturnLaunchResults(std::move(callback), {web_contents});
 }
 
 void LacrosWebAppsController::StopApp(const std::string& app_id) {
@@ -206,14 +206,14 @@ void LacrosWebAppsController::Launch(
   content::WebContents* web_contents = nullptr;
   if (launch_params->intent) {
     if (!profile_) {
-      ReturnLaunchResult(std::move(callback), nullptr);
+      ReturnLaunchResults(std::move(callback), {});
       return;
     }
 
     web_contents = publisher_helper().MaybeNavigateExistingWindow(
         launch_params->app_id, launch_params->intent->url);
     if (web_contents) {
-      ReturnLaunchResult(std::move(callback), web_contents);
+      ReturnLaunchResults(std::move(callback), {web_contents});
       return;
     }
   }
@@ -232,40 +232,33 @@ void LacrosWebAppsController::Launch(
 
   web_contents = publisher_helper().LaunchAppWithParams(std::move(params));
 
-  ReturnLaunchResult(std::move(callback), web_contents);
+  ReturnLaunchResults(std::move(callback), {web_contents});
 }
 
-void LacrosWebAppsController::ReturnLaunchResult(
+void LacrosWebAppsController::ReturnLaunchResults(
     LaunchCallback callback,
-    content::WebContents* web_contents) {
-  // TODO(crbug.com/1144877): Run callback when the window is ready.
+    const std::vector<content::WebContents*>& web_contentses) {
   auto* app_instance_tracker =
       apps::AppServiceProxyFactory::GetForProfile(profile_)
           ->BrowserAppInstanceTracker();
   auto launch_result = crosapi::mojom::LaunchResult::New();
+  launch_result->instance_id = base::UnguessableToken::Create();
+  launch_result->instance_ids = std::vector<base::UnguessableToken>();
+
+  // TODO(crbug.com/1144877): Replaced with DCHECK when the app instance tracker
+  // flag is turned on.
   if (app_instance_tracker) {
-    const apps::BrowserAppInstance* app_instance =
-        app_instance_tracker->GetAppInstance(web_contents);
-    launch_result->instance_id =
-        app_instance ? app_instance->id : base::UnguessableToken::Create();
-  } else {
-    // TODO(crbug.com/1144877): This part of code should not be reached
-    // after the instance tracker flag is turn on. Replaced with DCHECK when
-    // the app instance tracker flag is turned on.
-    launch_result->instance_id = base::UnguessableToken::Create();
+    for (content::WebContents* web_contents : web_contentses) {
+      const apps::BrowserAppInstance* app_instance =
+          app_instance_tracker->GetAppInstance(web_contents);
+      if (app_instance) {
+        launch_result->instance_ids->push_back(app_instance->id);
+      }
+    }
   }
   std::move(callback).Run(std::move(launch_result));
 }
 
-void LacrosWebAppsController::ReturnLaunchResults(
-    LaunchCallback callback,
-    const std::vector<content::WebContents*>& web_contentses) {
-  // TODO(crbug/1304003): update Lacros to support multilaunch.
-  DCHECK_LE(web_contentses.size(), 1U);
-  ReturnLaunchResult(std::move(callback),
-                     web_contentses.empty() ? nullptr : web_contentses[0]);
-}
-
 void LacrosWebAppsController::OnShortcutsMenuIconsRead(
     const std::string& app_id,
     crosapi::mojom::MenuItemsPtr menu_items,

chrome/browser/web_applications/app_service/lacros_web_apps_controller.h

@@ -106,11 +106,9 @@ class LacrosWebAppsController : public crosapi::mojom::AppController,
       absl::optional<bool> accessing_camera,
       absl::optional<bool> accessing_microphone) override;
 
-  void ReturnLaunchResult(LaunchCallback callback,
-                          content::WebContents* web_contents);
   void ReturnLaunchResults(
       LaunchCallback callback,
-      const std::vector<content::WebContents*>& web_contentses);
+      const std::vector<content::WebContents*>& web_contents);
 
   const WebApp* GetWebApp(const AppId& app_id) const;
 

chrome/browser/web_applications/app_service/web_app_publisher_helper.cc

@@ -1821,11 +1821,6 @@ void WebAppPublisherHelper::OnFileHandlerDialogCompleted(
   // system apps.
   const WebApp* web_app = GetWebApp(params.app_id);
   bool can_multilaunch = !(web_app && web_app->IsSystemApp());
-#if BUILDFLAG(IS_CHROMEOS_LACROS)
-  // Lacros also sticks to old behavior for now. TODO(crbug/1304003): update
-  // Lacros to support multilaunch.
-  can_multilaunch = false;
-#endif
   std::vector<content::WebContents*> web_contentses;
   if (can_multilaunch) {
     WebAppFileHandlerManager::LaunchInfos file_launch_infos =
@@ -1844,8 +1839,10 @@ void WebAppPublisherHelper::OnFileHandlerDialogCompleted(
     apps::AppLaunchParams params_for_file_launch(
         app_id, params.container, params.disposition, params.launch_source,
         params.display_id, params.launch_files, params.intent);
-    // For now, with Lacros, the URL is calculated by the file browser and
-    // passed in the intent.
+    // For system web apps, the URL is calculated by the file browser and passed
+    // in the intent.
+    // TODO(crbug.com/1264164): remove this check. It's only here to support
+    // tests that haven't been updated.
     if (params.intent) {
       params_for_file_launch.override_url = GURL(*params.intent->activity_name);
     }

chromeos/crosapi/mojom/app_service_types.mojom

@@ -2,7 +2,10 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
-// Next MinVersion: 19
+// NOTE: Future MinVersion values should take into account that MinVersion is
+// scoped per struct. In the past, some of the MinVersion values in this file
+// were set based on the understanding that the entire file should share a
+// MinVersion counter.
 
 module crosapi.mojom;
 
@@ -346,8 +349,11 @@ struct Intent {
 // The result from launching an app.
 [Stable]
 struct LaunchResult {
-  // The id to represent which instance the app is launched in.
+  // The id to represent which instance the app is launched in. Deprecated: use
+  // `instance_ids`.
   mojo_base.mojom.UnguessableToken instance_id@0;
+  // The IDs that represent the instances the app is launched in.
+  [MinVersion=1] array<mojo_base.mojom.UnguessableToken>? instance_ids@1;
 };
 
 // Enumeration of app launch sources.