file_manager: Unify the file task selection logic

chrome/browser/chromeos/extensions/file_manager/open_util.cc

Index: chrome/browser/chromeos/extensions/file_manager/open_util.cc
diff --git a/chrome/browser/chromeos/extensions/file_manager/open_util.cc b/chrome/browser/chromeos/extensions/file_manager/open_util.cc
index d3f9bbafc5746fbc8ff11b9db941a6766a16b8a8..ae86003e3d58f4a2705d456a6c172b3aee03af6e 100644
--- a/chrome/browser/chromeos/extensions/file_manager/open_util.cc
+++ b/chrome/browser/chromeos/extensions/file_manager/open_util.cc
@@ -134,129 +134,44 @@ void OpenFileManagerWithInternalActionId(const base::FilePath& file_path,
   ExecuteFileTaskForUrl(profile, task, url);
 }
 
-// Opens the file specified by |file_path| and |url| with a file handler,
-// preferably the default handler for the type of the file.  Returns false if
-// no file handler is found.
-bool OpenFileWithFileHandler(Profile* profile,
-                              const base::FilePath& file_path,
-                              const GURL& url,
-                              const std::string& mime_type,
-                              const std::string& default_task_id) {
-  ExtensionService* service = profile->GetExtensionService();
-  if (!service)
-    return false;
-
-  PathAndMimeTypeSet files;
-  files.insert(std::make_pair(file_path, mime_type));
-  const extensions::FileHandlerInfo* first_handler = NULL;
-  const extensions::Extension* extension_for_first_handler = NULL;
-
-  // If we find the default handler, we execute it immediately, but otherwise,
-  // we remember the first handler, and if there was no default handler, simply
-  // execute the first one.
-  for (ExtensionSet::const_iterator iter = service->extensions()->begin();
-       iter != service->extensions()->end();
-       ++iter) {
-    const Extension* extension = iter->get();
-
-    // We don't support using hosted apps to open files.
-    if (!extension->is_platform_app())
-      continue;
-
-    // We only support apps that specify "incognito: split" if in incognito
-    // mode.
-    if (profile->IsOffTheRecord() &&
-        !service->IsIncognitoEnabled(extension->id()))
-      continue;
-
-    typedef std::vector<const extensions::FileHandlerInfo*> FileHandlerList;
-    FileHandlerList file_handlers = FindFileHandlersForFiles(*extension, files);
-    for (FileHandlerList::iterator i = file_handlers.begin();
-         i != file_handlers.end(); ++i) {
-      const extensions::FileHandlerInfo* handler = *i;
-      std::string task_id = file_tasks::MakeTaskID(
-          extension->id(),
-          file_tasks::TASK_TYPE_FILE_HANDLER,
-          handler->id);
-      if (task_id == default_task_id) {
-        file_tasks::TaskDescriptor task(extension->id(),
-                                        file_tasks::TASK_TYPE_FILE_HANDLER,
-                                        handler->id);
-        ExecuteFileTaskForUrl(profile, task, url);
-        return true;
-
-      } else if (!first_handler) {
-        first_handler = handler;
-        extension_for_first_handler = extension;
-      }
-    }
-  }
-  if (first_handler) {
-    file_tasks::TaskDescriptor task(extension_for_first_handler->id(),
-                                    file_tasks::TASK_TYPE_FILE_HANDLER,
-                                    first_handler->id);
-    ExecuteFileTaskForUrl(profile, task, url);
-    return true;
-  }
-  return false;
-}
-
-// Opens the file specified by |file_path| and |url| with the file browser
-// handler specified by |handler|. Returns false if failed to open the file.
-bool OpenFileWithFileBrowserHandler(Profile* profile,
-                                    const base::FilePath& file_path,
-                                    const FileBrowserHandler& handler,
-                                    const GURL& url) {
-  file_tasks::TaskDescriptor task(handler.extension_id(),
-                                  file_tasks::TASK_TYPE_FILE_BROWSER_HANDLER,
-                                  handler.id());
-  ExecuteFileTaskForUrl(profile, task, url);
-  return true;
-}
-
-// Opens the file specified by |file_path| with a handler (either of file
-// browser handler or file handler, preferably the default handler for the
-// type of the file), or opens the file with the browser. Returns false if
-// failed to open the file.
-bool OpenFileWithHandler(Profile* profile, const base::FilePath& file_path) {
+// Opens the file specified by |file_path| by finding and executing a file
+// taskfor the file. Returns false if failed to open the file (i.e. no file

[kinaba, 2013/09/03 07:27:39]

nit: taskfor -> task for

[satorux1, 2013/09/03 08:07:30]

Done.

+// task is found).
+bool OpenFile(Profile* profile, const base::FilePath& file_path) {
   GURL url;
   if (!ConvertAbsoluteFilePathToFileSystemUrl(
           profile, file_path, kFileManagerAppId, &url))
     return false;
 
   std::string mime_type = GetMimeTypeForPath(file_path);

[kinaba, 2013/09/03 07:27:39]

This is getting the mime type from path, so I guess the issue similar to bug
280442 (extension-less files cannot be opened) remains for the download shelf.
Could you leave some comment?

[satorux1, 2013/09/03 08:07:30]

Done.

-  std::string default_task_id = file_tasks::GetDefaultTaskIdFromPrefs(
-      *profile->GetPrefs(), mime_type, file_path.Extension());
-
-  // We choose the file handler from the following in decreasing priority or
-  // fail if none support the file type:
-  // 1. default file browser handler
-  // 2. default file handler
-  // 3. a fallback handler (e.g. opening in the browser)
-  // 4. non-default file handler
-  // 5. non-default file browser handler
-  // Note that there can be at most one of default extension and default app.
-  const FileBrowserHandler* handler =
-      file_browser_handlers::FindFileBrowserHandlerForURLAndPath(
-          profile, url, file_path);
-  if (!handler) {
-    return OpenFileWithFileHandler(
-        profile, file_path, url, mime_type, default_task_id);
-  }
+  extensions::app_file_handler_util::PathAndMimeTypeSet path_mime_set;
+  path_mime_set.insert(std::make_pair(file_path, mime_type));
+
+  std::vector<GURL> file_urls;
+  file_urls.push_back(url);
+
+  std::vector<base::FilePath> file_paths;
+  file_paths.push_back(file_path);
+
+  std::vector<file_tasks::FullTaskDescriptor> tasks;
+  file_tasks::FindAllTypesOfTasks(profile,

[kinaba, 2013/09/03 07:27:39]

It looks FindAllTypesOfTasks always include DriveApps? What happens if the path
points to a local file in Downloads folder?

[satorux1, 2013/09/03 08:07:30]

Good catch!

We have a similar problem with the file action menu in Downloads. I checked that
fileBrowserPrivate.getFileTasks() returns Drive app tasks for files on Downloads
(oops!), but these tasks are filtered in file_tasks.js:


FileTasks.prototype.processTasks_ = function(tasks) {
  ...
    // Skip Drive App if the file is not on Drive.                              
    if (!isOnDrive && task.driveApp)
      continue;


I think fileBrowserPrivate.getFileTasks() (and FindAllTypesOfTasks) should not
return Drive apps for files not on Drive. Let me file a bug and fix it before
this patch.

[satorux1, 2013/09/04 04:19:42]

Fixed the issue at https://codereview.chromium.org/23477036/

+                                  path_mime_set,
+                                  file_urls,
+                                  file_paths,
+                                  &tasks);
+  if (tasks.empty())
+    return false;
 
-  const file_tasks::TaskDescriptor task_descriptor(
-      handler->extension_id(),
-      file_tasks::TASK_TYPE_FILE_BROWSER_HANDLER,
-      handler->id());
-  const std::string handler_task_id =
-      file_tasks::TaskDescriptorToId(task_descriptor);
-  if (handler_task_id != default_task_id &&
-      !file_browser_handlers::IsFallbackFileBrowserHandler(task_descriptor) &&
-      OpenFileWithFileHandler(
-          profile, file_path, url, mime_type, default_task_id)) {
-    return true;
+  const file_tasks::FullTaskDescriptor* chosen_task = &tasks[0];
+  for (size_t i =0; i < tasks.size(); ++i) {

[kinaba, 2013/09/03 07:27:39]

nit: "i =0" => "i = 0"

[satorux1, 2013/09/03 08:07:30]

Done.

+    if (tasks[i].is_default()) {
+      chosen_task = &tasks[i];
+      break;
+    }
   }
-  return OpenFileWithFileBrowserHandler(profile, file_path, *handler, url);
+
+  ExecuteFileTaskForUrl(profile, chosen_task->task_descriptor(), url);
+  return true;
 }
 
 // Used to implement OpenItem().
@@ -269,8 +184,8 @@ void ContinueOpenItem(Profile* profile,
     // A directory exists at |file_path|. Open it with the file manager.
     OpenFileManagerWithInternalActionId(file_path, "open");
   } else {
-    // |file_path| should be a file. Open it with a handler.
-    if (!OpenFileWithHandler(profile, file_path))
+    // |file_path| should be a file. Open it.
+    if (!OpenFile(profile, file_path))
       ShowWarningMessageBox(profile, file_path);
   }
 }