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

apps/launcher.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "apps/launcher.h"
 
 #include "apps/browser/api/app_runtime/app_runtime_api.h"
 #include "apps/browser/file_handler_util.h"
 #include "apps/common/api/app_runtime.h"
 #include "base/command_line.h"
@@ skipping 45 matching lines @@
 using extensions::Extension;
 using extensions::ExtensionHost;
 using extensions::ExtensionSystem;
 
 namespace apps {
 
 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;
 
   if (current_directory.empty()) {
     *file_path = base::MakeAbsoluteFilePath(*file_path);
     return !file_path->empty();
   }
 
   if (!current_directory.IsAbsolute())
     return false;
 
   *file_path = current_directory.Append(*file_path);
   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.
 void LaunchPlatformAppWithNoData(Profile* profile, const Extension* extension) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   AppEventRouter::DispatchOnLaunchedEvent(profile, extension);
 }
 
 // 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
@@ skipping 30 matching lines @@
     }
 
     OnFileValid();
   }
 
   void LaunchWithHandler(const std::string& handler_id) {
     handler_id_ = handler_id;
     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_)) {
       PlatformAppPathLauncher::GetMimeTypeAndLaunchForDriveFile();
       return;
     }
 #endif
 
     BrowserThread::PostTask(
         BrowserThread::FILE,
@@ skipping 138 matching lines @@
                         host->render_process_host()->GetID(),
                         file_path_,
                         false);
     AppEventRouter::DispatchOnLaunchedEventWithFileEntry(
         profile_, extension_, handler_id_, mime_type, file_entry);
   }
 
   // The profile the app should be run in.
   Profile* profile_;
   // 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_;
 
   DISALLOW_COPY_AND_ASSIGN(PlatformAppPathLauncher);
 };
 
 }  // namespace
 
 void LaunchPlatformAppWithCommandLine(Profile* profile,
                                       const Extension* extension,
                                       const CommandLine& command_line,
                                       const base::FilePath& current_directory) {
   // An app with "kiosk_only" should not be installed and launched
   // outside of ChromeOS kiosk mode in the first place. This is a defensive
   // check in case this scenario does occur.
   if (extensions::KioskModeInfo::IsKioskOnly(extension)) {
     bool in_kiosk_mode = false;
 #if defined(OS_CHROMEOS)
     chromeos::UserManager* user_manager = chromeos::UserManager::Get();
     in_kiosk_mode = user_manager && user_manager->IsLoggedInAsKioskApp();
 #endif
     if (!in_kiosk_mode) {
       LOG(ERROR) << "App with 'kiosk_only' attribute must be run in "
           << " ChromeOS kiosk mode.";
       NOTREACHED();
       return;
     }
   }
 
-  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,
                                const Extension* extension,
                                const base::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<PlatformAppPathLauncher> launcher =
       new PlatformAppPathLauncher(profile, extension, file_path);
@@ skipping 42 matching lines @@
 void LaunchPlatformAppWithUrl(Profile* profile,
                               const Extension* extension,
                               const std::string& handler_id,
                               const GURL& url,
                               const GURL& referrer_url) {
   AppEventRouter::DispatchOnLaunchedEventWithUrl(
       profile, extension, handler_id, url, referrer_url);
 }
 
 }  // namespace apps