Chrome OS "open with" picker allowing Web Intents

chrome/browser/extensions/platform_app_launcher.cc

Index: chrome/browser/extensions/platform_app_launcher.cc
diff --git a/chrome/browser/extensions/platform_app_launcher.cc b/chrome/browser/extensions/platform_app_launcher.cc
index 3af7454a25ab3221dd2d1fa85cb3c01c7a13e6af..a2fb75390731bd65f308a5b0d647d5065c3ec388 100644
--- a/chrome/browser/extensions/platform_app_launcher.cc
+++ b/chrome/browser/extensions/platform_app_launcher.cc
@@ -53,84 +53,70 @@ bool MakePathAbsolute(const FilePath& current_directory,
   return true;
 }
 
-// Class to handle launching of platform apps with command line information.
+// Class to handle launching of platform apps to open a specific path.
 // An instance of this class is created for each launch. The lifetime of these
 // instances is managed by reference counted pointers. As long as an instance
 // has outstanding tasks on a message queue it will be retained; once all
 // outstanding tasks are completed it will be deleted.
-class PlatformAppCommandLineLauncher
-    : public base::RefCountedThreadSafe<PlatformAppCommandLineLauncher> {
+class PlatformAppPathLauncher
+    : public base::RefCountedThreadSafe<PlatformAppPathLauncher> {
  public:
-  PlatformAppCommandLineLauncher(Profile* profile,
-                                 const Extension* extension,
-                                 const CommandLine* command_line,
-                                 const FilePath& current_directory)
+  PlatformAppPathLauncher(Profile* profile,
+                          const Extension* extension,
+                          const FilePath& file_path)
       : profile_(profile),
         extension_(extension),
-        command_line_(command_line),
-        current_directory_(current_directory) {}
+        file_path_(file_path) {}
 
   void Launch() {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-    if (!command_line_ || !command_line_->GetArgs().size()) {
+    if (file_path_.empty()) {
       LaunchWithNoLaunchData();
       return;
     }
 
-    FilePath file_path(command_line_->GetArgs()[0]);
+    DCHECK(file_path_.IsAbsolute());
     BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, base::Bind(
-            &PlatformAppCommandLineLauncher::GetMimeTypeAndLaunch,
-            this, file_path));
+            &PlatformAppPathLauncher::GetMimeTypeAndLaunch, this));
   }
 
  private:
-  friend class base::RefCountedThreadSafe<PlatformAppCommandLineLauncher>;
+  friend class base::RefCountedThreadSafe<PlatformAppPathLauncher>;
 
-  virtual ~PlatformAppCommandLineLauncher() {}
+  virtual ~PlatformAppPathLauncher() {}
 
   void LaunchWithNoLaunchData() {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
     extensions::AppEventRouter::DispatchOnLaunchedEvent(profile_, extension_);
   }
 
-  void GetMimeTypeAndLaunch(const FilePath& file_path) {
+  void GetMimeTypeAndLaunch() {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
-    // If we cannot construct an absolute path, launch with no launch data.
-    FilePath absolute_path(file_path);
-    if (!MakePathAbsolute(current_directory_, &absolute_path)) {
-      LOG(WARNING) << "Cannot make absolute path from " << file_path.value();
-      BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(
-              &PlatformAppCommandLineLauncher::LaunchWithNoLaunchData, this));
-      return;
-    }
-
     // If the file doesn't exist, or is a directory, launch with no launch data.
-    if (!file_util::PathExists(absolute_path) ||
-        file_util::DirectoryExists(absolute_path)) {
-      LOG(WARNING) << "No file exists with path " << absolute_path.value();
+    if (!file_util::PathExists(file_path_) ||
+        file_util::DirectoryExists(file_path_)) {
+      LOG(WARNING) << "No file exists with path " << file_path_.value();
       BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(
-              &PlatformAppCommandLineLauncher::LaunchWithNoLaunchData, this));
+              &PlatformAppPathLauncher::LaunchWithNoLaunchData, this));
       return;
     }
 
     std::string mime_type;
     // If we cannot obtain the MIME type, launch with no launch data.
-    if (!net::GetMimeTypeFromFile(absolute_path, &mime_type)) {
+    if (!net::GetMimeTypeFromFile(file_path_, &mime_type)) {
       LOG(WARNING) << "Could not obtain MIME type for "
-                   << absolute_path.value();
+                   << file_path_.value();
       BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(
-              &PlatformAppCommandLineLauncher::LaunchWithNoLaunchData, this));
+              &PlatformAppPathLauncher::LaunchWithNoLaunchData, this));
       return;
     }
 
     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(
-            &PlatformAppCommandLineLauncher::LaunchWithMimeTypeAndPath,
-            this, absolute_path, mime_type));
+            &PlatformAppPathLauncher::LaunchWithMimeType, this, mime_type));
   }
 
-  void LaunchWithMimeTypeAndPath(const FilePath& file_path,
-                                 const std::string& mime_type) {
+  void LaunchWithMimeType(const std::string& mime_type) {
     // Find the intent service from the platform app for the file being opened.
     webkit_glue::WebIntentServiceData service;
     bool found_service = false;
@@ -151,7 +137,7 @@ class PlatformAppCommandLineLauncher
     // no launch data.
     if (!found_service) {
       LOG(WARNING) << "Extension does not provide a valid intent for "
-                   << file_path.value();
+                   << file_path_.value();
       LaunchWithNoLaunchData();
       return;
     }
@@ -165,8 +151,8 @@ class PlatformAppCommandLineLauncher
         ExtensionSystem::Get(profile_)->lazy_background_task_queue();
     if (queue->ShouldEnqueueTask(profile_, extension_)) {
       queue->AddPendingTask(profile_, extension_->id(), base::Bind(
-              &PlatformAppCommandLineLauncher::GrantAccessToFileAndLaunch,
-              this, file_path, mime_type));
+              &PlatformAppPathLauncher::GrantAccessToFileAndLaunch,
+              this, mime_type));
       return;
     }
 
