Stop showing message incorrectly when opening files on another desktop

In a multiprofile session, windows can be sent to other desktops. This
can be done to the Files app, and when files are opened when it is in
this state the apps may open on the original desktop (i.e. that of the
profile that owns the files). When this occurs an info message is
displayed as otherwise it appears to the user as though nothing
happened.

The files app shows this message for web apps even though they are
opened on the current desktop. This change stops the info message being
shown in this case.

This change also adds some clarifying comments and also adds some TODOs
for other situations that might have similar bugs.

Bug: 1109589
Change-Id: I9ceab68fc6547813d4e83f879e04adbcf07a1093
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2359870Reviewed-by: Alex Danilo <adanilo@chromium.org>
Commit-Queue: Ben Wells <benwells@chromium.org>
Cr-Commit-Position: refs/heads/master@{#806545}

chrome/browser/chromeos/file_manager/app_service_file_tasks.cc

@@ -137,6 +137,8 @@ void ExecuteAppServiceTask(
                           /*prefer_container=*/true),
       launch_source, file_urls, mime_types);
 
+  // TODO(benwells): return the correct code here, depending on how the app will
+  // be opened in multiprofile.
   std::move(done).Run(
       extensions::api::file_manager_private::TASK_RESULT_MESSAGE_SENT, "");
 }

chrome/browser/chromeos/file_manager/arc_file_tasks.cc

