file_manager: plugin_vm: remove shared directory logic from front end

Bug: 1077160
Bug: 1049453
Change-Id: I231edda50a25fbc5b5ae4c9238e07a10d8a41548
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2206868Reviewed-by: Timothy Loh <timloh@chromium.org>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Commit-Queue: Julian Watson <juwa@google.com>
Cr-Commit-Position: refs/heads/master@{#772091}

chrome/browser/chromeos/file_manager/guest_os_file_tasks.cc

@@ -105,18 +105,6 @@ void OnAppIconsLoaded(
   std::move(completion_closure).Run();
 }
 
-void OnTaskComplete(FileTaskFinishedCallback done,
-                    bool success,
-                    const std::string& failure_reason) {
-  if (!success) {
-    LOG(ERROR) << "Crostini task error: " << failure_reason;
-  }
-  std::move(done).Run(
-      success ? extensions::api::file_manager_private::TASK_RESULT_MESSAGE_SENT
-              : extensions::api::file_manager_private::TASK_RESULT_FAILED,
-      failure_reason);
-}
-
 bool HasSupportedMimeType(
     const std::set<std::string>& supported_mime_types,
     const std::string& vm_name,
@@ -197,6 +185,19 @@ bool AppSupportsAllEntries(
   }
 }
 
+auto ConvertLaunchPluginVmAppResultToTaskResult(
+    plugin_vm::LaunchPluginVmAppResult result) {
+  namespace fmp = extensions::api::file_manager_private;
+  switch (result) {
+    case plugin_vm::LaunchPluginVmAppResult::SUCCESS:
+      return fmp::TASK_RESULT_MESSAGE_SENT;
+    case plugin_vm::LaunchPluginVmAppResult::FAILED_DIRECTORY_NOT_SHARED:
+      return fmp::TASK_RESULT_FAILED_PLUGIN_VM_TASK_DIRECTORY_NOT_SHARED;
+    case plugin_vm::LaunchPluginVmAppResult::FAILED:
+      return fmp::TASK_RESULT_FAILED;
+  }
+}
+
 }  // namespace
 
 void FindGuestOsApps(
@@ -276,14 +277,38 @@ void ExecuteGuestOsTask(
       DCHECK(crostini::CrostiniFeatures::Get()->IsUIAllowed(profile));
       crostini::LaunchCrostiniApp(
           profile, task.app_id, display::kInvalidDisplayId, file_system_urls,
-          base::BindOnce(OnTaskComplete, std::move(done)));
+          base::BindOnce(
+              [](FileTaskFinishedCallback done, bool success,
+                 const std::string& failure_reason) {
+                if (!success) {
+                  LOG(ERROR) << "Crostini task error: " << failure_reason;
+                }
+                std::move(done).Run(
+                    success ? extensions::api::file_manager_private::
+                                  TASK_RESULT_MESSAGE_SENT
+                            : extensions::api::file_manager_private::
+                                  TASK_RESULT_FAILED,
+                    failure_reason);
+              },
+              std::move(done)));
       return;
     case guest_os::GuestOsRegistryService::VmType::
         ApplicationList_VmType_PLUGIN_VM:
       DCHECK(plugin_vm::IsPluginVmEnabled(profile));
       plugin_vm::LaunchPluginVmApp(
           profile, task.app_id, file_system_urls,
-          base::BindOnce(OnTaskComplete, std::move(done)));
+          base::BindOnce(
+              [](FileTaskFinishedCallback done,
+                 plugin_vm::LaunchPluginVmAppResult result,
+                 const std::string& failure_reason) {
+                if (result != plugin_vm::LaunchPluginVmAppResult::SUCCESS) {
+                  LOG(ERROR) << "Plugin VM task error: " << failure_reason;
+                }
+                std::move(done).Run(
+                    ConvertLaunchPluginVmAppResultToTaskResult(result),
+                    failure_reason);
+              },
+              std::move(done)));
       return;
     default:
       std::move(done).Run(

chrome/browser/chromeos/plugin_vm/plugin_vm_files.cc

@@ -82,7 +82,8 @@ void LaunchPluginVmAppImpl(Profile* profile,
                            LaunchPluginVmAppCallback callback,
                            bool plugin_vm_is_running) {
   if (!plugin_vm_is_running) {
-    return std::move(callback).Run(false, "Plugin VM could not be started");
+    return std::move(callback).Run(LaunchPluginVmAppResult::FAILED,
+                                   "Plugin VM could not be started");
   }
 
   auto* registry_service =
@@ -90,7 +91,8 @@ void LaunchPluginVmAppImpl(Profile* profile,
   auto registration = registry_service->GetRegistration(app_id);
   if (!registration) {
     return std::move(callback).Run(
-        false, "LaunchPluginVmApp called with an unknown app_id: " + app_id);
+        LaunchPluginVmAppResult::FAILED,
+        "LaunchPluginVmApp called with an unknown app_id: " + app_id);
   }
 
   vm_tools::cicerone::LaunchContainerApplicationRequest request;
@@ -117,14 +119,14 @@ void LaunchPluginVmAppImpl(Profile* profile,
                   LOG(ERROR) << "Failed to launch application. "
                              << (response ? response->failure_reason()
                                           : "Empty response.");
-                  std::move(callback).Run(/*success=*/false,
+                  std::move(callback).Run(LaunchPluginVmAppResult::FAILED,
                                           "Failed to launch " + app_id);
                   return;
                 }
 
                 FocusAllPluginVmWindows();
 
-                std::move(callback).Run(/*success=*/true, "");
+                std::move(callback).Run(LaunchPluginVmAppResult::SUCCESS, "");
               },
               std::move(app_id), std::move(callback)));
 }