@@ -175,11 +161,10 @@ class PlatformAppCommandLineLauncher
     extensions::ExtensionHost* host =
         process_manager->GetBackgroundHostForExtension(extension_->id());
     DCHECK(host);
-    GrantAccessToFileAndLaunch(file_path, mime_type, host);
+    GrantAccessToFileAndLaunch(mime_type, host);
   }
 
-  void GrantAccessToFileAndLaunch(const FilePath& file_path,
-                                  const std::string& mime_type,
+  void GrantAccessToFileAndLaunch(const std::string& mime_type,
                                   extensions::ExtensionHost* host) {
     // If there was an error loading the app page, |host| will be NULL.
     if (!host) {
@@ -195,15 +180,15 @@ class PlatformAppCommandLineLauncher
     // If the renderer already has permission to read these paths, it is not
     // regranted, as this would overwrite any other permissions which the
     // renderer may already have.
-    if (!policy->CanReadFile(renderer_id, file_path))
-      policy->GrantReadFile(renderer_id, file_path);
+    if (!policy->CanReadFile(renderer_id, file_path_))
+      policy->GrantReadFile(renderer_id, file_path_);
 
     std::string registered_name;
     fileapi::IsolatedContext* isolated_context =
         fileapi::IsolatedContext::GetInstance();
     DCHECK(isolated_context);
     std::string filesystem_id = isolated_context->RegisterFileSystemForPath(
-        fileapi::kFileSystemTypeIsolated, file_path, &registered_name);
+        fileapi::kFileSystemTypeIsolated, file_path_, &registered_name);
     // Granting read file system permission as well to allow file-system
     // read operations.
     policy->GrantReadFileSystem(renderer_id, filesystem_id);
@@ -217,12 +202,10 @@ class PlatformAppCommandLineLauncher
   Profile* profile_;
   // The extension providing the app.
   const Extension* extension_;
-  // The command line to be passed through to the app, or NULL.
-  const CommandLine* command_line_;
-  // If non-empty, this is used to expand relative paths.
-  const FilePath current_directory_;
+  // The path to be passed through to the app. This may be the empty path.
+  const FilePath file_path_;
 
-  DISALLOW_COPY_AND_ASSIGN(PlatformAppCommandLineLauncher);
+  DISALLOW_COPY_AND_ASSIGN(PlatformAppPathLauncher);
 };
 
 // Class to handle launching of platform apps with WebIntent data that is being
@@ -309,12 +292,31 @@ void LaunchPlatformApp(Profile* profile,
                        const Extension* extension,
                        const CommandLine* command_line,
                        const FilePath& current_directory) {
+  // Optionally resolve the file_path from the first command-line argument.

[benwells, 2012/08/30 08:09:03]

I really like these refactorings as the role of the PlatformAppPathLauncher
seems really clean: it launches apps with paths.

Let's keep it nice and clean by making it not handle the simple case of
launching with no data. That can just become a function in the anonymous
namespace used by LaunchPlatformApp if there is no command line data, or by the
PlatformAppPathLauncher if there is some problem.

Also, creating a function GetAbsolutePathFromCommandLine  would make this really
nice, it can then be something like:

if (GetAbsolutePathFromCommandLine(command_line, cur_dir, &path) {
  LaunchPlatformAppWIthPath(...)
  return;
}

LaunchPlatformAppWithNoData(...)

[thorogood, 2012/08/31 01:27:32]

These are great suggestions! The code feels much neater now. 
The only thing I'm hesitant about is that we still have the method
"LaunchPlatformApp", when we could instead call it
"LaunchPlatformAppWithCommandLine". However perhaps it's not required...

+  FilePath file_path;
+  if (command_line && command_line->GetArgs().size()) {
+    file_path = FilePath(command_line->GetArgs()[0]);
+
+    // If we cannot construct an absolute path, launch with no launch data.
+    FilePath absolute_path(file_path);
+    if (!MakePathAbsolute(current_directory, &absolute_path)) {
+      LOG(WARNING) << "Cannot make absolute path from " << file_path.value();
+      file_path = FilePath();
+    } else {
+      file_path = absolute_path;
+    }
+  }
+  LaunchPlatformAppWithPath(profile, extension, file_path);
+}
+
+void LaunchPlatformAppWithPath(Profile* profile,
+                               const Extension* extension,
+                               const FilePath& file_path) {
   // launcher will be freed when nothing has a reference to it. The message
   // queue will retain a reference for any outstanding task, so when the
   // launcher has finished it will be freed.
-  scoped_refptr<PlatformAppCommandLineLauncher> launcher =
-      new PlatformAppCommandLineLauncher(profile, extension, command_line,
-                                         current_directory);
+  scoped_refptr<PlatformAppPathLauncher> launcher =
+      new PlatformAppPathLauncher(profile, extension, file_path);
   launcher->Launch();
 }