@@ -290,6 +290,8 @@ void ExecuteArcTaskAfterContentUrlsResolved(
       ConstructOpenUrlsRequest(task, content_urls, mime_types);
   arc_file_system->OpenUrlsWithPermission(std::move(request),
                                           base::DoNothing());
+  // TODO(benwells): return the correct code here, depending on how the app
+  // will be opened in multiprofile.
   std::move(done).Run(
       extensions::api::file_manager_private::TASK_RESULT_MESSAGE_SENT, "");
 

chrome/browser/chromeos/file_manager/external_filesystem_apitest.cc

@@ -764,7 +764,9 @@ std::string MediaAppBoolString(const testing::TestParamInfo<bool> info) {
 // gallery.
 IN_PROC_BROWSER_TEST_P(FileSystemExtensionApiTestWithApps, OpenGalleryForPng) {
   base::HistogramTester histogram_tester;
-  EXPECT_TRUE(RunBackgroundPageTestCase("open_gallery", "testPngOpensGallery"))
+  EXPECT_TRUE(RunBackgroundPageTestCase(
+      "open_gallery", MediaAppEnabled() ? "testPngOpensGalleryReturnsOpened"
+                                        : "testPngOpensGalleryReturnsMsgSent"))
       << message_;
   histogram_tester.ExpectBucketCount(kAppLaunchMetric, kGalleryUmaBucket,
                                      MediaAppEnabled() ? 0 : 1);

chrome/browser/chromeos/file_manager/file_browser_handlers.cc

@@ -300,6 +300,9 @@ void FileBrowserHandlerExecutor::ExecuteDoneOnUIThread(
     std::string failure_reason) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (done_) {
+    // In a multiprofile session, extension handlers will open on the desktop
+    // corresponding to the profile that owns the files, so return
+    // TASK_RESULT_MESSAGE_SENT.
     std::move(done_).Run(
         success
             ? extensions::api::file_manager_private::TASK_RESULT_MESSAGE_SENT

chrome/browser/chromeos/file_manager/file_tasks.cc

@@ -552,7 +552,9 @@ bool ExecuteFileTask(Profile* profile,
   }
 
   // Some action IDs of the file manager's file browser handlers require the
-  // files to be directly opened with the browser.
+  // files to be directly opened with the browser. In a multiprofile session
+  // this will always open on the current desktop, regardless of which profile
+  // owns the files, so return TASK_RESULT_OPENED.
   if (ShouldBeOpenedWithBrowser(task.app_id, task.action_id)) {
     const bool result =
         OpenFilesWithBrowser(profile, file_urls, task.action_id);
@@ -585,6 +587,9 @@ bool ExecuteFileTask(Profile* profile,
     DCHECK(!extension->from_bookmark());
     apps::LaunchPlatformAppWithFileHandler(extension_task_profile, extension,
                                            task.action_id, paths);
+    // In a multiprofile session, platform apps will open on the desktop
+    // corresponding to the profile that owns the files, so return
+    // TASK_RESULT_MESSAGE_SENT.
     if (!done.is_null())
       std::move(done).Run(
           extensions::api::file_manager_private::TASK_RESULT_MESSAGE_SENT, "");

chrome/browser/chromeos/file_manager/guest_os_file_tasks.cc

@@ -129,6 +129,8 @@ bool AppSupportsExtensionOfAllEntries(
 
 auto ConvertLaunchPluginVmAppResultToTaskResult(
     plugin_vm::LaunchPluginVmAppResult result) {
+  // TODO(benwells): return the correct code here, depending on how the app will
+  // be opened in multiprofile.
   namespace fmp = extensions::api::file_manager_private;
   switch (result) {
     case plugin_vm::LaunchPluginVmAppResult::SUCCESS:
@@ -294,6 +296,8 @@ void ExecuteGuestOsTask(
                   LOG(ERROR) << "Crostini task error: " << failure_reason;
                 }
                 std::move(done).Run(
+                    // TODO(benwells): return the correct code here, depending
+                    // on how the app will be opened in multiprofile.
                     success ? extensions::api::file_manager_private::
                                   TASK_RESULT_MESSAGE_SENT
                             : extensions::api::file_manager_private::

chrome/browser/chromeos/file_manager/web_file_tasks.cc

@@ -161,8 +161,11 @@ void ExecuteWebTask(Profile* profile,
                           /* preferred_containner=*/false),
       apps::mojom::LaunchSource::kFromFileManager, std::move(launch_files));
 
-  std::move(done).Run(
-      extensions::api::file_manager_private::TASK_RESULT_MESSAGE_SENT, "");
+  // In a multiprofile session, web apps always open on the current desktop,
+  // regardless of which profile owns the files being opened, so use
+  // TASK_RESULT_OPENED.
+  std::move(done).Run(extensions::api::file_manager_private::TASK_RESULT_OPENED,
+                      "");
 }
 
 }  // namespace file_tasks

chrome/common/extensions/api/file_manager_private.idl

@@ -152,10 +152,12 @@ enum DriveSyncErrorType {
 // Result of task execution. If changing, update the strings used in
 // ui/file_manager/file_manager/foreground/js/file_tasks.js
 enum TaskResult {
-  // The task execution succeeded and a new window/tab was opened.
+  // The task execution succeeded and a new window/tab was opened on the current
+  // desktop.
   opened,
   // The task execution succeeded and the message was sent to the proper
-  // extension.
+  // extension or app. This could result in a window being opened on a different
+  // desktop.
   message_sent,
   // The task execution failed.
   failed,

chrome/test/data/extensions/api_test/file_browser/open_gallery/test.js

@@ -36,20 +36,31 @@ function getFileEntry(volumeType, path) {
  * A method the camera app uses to open its "camera roll". Instrumented for
  * testing. See chromeos/camera/src/js/browser_proxy/browser_proxy.js.
  */
-function openGallery(entry) {
+function openGallery(entry, expectedResult) {
   // "jhdjimmaggjajfjphpljagpgkidjilnj" is the MediaApp app id. This task id is
   // hard-coded in the Camera component app.
   const id = 'jhdjimmaggjajfjphpljagpgkidjilnj|web|open';
   function taskCallback(taskResult) {
-    chrome.test.assertEq(
-        chrome.fileManagerPrivate.TaskResult.MESSAGE_SENT, taskResult);
+    chrome.test.assertEq(expectedResult, taskResult);
     chrome.test.succeed();
   }
   chrome.fileManagerPrivate.executeTask(id, [entry], taskCallback);
 }
 
-function testPngOpensGallery() {
-  getFileEntry('testing', kTestPng).then(openGallery);
+function openGalleryExpectOpened(entry) {
+  openGallery(entry, chrome.fileManagerPrivate.TaskResult.OPENED);
+}
+
+function openGalleryExpectMsgSent(entry) {
+  openGallery(entry, chrome.fileManagerPrivate.TaskResult.MESSAGE_SENT);
+}
+
+function testPngOpensGalleryReturnsOpened() {
+  getFileEntry('testing', kTestPng).then(openGalleryExpectOpened);
+}
+
+function testPngOpensGalleryReturnsMsgSent() {
+  getFileEntry('testing', kTestPng).then(openGalleryExpectMsgSent);
 }
 
 // Handle the case where JSTestStarter has already injected a test to run.