gdata: Convert public synchronous functions in GDataFileSystem to asynchronous.

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 6aab112b378d60b308f4cbfe0c54a7686e595035..1b742306a08a426a319c5cc8764962d291e9df1f 100644
--- a/chrome/browser/chromeos/extensions/file_handler_util.cc
+++ b/chrome/browser/chromeos/extensions/file_handler_util.cc
@@ -359,10 +359,14 @@ class FileTaskExecutor::ExecuteTasksFileSystemCallbackDispatcher {
         base::Bind(
             &FileTaskExecutor::ExecuteFileActionsOnUIThread,
             executor_,
-            file_system_name,
-            file_system_root,
             file_list,
-            handler_pid_));
+            base::Bind(
+                &FileTaskExecutor::OnInitAccessForExecuteFileActionsOnUIThread,
+                executor_,

[tbarzic, 2012/06/07 22:47:56]

why do you create callback here instead in ExecuteFileActionsOnUIThread, when we
actually call InitHandlerHostFileAccessPermissions?

[hshi1, 2012/06/08 00:22:05]

Because several parameters (such as file_system_name and file_system_root) are
only used by OnInitAccessForExecuteFileActionsOnUIThread but not in
InitHandlerHostFileAccessPermissions. By binding these parameters here I avoid
unnecessarily passing the parameters through ExecuteFileActionsOnUIThread.

[tbarzic, 2012/06/08 17:59:13]

Yeah, but this way it harder to follow what's going on.
I'd rather if we create the callback near the place the function that owns the
callback is invoked..

[hshi1, 2012/06/08 18:21:41]

Done.

+                file_system_name,
+                file_system_root,
+                file_list,
+                handler_pid_)));
   }
 
   void DidFail(base::PlatformFileError error_code) {
@@ -549,10 +553,8 @@ void FileTaskExecutor::ExecuteFailedOnUIThread() {
 }
 
 void FileTaskExecutor::ExecuteFileActionsOnUIThread(
-    const std::string& file_system_name,
-    const GURL& file_system_root,
     const FileDefinitionList& file_list,
-    int handler_pid) {
+    const ExecuteFileActionsCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   ExtensionService* service = profile_->GetExtensionService();
@@ -567,7 +569,17 @@ void FileTaskExecutor::ExecuteFileActionsOnUIThread(
     return;
   }
 
-  InitHandlerHostFileAccessPermissions(file_list, extension, action_id_);
+  InitHandlerHostFileAccessPermissions(file_list, extension, action_id_,
+                                       callback);
+}
+
+void FileTaskExecutor::OnInitAccessForExecuteFileActionsOnUIThread(
+    const std::string& file_system_name,
+    const GURL& file_system_root,
+    const FileDefinitionList& file_list,
+    int handler_pid,
+    const Extension* extension) {

[tbarzic, 2012/06/07 22:47:56]

what if the extension goes away while we were getting cache paths?

I'd rather just refetch extension from the service again that pass the pointer..

[hshi1, 2012/06/08 00:22:05]

Done.

+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (handler_pid > 0) {
     SetupPermissionsAndDispatchEvent(file_system_name, file_system_root,
@@ -648,9 +660,11 @@ void FileTaskExecutor::SetupPermissionsAndDispatchEvent(
 void FileTaskExecutor::InitHandlerHostFileAccessPermissions(
     const FileDefinitionList& file_list,
     const Extension* handler_extension,
-    const std::string& action_id) {
+    const std::string& action_id,
+    const ExecuteFileActionsCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  scoped_ptr<std::vector<FilePath> > gdata_paths(new std::vector<FilePath>);
   for (FileDefinitionList::const_iterator iter = file_list.begin();
        iter != file_list.end();
        ++iter) {
@@ -659,15 +673,18 @@ void FileTaskExecutor::InitHandlerHostFileAccessPermissions(
         iter->absolute_path,
         GetAccessPermissionsForHandler(handler_extension, action_id)));
 
-    if (!gdata::util::IsUnderGDataMountPoint(iter->absolute_path))
-      continue;
-
-    // If the file is on gdata mount point, we'll have to give handler host
-    // permissions for file's gdata cache paths.
-    // This has to be called on UI thread.
-    gdata::util::InsertGDataCachePathsPermissions(profile_, iter->absolute_path,
-        &handler_host_permissions_);
+    if (gdata::util::IsUnderGDataMountPoint(iter->absolute_path))
+      gdata_paths->push_back(iter->absolute_path);
   }
+
+  // For files on gdata mount point, we'll have to give handler host permissions
+  // for their cache paths. This has to be called on UI thread.
+  gdata::util::InsertGDataCachePathsPermissions(

[tbarzic, 2012/06/07 22:47:56]

We don't really have to go to gdata_util if there are no gdata files, so we
could do something like
if (gdata_paths->empty()) {
  run callback (or post callback to the current message loop)
  return;
}

this may be easier to follow.. and we wouldn't have to try getting gdata file
system..

[hshi1, 2012/06/08 00:22:05]

Done.

+      profile_,
+      gdata_paths.Pass(),
+      &handler_host_permissions_,
+      base::Bind(callback,

[tbarzic, 2012/06/07 22:47:56]

this fits in a single line

[hshi1, 2012/06/08 00:22:05]

Done.

+                 handler_extension));

[satorux1, 2012/06/07 22:47:17]

nit: merge the line to the previous line?

[hshi1, 2012/06/08 00:22:05]

Done.

 }
 
 void FileTaskExecutor::SetupHandlerHostFileAccessPermissions(int handler_pid) {