Chrome OS "open with" picker allowing Web Intents

chrome/browser/chromeos/extensions/file_browser_private_api.cc

Index: chrome/browser/chromeos/extensions/file_browser_private_api.cc
diff --git a/chrome/browser/chromeos/extensions/file_browser_private_api.cc b/chrome/browser/chromeos/extensions/file_browser_private_api.cc
index 2999551744cd52e8b891817bb33e93e6275bbed2..a56f79779ceeb01980909fa5245e8fc8cf73fc4c 100644
--- a/chrome/browser/chromeos/extensions/file_browser_private_api.cc
+++ b/chrome/browser/chromeos/extensions/file_browser_private_api.cc
@@ -9,6 +9,9 @@
 
 #include "base/base64.h"
 #include "base/bind.h"
+#include "base/file_path.h"
+#include "base/file_util.h"
+#include "base/i18n/case_conversion.h"
 #include "base/json/json_writer.h"
 #include "base/logging.h"
 #include "base/memory/scoped_vector.h"
@@ -17,6 +20,7 @@
 #include "base/stringprintf.h"
 #include "base/time.h"
 #include "base/values.h"
+#include "base/utf_string_conversions.h"
 #include "chrome/browser/chromeos/cros/cros_library.h"
 #include "chrome/browser/chromeos/cros/network_library.h"
 #include "chrome/browser/chromeos/extensions/file_handler_util.h"
@@ -35,6 +39,7 @@
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_tab_util.h"
 #include "chrome/browser/extensions/process_map.h"
+#include "chrome/browser/intents/web_intents_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_window.h"
@@ -54,6 +59,7 @@
 #include "grit/generated_resources.h"
 #include "grit/platform_locale_settings.h"
 #include "net/base/escape.h"
+#include "net/base/mime_util.h"
 #include "ui/base/dialogs/selected_file_info.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "webkit/chromeos/fileapi/cros_mount_point_provider.h"
@@ -63,6 +69,7 @@
 #include "webkit/fileapi/file_system_types.h"
 #include "webkit/fileapi/file_system_url.h"
 #include "webkit/fileapi/file_system_util.h"
+#include "webkit/glue/web_intent_service_data.h"
 
 using chromeos::disks::DiskMountManager;
 using content::BrowserContext;
@@ -78,7 +85,7 @@ using gdata::OperationRegistry;
 namespace {
 
 // Default icon path for drive docs.
-const char kDefaultDriveIcon[] = "images/filetype_generic.png";
+const char kDefaultIcon[] = "images/filetype_generic.png";
 const int kPreferredIconSize = 16;
 
 // Error messages.
@@ -235,6 +242,45 @@ GURL FindPreferredIcon(const InstalledApp::IconList& icons,
   return result;
 }
 
+// Given an extension, a Web Intents |action|, and a set of |mime_types|,
+// determines whether this extension supports this action for all specified
+// mime-types.
+// If this method returns true, also returns the |title| of this action.

[tbarzic, 2012/09/13 04:29:17]

why not add function std::string GetTitleForAction()
that will return the title of the first intents_service (or first non empty
title,, in case it can be ''). 

[thorogood, 2012/09/13 08:14:04]

I actually started with that, and benwells@ suggested rolling it into this
method. I feel like this approach is fine.

[benwells, 2012/09/13 09:52:30]

Either way you have to handle the case where an extension has multiple view
actions for different mime types, with different titles. In this case you need
to look at the mime types you're handling to pick the right title. If there are
multiple mime types being handled there might be multiple possible titles, but
you should always pick a valid one.

[tbarzic, 2012/09/13 18:44:45]

yeah, makes sense..
I missed continue part in the for loop when I was writing the comment.

Still, I think it would be clearer if we avoided having function that does two
things.. but, do as you like :)

You should probably reflect in the function name that it also returns action
title..

[thorogood, 2012/09/16 03:41:18]

I've changed this method to be named "FindTitleForActionWithTypes", which I feel
is also accurate, and updated the comment. Basically you can legitimately think
of this method as finding the title of the specified action that supports these
mime-types.

