Use file thread when converting paths to absolute when loading apps.

apps/launcher.cc

Index: apps/launcher.cc
diff --git a/apps/launcher.cc b/apps/launcher.cc
index 8d31a0115c46e671d56a953aa194942a20f388df..95a6946da07c92005a5cb5948b1d4c1cbfc53181 100644
--- a/apps/launcher.cc
+++ b/apps/launcher.cc
@@ -63,8 +63,8 @@ namespace {
 
 const char kFallbackMimeType[] = "application/octet-stream";
 
-bool MakePathAbsolute(const base::FilePath& current_directory,
-                      base::FilePath* file_path) {
+bool DoMakePathAbsolute(const base::FilePath& current_directory,
+                        base::FilePath* file_path) {
   DCHECK(file_path);
   if (file_path->IsAbsolute())
     return true;
@@ -81,22 +81,6 @@ bool MakePathAbsolute(const base::FilePath& current_directory,
   return true;
 }
 
-bool GetAbsolutePathFromCommandLine(const CommandLine& command_line,
-                                    const base::FilePath& current_directory,
-                                    base::FilePath* path) {
-  if (!command_line.GetArgs().size())
-    return false;
-
-  base::FilePath relative_path(command_line.GetArgs()[0]);
-  base::FilePath absolute_path(relative_path);
-  if (!MakePathAbsolute(current_directory, &absolute_path)) {
-    LOG(WARNING) << "Cannot make absolute path from " << relative_path.value();
-    return false;
-  }
-  *path = absolute_path;
-  return true;
-}
-
 // Helper method to launch the platform app |extension| with no data. This
 // should be called in the fallback case, where it has been impossible to
 // load or obtain file launch data.
@@ -147,11 +131,33 @@ class PlatformAppPathLauncher
     Launch();
   }
 
+  void LaunchWithRelativePath(const base::FilePath& current_directory) {
+    BrowserThread::PostTask(
+        BrowserThread::FILE,
+        FROM_HERE,
+        base::Bind(&PlatformAppPathLauncher::MakePathAbsolute,
+                   this,
+                   current_directory));
+  }
+
  private:
   friend class base::RefCountedThreadSafe<PlatformAppPathLauncher>;
 
   virtual ~PlatformAppPathLauncher() {}
 
+  void MakePathAbsolute(const base::FilePath& current_directory) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));

[tapted, 2014/05/12 00:27:45]

oh! I forgot - there's now also DCHECK_CURRENTLY_ON for this

[benwells, 2014/05/12 05:51:05]

Nice, I didn't know about that.

+
+    if (!DoMakePathAbsolute(current_directory, &file_path_)) {
+      LOG(WARNING) << "Cannot make absolute path from " << file_path_.value();
+      file_path_ = base::FilePath();
+    }
+
+    BrowserThread::PostTask(BrowserThread::UI,
+                            FROM_HERE,
+                            base::Bind(&PlatformAppPathLauncher::Launch, this));
+  }
+
   void OnFileValid() {
 #if defined(OS_CHROMEOS)
     if (drive::util::IsUnderDriveMountPoint(file_path_)) {
@@ -312,7 +318,7 @@ class PlatformAppPathLauncher
   // The extension providing the app.
   const Extension* extension_;

[tapted, 2014/05/12 00:07:51]

should this be a scoped_refptr?

[benwells, 2014/05/12 00:18:51]

I don't think so. My understanding is that extension is not meant to be used as
a ref ptr except internally.

To protect from any extension destruction while a launch with command line data
is going on, we'd have to store the id and look it up all the time.

[benwells, 2014/05/12 05:51:05]

As discussed, added a TODO for this.

   // The path to be passed through to the app.
-  const base::FilePath file_path_;
+  base::FilePath file_path_;
   // The ID of the file handler used to launch the app.
   std::string handler_id_;
 
@@ -342,14 +348,18 @@ void LaunchPlatformAppWithCommandLine(Profile* profile,
     }
   }
 
-  base::FilePath path;
-  if (!GetAbsolutePathFromCommandLine(command_line, current_directory, &path)) {
+  if (!command_line.GetArgs().size()) {

[tapted, 2014/05/12 00:07:51]

nit: if (command_line.GetArgs().empty()) {

[benwells, 2014/05/12 05:51:05]

Done.

     LaunchPlatformAppWithNoData(profile, extension);
     return;
   }
 
-  // TODO(benwells): add a command-line argument to provide a handler ID.
-  LaunchPlatformAppWithPath(profile, extension, path);
+  base::FilePath file_path(command_line.GetArgs()[0]);
+  // launcher will be freed when nothing has a reference to it. The message

[tapted, 2014/05/12 00:07:51]

nit: this comment seems like it should be on the constructor rather than
repeated each call.. here I'd maybe say `|launcher| manages its own lifetime.`
to direct someone to more precise comments on the class.

[benwells, 2014/05/12 00:18:51]

Yep, I'll move it. I remember adding this comment when I had just figured out
how this stuff worked.

[benwells, 2014/05/12 05:51:05]

Actually there is already a comment there :)

+  // queue will retain a reference for any outstanding task, so when the
+  // launcher has finished it will be freed.
+  scoped_refptr<PlatformAppPathLauncher> launcher =
+      new PlatformAppPathLauncher(profile, extension, file_path);
+  launcher->LaunchWithRelativePath(current_directory);

[tapted, 2014/05/12 00:07:51]

You could also do

  base::PostTaskAndReplyWithResult(
      BrowserThread::GetBlockingPool(),
      FROM_HERE,
      base::Bind(GetAbsolutePathFromCommandLine,
                 command_line,
                 current_directory),
      base::Bind(LaunchPlatformAppWithPath, profile, extension));

It would even properly wrap the refptr. You'd just need to change
GetAbsolutePathFromCommandLine to return a base::FilePath

But also I think I like making PlatformAppPathLauncher more of a thing.

 }
 
 void LaunchPlatformAppWithPath(Profile* profile,