Chrome OS "open with" picker allowing Web Intents

chrome/browser/chromeos/extensions/file_handler_util.cc

Index: chrome/browser/chromeos/extensions/file_handler_util.cc
diff --git a/chrome/browser/chromeos/extensions/file_handler_util.cc b/chrome/browser/chromeos/extensions/file_handler_util.cc
index 85a9a3029df2e85e7e4c16783e40a2adbf146a45..4bc88ce4acab31426a99b440f54ca0c3e6572ae6 100644
--- a/chrome/browser/chromeos/extensions/file_handler_util.cc
+++ b/chrome/browser/chromeos/extensions/file_handler_util.cc
@@ -20,6 +20,7 @@
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/extensions/extension_tab_util.h"
 #include "chrome/browser/extensions/lazy_background_task_queue.h"
+#include "chrome/browser/extensions/platform_app_launcher.h"
 #include "chrome/browser/prefs/scoped_user_pref_update.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
@@ -38,6 +39,7 @@
 #include "webkit/fileapi/file_system_context.h"
 #include "webkit/fileapi/file_system_url.h"
 #include "webkit/fileapi/file_system_util.h"
+#include "webkit/fileapi/isolated_context.h"
 
 using content::BrowserContext;
 using content::BrowserThread;
@@ -48,11 +50,6 @@ using extensions::Extension;
 
 namespace file_handler_util {
 
-// The prefix used to differentiate drive extensions from Chrome extensions.
-const char FileTaskExecutor::kDriveTaskExtensionPrefix[] = "drive-app:";
-const size_t FileTaskExecutor::kDriveTaskExtensionPrefixLength =
-    arraysize(FileTaskExecutor::kDriveTaskExtensionPrefix) - 1;
-
 namespace {
 typedef std::set<const FileBrowserHandler*> FileBrowserHandlerSet;
 
@@ -101,7 +98,11 @@ const FileBrowserHandler* FindFileBrowserHandler(const Extension* extension,
 }
 
 unsigned int GetAccessPermissionsForHandler(const Extension* extension,
-                                            const std::string& action_id) {
+                                            const std::string& action_id,
+                                            TaskType task_type) {
+  if (task_type == TASK_WEBINTENT)
+    return kReadOnlyFilePermissions;
+
   const FileBrowserHandler* action =
       FindFileBrowserHandler(extension, action_id);
   if (!action)
@@ -115,7 +116,6 @@ unsigned int GetAccessPermissionsForHandler(const Extension* extension,
   return result;
 }
 
-
 std::string EscapedUtf8ToLower(const std::string& str) {
   string16 utf16 = UTF8ToUTF16(
       net::UnescapeURLComponent(str, net::UnescapeRule::NORMAL));
@@ -233,52 +233,42 @@ int GetReadOnlyPermissions() {
 }
 
 std::string MakeTaskID(const std::string& extension_id,
+                       TaskType task_type,
                        const std::string& action_id) {
-  return base::StringPrintf("%s|%s", extension_id.c_str(), action_id.c_str());
-}
-
-std::string MakeDriveTaskID(const std::string& app_id,
-                            const std::string& action_id) {
-  return MakeTaskID(FileTaskExecutor::kDriveTaskExtensionPrefix + app_id,
-                    action_id);
-}
-
-bool CrackDriveTaskID(const std::string& task_id,
-                      std::string* app_id,
-                      std::string* action_id) {
-  std::string app_id_tmp;
-  std::string action_id_tmp;
-  if (!CrackTaskID(task_id, &app_id_tmp, &action_id_tmp))
-    return false;
-  if (StartsWithASCII(app_id_tmp,
-                      FileTaskExecutor::kDriveTaskExtensionPrefix,
-                      false)) {
-    // Strip off the prefix from the extension ID so we convert it to an app id.
-    if (app_id) {
-      *app_id = app_id_tmp.substr(
-          FileTaskExecutor::kDriveTaskExtensionPrefixLength);
-    }
-    if (action_id)
-      *action_id = action_id_tmp;
-    return true;
-  }
-  return false;
+  DCHECK(task_type == TASK_FILE
+      || task_type == TASK_DRIVE
+      || task_type == TASK_WEBINTENT);
+  return base::StringPrintf("%s|%d|%s",

[tbarzic, 2012/09/11 02:01:40]

I'd rather make task_type string (or, if you keep integers, add a comment in
TaskType enum that order of enum values cannot change)

[thorogood, 2012/09/13 02:34:49]

I agree with you. I've updated this CL with all the other suggestions, and I'll
ping it again when I've converted this to a string.

[thorogood, 2012/09/19 07:07:17]

just updating this as Done :)

+                            extension_id.c_str(),
+                            task_type,
+                            action_id.c_str());
 }
 
 // Breaks down task_id that is used between getFileTasks() and executeTask() on
 // its building blocks. task_id field the following structure:
-//     <extension-id>|<task-action-id>
+//     <extension-id>|<task-type>|<task-action-id>
 bool CrackTaskID(const std::string& task_id,
                  std::string* extension_id,
+                 TaskType* task_type,
                  std::string* action_id) {
   std::vector<std::string> result;
   int count = Tokenize(task_id, std::string("|"), &result);
-  if (count != 2)
+  if (count != 3)

[tbarzic, 2012/09/11 02:01:40]

Since task ids are persisted in prefs, we should probably keep supporting old
style task ids (at least for a while)

[thorogood, 2012/09/13 02:34:49]

Sounds good. If we see two components I'll try to parse out the Drive/File
intent that was previously contained.

     return false;
+
   if (extension_id)
     *extension_id = result[0];
+
+  if (task_type) {
+    *task_type = file_handler_util::TaskType(atoi(result[1].c_str()));
+    DCHECK(*task_type == TASK_FILE
+        || *task_type == TASK_DRIVE
+        || *task_type == TASK_WEBINTENT);
+  }
+
   if (action_id)
-    *action_id = result[1];
+    *action_id = result[2];
+
   return true;
 }
 
@@ -296,6 +286,8 @@ FileBrowserHandlerSet::iterator FindHandler(
   return iter;
 }
 
+// Given the list of selected files, returns array of file action tasks
+// that are shared between them.
 void FindDefaultTasks(Profile* profile,
                       const std::vector<GURL>& files_list,
                       const FileBrowserHandlerSet& common_tasks,
@@ -324,7 +316,7 @@ void FindDefaultTasks(Profile* profile,
   // from common_tasks.
   for (FileBrowserHandlerSet::const_iterator task_iter = common_tasks.begin();
        task_iter != common_tasks.end(); ++task_iter) {
-    std::string task_id = MakeTaskID((*task_iter)->extension_id(),
+    std::string task_id = MakeTaskID((*task_iter)->extension_id(), TASK_FILE,
                                      (*task_iter)->id());
     for (std::set<std::string>::iterator default_iter = default_ids.begin();
          default_iter != default_ids.end(); ++default_iter) {
@@ -426,6 +418,7 @@ class ExtensionTaskExecutor : public FileTaskExecutor {
   ExtensionTaskExecutor(Profile* profile,
                         const GURL source_url,
                         const std::string& extension_id,
+                        TaskType task_type,
                         const std::string& action_id);
   virtual ~ExtensionTaskExecutor();
 
@@ -464,7 +457,6 @@ class ExtensionTaskExecutor : public FileTaskExecutor {
   void InitHandlerHostFileAccessPermissions(
       const FileDefinitionList& file_list,
       const extensions::Extension* handler_extension,
-      const std::string& action_id,
       const base::Closure& callback);
 
   // Invoked upon completion of InitHandlerHostFileAccessPermissions initiated
@@ -484,6 +476,7 @@ class ExtensionTaskExecutor : public FileTaskExecutor {
 
   const GURL source_url_;
   const std::string extension_id_;
+  TaskType task_type_;
   const std::string action_id_;
   FileTaskFinishedCallback done_;
 
@@ -495,12 +488,9 @@ class ExtensionTaskExecutor : public FileTaskExecutor {
 FileTaskExecutor* FileTaskExecutor::Create(Profile* profile,
                                            const GURL source_url,
                                            const std::string& extension_id,
+                                           TaskType task_type,
                                            const std::string& action_id) {
-  // Check out the extension ID and see if this is a drive task,
-  // and instantiate drive-specific executor if so.
-  if (StartsWithASCII(extension_id,
-                      FileTaskExecutor::kDriveTaskExtensionPrefix,
-                      false)) {
+  if (task_type == TASK_DRIVE) {
     return new gdata::DriveTaskExecutor(profile,
                                         extension_id, // really app_id
                                         action_id);
@@ -508,6 +498,7 @@ FileTaskExecutor* FileTaskExecutor::Create(Profile* profile,
     return new ExtensionTaskExecutor(profile,
                                      source_url,
                                      extension_id,
+                                     task_type,
                                      action_id);
   }
 }
@@ -695,10 +686,12 @@ ExtensionTaskExecutor::ExtensionTaskExecutor(
     Profile* profile,
     const GURL source_url,
     const std::string& extension_id,
+    TaskType task_type,
     const std::string& action_id)
   : FileTaskExecutor(profile),
     source_url_(source_url),
     extension_id_(extension_id),
+    task_type_(task_type),
     action_id_(action_id) {
 }
 
@@ -792,7 +785,6 @@ void ExtensionTaskExecutor::ExecuteFileActionsOnUIThread(
   InitHandlerHostFileAccessPermissions(
       file_list,
       extension,
-      action_id_,
       base::Bind(
           &ExtensionTaskExecutor::OnInitAccessForExecuteFileActionsOnUIThread,
           this,
@@ -849,6 +841,19 @@ void ExtensionTaskExecutor::SetupPermissionsAndDispatchEvent(
     return;
   }
 
+  // If we're a Web Intent action (to an extension), short-circuit and deliver
+  // the Web Intent via LaunchPlatformAppWithPath.
+  if (task_type_ == TASK_WEBINTENT) {
+    for (FileDefinitionList::const_iterator iter = file_list.begin();

[tbarzic, 2012/09/11 02:01:40]

why don't you create a separate executor for intents.. looks like you don't need
the most of the stuff handled in ExtensionTaskExecutor (opening external file
system, granting file access permissions and waking up lazy background page,
dispatching task event).
afaik, LaunchPlatformAppWihtPath does all of this for the web intent (correct me
if I'm wrong :) )

(the extension only needs access to isolated file system, right?)

[thorogood, 2012/09/13 02:34:49]

Yup. You're totally right. This is much more concise now, thanks for the
suggestion.

+         iter != file_list.end(); ++iter) {
+      extensions::LaunchPlatformAppWithPath(
+          profile(), GetExtension(), iter->absolute_path);
+    }
+    ExecuteDoneOnUIThread(true);
+    return;
+  }
+  DCHECK_EQ(task_type_, TASK_FILE);
+
   extensions::EventRouter* event_router = profile()->GetExtensionEventRouter();
   if (!event_router) {
     ExecuteDoneOnUIThread(false);
@@ -895,7 +900,6 @@ void ExtensionTaskExecutor::SetupPermissionsAndDispatchEvent(
 void ExtensionTaskExecutor::InitHandlerHostFileAccessPermissions(
     const FileDefinitionList& file_list,
     const Extension* handler_extension,
-    const std::string& action_id,
     const base::Closure& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
@@ -904,9 +908,10 @@ void ExtensionTaskExecutor::InitHandlerHostFileAccessPermissions(
        iter != file_list.end();
        ++iter) {
     // Setup permission for file's absolute file.
-    handler_host_permissions_.push_back(std::make_pair(
-        iter->absolute_path,
-        GetAccessPermissionsForHandler(handler_extension, action_id)));
+    handler_host_permissions_.push_back(std::make_pair(iter->absolute_path,
+        GetAccessPermissionsForHandler(handler_extension,
+                                       action_id_,
+                                       task_type_)));
 
     if (gdata::util::IsUnderGDataMountPoint(iter->absolute_path))
       gdata_paths->push_back(iter->virtual_path);