Btw, interestingly enough, since Web Intents are specified as a action/spec
mapping, then there is a direct mapping between action and title. (It's just
that if an action supports multiple mime types, this is flattened in the
Extension initialisation code - so finding the "first" match is fine, since
they'll all be the same). Ben's comment is thus invalid.

I know this probably feels like bikeshedding from all directions, but given the
above paragraph, I'm happy to break this apart into two methods again if you
think it's cleaner.

[tbarzic, 2012/09/16 04:26:11]

I'm ok with this..

[benwells, 2012/09/17 01:37:23]

Ah, my bad. I'm OK with either one or two functions since the titles will all be
the same.

+bool ExtensionSupportsAction(const Extension* extension,
+    const std::string& action,
+    const std::set<std::string>& mime_types,
+    std::string* title) {
+  DCHECK(!mime_types.empty());
+  std::set<std::string> pending(mime_types.begin(), mime_types.end());
+  std::string found_title;
+
+  for (std::vector<webkit_glue::WebIntentServiceData>::const_iterator i =

[tbarzic, 2012/09/13 04:29:17]

since you have nested loops, it would probably be better if iterator names are
more descriptive.

[thorogood, 2012/09/13 08:14:04]

Done.

+          extension->intents_services().begin();
+       i != extension->intents_services().end(); ++i) {
+    if (pending.empty())
+      break;
+
+    if (UTF16ToUTF8(i->action) != action)
+      continue;
+
+    std::set<std::string>::iterator pi = pending.begin();
+    while (pi != pending.end()) {

[tbarzic, 2012/09/13 04:29:17]

how about (I think it is nicer, but your call):

std::set<std::string>::iterator current = pi++;
if (net::MatchesMime...)
  pending.erase(current);

[thorogood, 2012/09/13 08:14:04]

I like it. I'm hardly a C++ guru :)

+      if (net::MatchesMimeType(UTF16ToUTF8(i->type), *pi))
+        pending.erase(pi++);
+      else
+        ++pi;
+    }
+    if (found_title.empty())
+      found_title = UTF16ToUTF8(i->title);
+  }
+
+  if (!pending.empty())
+    return false;   // we haven't found all mime-types
+
+  *title = found_title;
+  return true;
+}
+
 // Retrieves total and remaining available size on |mount_path|.
 void GetSizeStatsOnFileThread(const std::string& mount_path,
                               size_t* total_size_kb,
@@ -601,8 +647,8 @@ void GetFileTasksFileBrowserFunction::IntersectAvailableDriveTasks(
           app_info->insert(std::make_pair((*apps)->app_id, *apps));
       // TODO(gspencer): For now, the action id is always "open-with", but we
       // could add any actions that the drive app supports.
-      std::string task_id =
-          file_handler_util::MakeDriveTaskID((*apps)->app_id, "open-with");
+      std::string task_id = file_handler_util::MakeTaskID(
+          (*apps)->app_id, file_handler_util::TASK_DRIVE, "open-with");
       tasks_for_this_file.insert(task_id);
       // If we failed to insert a task_id because there was a duplicate, then we
       // must delete it (since we own it).
@@ -655,9 +701,13 @@ void GetFileTasksFileBrowserFunction::CreateDriveTasks(
   for (std::set<std::string>::const_iterator app_iter = available_tasks.begin();
        app_iter != available_tasks.end(); ++app_iter) {
     std::string app_id;
-    bool result = file_handler_util::CrackDriveTaskID(*app_iter, &app_id, NULL);
+    file_handler_util::TaskType task_type;
+    bool result = file_handler_util::CrackTaskID(
+        *app_iter, &app_id, &task_type, NULL);
     DCHECK(result) << "Unable to parse Drive task id: " << *app_iter;
-    if (!result)
+    DCHECK_EQ(task_type, file_handler_util::TASK_DRIVE)

[tbarzic, 2012/09/13 04:29:17]

I don't think the log adds extra value to the check..

[thorogood, 2012/09/13 08:14:04]

Done.

+        << "Expected Drive task type";
+    if (!result || task_type != file_handler_util::TASK_DRIVE)

[tbarzic, 2012/09/13 04:29:17]

you have a dcheck for (task_type == TASK_DRIVE), so you shouldn't handle the
case task_type != TASK_DRIVE...

[thorogood, 2012/09/13 08:14:04]

Well, before I got here, there was also a DCHECK for calling CrackDriveTaskID.
This if statement was also here before, and is just sanity for production code.

[tbarzic, 2012/09/13 18:44:45]

yeah, but the thing is DCHECK contradicts the need to handle failure case. So
you should either remove if() or DCHECK.
(in this case, tasks in available_tasks are created by MakeDriveTaskID (see
IntersectAvaliableDriveTasks) just before calling this function, so I would go
with keeping DCHECKs)

[thorogood, 2012/09/16 03:41:18]

Fair enough. I think I assumed that this might be somehow unsanitised data, but
you're right here. I'll remove the if in favour of DCHECKs.

       continue;
     WebAppInfoMap::const_iterator info_iter = app_info.find(app_id);
     DCHECK(info_iter != app_info.end());
@@ -706,7 +756,6 @@ bool GetFileTasksFileBrowserFunction::FindDriveAppTasks(
   if (!system_service || !system_service->webapps_registry())
     return true;
 
-
   gdata::DriveWebAppsRegistry* registry = system_service->webapps_registry();
 
   // Map of app_id to DriveWebAppInfo so we can look up the apps we've found
@@ -728,6 +777,73 @@ bool GetFileTasksFileBrowserFunction::FindDriveAppTasks(
   return true;
 }
 
+// Find Web Intent platform apps that support the View task, and add them to
+// the |result_list|. These will be marked as TASK_WEBINTENT.
+bool GetFileTasksFileBrowserFunction::FindWebIntentTasks(
+    const std::vector<GURL>& file_urls,
+    ListValue* result_list) {
+  DCHECK(!file_urls.empty());
+  ExtensionService* service = profile_->GetExtensionService();
+  if (!service)
+    return false;
+
+  std::set<std::string> mime_types;
+  for (std::vector<GURL>::const_iterator iter = file_urls.begin();
+       iter != file_urls.end(); ++iter) {
+    const FilePath file = FilePath(GURL(iter->spec()).ExtractFileName());
+    const FilePath::StringType file_extension =
+        StringToLowerASCII(file.Extension());
+
+    // TODO(thorogood): Rearchitect this call so it can run on the File thread;
+    // GetMimeTypeFromFile requires this on Linux. Right now, we use
+    // Chrome-level knowledge only.
+    std::string mime_type;
+    if (!file_extension.empty() && !net::GetWellKnownMimeTypeFromExtension(
+          file_extension.substr(1), &mime_type))
+      mime_type = std::string();  // There was no mime-type available from this

[tbarzic, 2012/09/13 04:29:17]

does this mean that std::string() is used to represent "*" matcher?

[thorogood, 2012/09/13 08:14:04]

Almost. You're the wrong way around; this says that if the file doesn't have an
extension (or we can't find it's mime-type) then we say the file has a mime type
of "", which is only supported if the Web Intents handler is "*" or "*/*".

[tbarzic, 2012/09/13 18:44:45]

oh, could you please clarify this in the comment.

also nits:
could you put the comment in the line before |mime_type = std::string()|

please add { } for the if block

[thorogood, 2012/09/16 03:41:18]

I've made these changes and made this a bit clearer with an if/else block.

+                                  // file, but we'll still match "*".
+    mime_types.insert(mime_type);
+  }
+
+  for (ExtensionSet::const_iterator iter = service->extensions()->begin();
+       iter != service->extensions()->end();
+       ++iter) {
+    const Extension* extension = *iter;
+    std::string title;
+
+    // We don't support using hosted apps to open files.
+    if (!extension->is_platform_app())
+      continue;
+
+    if (profile_->IsOffTheRecord() &&
+        !service->IsIncognitoEnabled(extension->id()))
+      continue;
+
+    if (!ExtensionSupportsAction(
+        extension, web_intents::kActionView, mime_types, &title))
+      continue;
+
+    DictionaryValue* task = new DictionaryValue;
+    std::string task_id = file_handler_util::MakeTaskID(extension->id(),
+        file_handler_util::TASK_WEBINTENT, web_intents::kActionView);
+    task->SetString("taskId", task_id);
+    task->SetString("title", title);
+    task->SetBoolean("isDefault", false);
+
+    GURL best_icon = extension->GetIconURL(kPreferredIconSize,
+                                           ExtensionIconSet::MATCH_BIGGER);
+    if (!best_icon.is_empty())
+      task->SetString("iconUrl", best_icon.spec());
+    else
+      task->SetString("iconUrl", kDefaultIcon);
+
+    task->SetBoolean("driveApp", false);
+    result_list->Append(task);
+  }
+
+  return true;
+}
+
 bool GetFileTasksFileBrowserFunction::RunImpl() {
   // First argument is the list of files to get tasks for.
   ListValue* files_list = NULL;
@@ -797,8 +913,8 @@ bool GetFileTasksFileBrowserFunction::RunImpl() {
     const Extension* extension = service->GetExtensionById(extension_id, false);
     CHECK(extension);
     DictionaryValue* task = new DictionaryValue;
-    task->SetString("taskId",
-        file_handler_util::MakeTaskID(extension_id, handler->id()));
+    task->SetString("taskId", file_handler_util::MakeTaskID(
+        extension_id, file_handler_util::TASK_FILE, handler->id()));
     task->SetString("title", handler->title());
     // TODO(zelidrag): Figure out how to expose icon URL that task defined in
     // manifest instead of the default extension icon.
@@ -822,6 +938,12 @@ bool GetFileTasksFileBrowserFunction::RunImpl() {
     result_list->Append(task);
   }
 
+  // Take the union of Web Intents (that platform apps may accept) and all
+  // previous Drive and extension tasks. As above, we know there aren't
+  // duplicates because they're entirely different kinds of tasks.
+  if (!FindWebIntentTasks(file_urls, result_list))
+    return false;
+
   if (VLOG_IS_ON(1)) {
     std::string result_json;
     base::JSONWriter::WriteWithOptions(
@@ -832,8 +954,6 @@ bool GetFileTasksFileBrowserFunction::RunImpl() {
     VLOG(1) << "GetFileTasks result:\n" << result_json;
   }
 
-  // TODO(zelidrag, serya): Add intent content tasks to result_list once we
-  // implement that API.
   SendResponse(true);
   return true;
 }
@@ -862,8 +982,10 @@ bool ExecuteTasksFileBrowserFunction::RunImpl() {
     return false;
 
   std::string extension_id;
+  file_handler_util::TaskType task_type;
   std::string action_id;
-  if (!file_handler_util::CrackTaskID(task_id, &extension_id, &action_id)) {
+  if (!file_handler_util::CrackTaskID(
+      task_id, &extension_id, &task_type, &action_id)) {
     LOG(WARNING) << "Invalid task " << task_id;
     return false;
   }
@@ -885,6 +1007,7 @@ bool ExecuteTasksFileBrowserFunction::RunImpl() {
       FileTaskExecutor::Create(profile(),
                                source_url(),
                                extension_id,
+                               task_type,
                                action_id));
 
   if (!executor->ExecuteAndNotify(
@@ -2016,7 +2139,8 @@ void GetDriveFilePropertiesFunction::OnOperationComplete(
         file_specific_info.content_mime_type(),
         file_path.Extension());
     std::string default_app_id;
-    file_handler_util::CrackDriveTaskID(default_task_id, &default_app_id, NULL);
+    file_handler_util::CrackTaskID(
+        default_task_id, &default_app_id, NULL, NULL);
 
     ListValue* apps = new ListValue();
     property_dict->Set("driveApps", apps);