@@ -174,14 +176,15 @@ void LaunchPluginVmApp(Profile* profile,
                        const std::vector<storage::FileSystemURL>& files,
                        LaunchPluginVmAppCallback callback) {
   if (!plugin_vm::IsPluginVmEnabled(profile)) {
-    return std::move(callback).Run(false,
+    return std::move(callback).Run(LaunchPluginVmAppResult::FAILED,
                                    "Plugin VM is not enabled for this profile");
   }
 
   auto* manager = PluginVmManagerFactory::GetForProfile(profile);
 
   if (!manager) {
-    return std::move(callback).Run(false, "Could not get PluginVmManager");
+    return std::move(callback).Run(LaunchPluginVmAppResult::FAILED,
+                                   "Could not get PluginVmManager");
   }
 
   std::vector<std::string> file_paths;
@@ -191,7 +194,8 @@ void LaunchPluginVmApp(Profile* profile,
         ConvertFileSystemURLToPathInsidePluginVmSharedDir(profile, file);
     if (!file_path) {
       return std::move(callback).Run(
-          false, "Only files in the shared dir are supported. Got: " +
+          LaunchPluginVmAppResult::FAILED_DIRECTORY_NOT_SHARED,
+          "Only files in the shared dir are supported. Got: " +
               file.DebugString());
     }
     file_paths.push_back(std::move(*file_path));

chrome/browser/chromeos/plugin_vm/plugin_vm_files.h

@@ -26,8 +26,15 @@ base::Optional<std::string> ConvertFileSystemURLToPathInsidePluginVmSharedDir(
     Profile* profile,
     const storage::FileSystemURL& file_system_url);
 
+enum class LaunchPluginVmAppResult {
+  SUCCESS,
+  FAILED,
+  FAILED_DIRECTORY_NOT_SHARED,
+};
+
 using LaunchPluginVmAppCallback =
-    base::OnceCallback<void(bool success, const std::string& failure_reason)>;
+    base::OnceCallback<void(LaunchPluginVmAppResult result,
+                            const std::string& failure_reason)>;
 
 // Launch a Plugin VM App with a given set of files, given as cracked urls in
 // the VM. Will start Plugin VM if it is not already running.

chrome/browser/chromeos/plugin_vm/plugin_vm_files_unittest.cc

@@ -162,7 +162,7 @@ TEST_F(PluginVmFilesTest, LaunchPluginVmApp) {
   vm_tools::cicerone::LaunchContainerApplicationResponse response;
   response.set_success(true);
   EXPECT_CALL(mock_window, Activate());
-  EXPECT_CALL(app_launched_callback, Run(true, ""));
+  EXPECT_CALL(app_launched_callback, Run(LaunchPluginVmAppResult::SUCCESS, ""));
   std::move(cicerone_response_callback).Run(std::move(response));
 }
 

chrome/common/extensions/api/file_manager_private.idl

@@ -146,7 +146,8 @@ enum DriveSyncErrorType {
   misc
 };
 
-// Result of task execution.
+// 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.
   opened,
@@ -156,7 +157,9 @@ enum TaskResult {
   // The task execution failed.
   failed,
   // No URL is specified.
-  empty
+  empty,
+  // The task was a |plugin_vm| task, and the file was in a unshared directory
+  failed_plugin_vm_task_directory_not_shared
 };
 
 // Drive share type.

third_party/closure_compiler/externs/file_manager_private.js

@@ -176,6 +176,7 @@ chrome.fileManagerPrivate.TaskResult = {
   MESSAGE_SENT: 'message_sent',
   FAILED: 'failed',
   EMPTY: 'empty',
+  FAILED_PLUGIN_VM_TASK_DIRECTORY_NOT_SHARED: 'failed_plugin_vm_task_directory_not_shared',
 };
 
 /** @enum {string} */

ui/file_manager/file_manager/background/js/test_util.js

@@ -399,8 +399,8 @@ test.util.sync.overrideTasks = (contentWindow, taskList) => {
     }, 0);
   };
 
-  const executeTask = (taskId, entry) => {
-    test.util.executedTasks_.push(taskId);
+  const executeTask = (taskId, entries, callback) => {
+    test.util.executedTasks_.push({taskId, entries, callback});
   };
 
   const setDefaultTask = taskId => {
@@ -426,7 +426,26 @@ test.util.sync.getExecutedTasks = contentWindow => {
     console.error('Please call overrideTasks() first.');
     return null;
   }
-  return test.util.executedTasks_;
+  return test.util.executedTasks_.map(task => task.taskId);
+};
+
+/**
+ * Invokes an executed task with |responseArgs|.
+ * @param {Window} contentWindow Window to be tested.
+ * @param {string} taskId the task to be replied to.
+ * @param {Array<Object>} responseArgs the arguments to inoke the callback with.
+ */
+test.util.sync.replyExecutedTask = (contentWindow, taskId, responseArgs) => {
+  if (!test.util.executedTasks_) {
+    console.error('Please call overrideTasks() first.');
+    return null;
+  }
+  const found = test.util.executedTasks_.find(task => task.taskId === taskId);
+  if (!found) {
+    console.error(`No task with id ${taskId}`);
+    return null;
+  }
+  found.callback(...responseArgs);
 };
 
 /**

ui/file_manager/file_manager/foreground/js/file_tasks.js

@@ -365,25 +365,6 @@ class FileTasks {
     return task.verb === chrome.fileManagerPrivate.Verb.SHARE_WITH;
   }
 
-  /**
-   * @param {string} taskId Task identifier.
-   * @return {boolean} True if the task ID is for Plugin VM.
-   * @private
-   */
-  static isPluginVmTask_(taskId) {
-    return taskId.split('|')[1] === 'pluginvm';
-  }
-
-
-  /**
-   *  @param {!Entry} entry
-   *  @return {boolean} True if entry is in the Plugin VM shared folder.
-   *  @private
-   */
-  static entryInPluginVmSharedFolder_(entry) {
-    return entry.fullPath.startsWith('/PvmDefault/');
-  }
-
   /**
    * Annotates tasks returned from the API.
    *
@@ -686,19 +667,6 @@ class FileTasks {
       this.ui_.speakA11yMessage(msg);
       if (FileTasks.isInternalTask_(task.taskId)) {
         this.executeInternalTask_(task.taskId);
-      } else if (
-          // TODO(crbug.com/1077160): Remove this logic from the front end and
-          // instead show the dialog based on the response of
-          // fileManagerPrivate.executeTask.
-          FileTasks.isPluginVmTask_(task.taskId) &&
-          !this.entries_.every(FileTasks.entryInPluginVmSharedFolder_)) {
-        this.ui_.alertDialog.showHtml(
-            strf(
-                'UNABLE_TO_OPEN_WITH_PLUGIN_VM_TITLE',
-                strf('PLUGIN_VM_APP_NAME')),
-            strf(
-                'UNABLE_TO_OPEN_WITH_PLUGIN_VM_MESSAGE',
-                strf('PLUGIN_VM_APP_NAME'), strf('PLUGIN_VM_DIRECTORY_LABEL')));
       } else {
         FileTasks.recordZipHandlerUMA_(task.taskId);
         chrome.fileManagerPrivate.executeTask(
@@ -709,14 +677,26 @@ class FileTasks {
                     chrome.runtime.lastError.message);
                 return;
               }
-              if (result !== 'message_sent') {
-                return;
-              }
+              switch (result) {
+                case chrome.fileManagerPrivate.TaskResult.MESSAGE_SENT:
                   util.isTeleported(window).then((teleported) => {
                     if (teleported) {
                       this.ui_.showOpenInOtherDesktopAlert(this.entries_);
                     }
                   });
+                  break;
+                case chrome.fileManagerPrivate.TaskResult
+                    .FAILED_PLUGIN_VM_TASK_DIRECTORY_NOT_SHARED:
+                  this.ui_.alertDialog.showHtml(
+                      strf(
+                          'UNABLE_TO_OPEN_WITH_PLUGIN_VM_TITLE',
+                          strf('PLUGIN_VM_APP_NAME')),
+                      strf(
+                          'UNABLE_TO_OPEN_WITH_PLUGIN_VM_MESSAGE',
+                          strf('PLUGIN_VM_APP_NAME'),
+                          strf('PLUGIN_VM_DIRECTORY_LABEL')));
+                  break;
+              }
             });
       }
     });

ui/file_manager/file_manager/foreground/js/file_tasks_unittest.js

@@ -85,6 +85,10 @@ function setUp() {
       Verb: {
         SHARE_WITH: 'share_with',
       },
+      TaskResult: {
+        MESSAGE_SENT: 'test_ms_task',
+        FAILED_PLUGIN_VM_TASK_DIRECTORY_NOT_SHARED: 'test_fpvtdns_task',
+      },
       getFileTasks: function(entries, callback) {
         setTimeout(callback.bind(null, [mockTask]), 0);
       },

ui/file_manager/integration_tests/file_manager/crostini.js

@@ -114,6 +114,9 @@ testcase.pluginVmErrorDialog = async () => {
     `#default-tasks-list [tabindex]:nth-of-type(${
         appOptions.map(el => el.text).indexOf('Open with Plugin VM App') + 1})`
   ]);
+  await remoteCall.waitUntilTaskExecutes(
+      appId, 'plugin-vm-app-id|pluginvm|open-with',
+      ['failed_plugin_vm_task_directory_not_shared']);
   await remoteCall.waitForElement(appId, '.files-alert-dialog:not([hidden])');
 
   // Validate error messages.

ui/file_manager/integration_tests/remote_call.js

@@ -496,10 +496,11 @@ class RemoteCallFilesApp extends RemoteCall {
    * Waits until the given taskId appears in the executed task list.
    * @param {string} appId App window Id.
    * @param {string} taskId Task ID to watch.
+   * @param {Array<Object>} opt_replyArgs arguments to reply to executed task.
    * @return {Promise} Promise to be fulfilled when the task appears in the
    *     executed task list.
    */
-  waitUntilTaskExecutes(appId, taskId) {
+  waitUntilTaskExecutes(appId, taskId, opt_replyArgs) {
     const caller = getCaller();
     return repeatUntil(async () => {
       const executedTasks =
@@ -507,6 +508,10 @@ class RemoteCallFilesApp extends RemoteCall {
       if (executedTasks.indexOf(taskId) === -1) {
         return pending(caller, 'Executed task is %j', executedTasks);
       }
+      if (opt_replyArgs) {
+        await this.callRemoteTestUtil(
+            'replyExecutedTask', appId, [taskId, opt_replyArgs]);
+      }
     });
   }