gdata: Convert public synchronous functions in GDataFileSystem to asynchronous.

chrome/browser/chromeos/extensions/file_handler_util.cc

 // Copyright (c) 2012 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 "chrome/browser/chromeos/extensions/file_handler_util.h"
 
 #include "base/bind.h"
 #include "base/file_util.h"
 #include "base/i18n/case_conversion.h"
 #include "base/json/json_writer.h"
@@ skipping 341 matching lines @@
               &FileTaskExecutor::ExecuteFailedOnUIThread,
               executor_));
       return;
     }
 
     BrowserThread::PostTask(
         BrowserThread::UI, FROM_HERE,
         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) {
     BrowserThread::PostTask(
         BrowserThread::UI, FROM_HERE,
         base::Bind(
             &FileTaskExecutor::ExecuteFailedOnUIThread,
             executor_));
   }
 
@@ skipping 166 matching lines @@
           handler_pid,
           action_id_,
           file_urls));
 }
 
 void FileTaskExecutor::ExecuteFailedOnUIThread() {
   Done(false);
 }
 
 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();
   if (!service) {
     Done(false);
     return;
   }
 
   const Extension* extension = service->GetExtensionById(extension_id_, false);
   if (!extension) {
     Done(false);
     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,
         file_list, handler_pid, NULL);
   } else {
     // We have to wake the handler background page before we proceed.
     extensions::LazyBackgroundTaskQueue* queue =
         ExtensionSystem::Get(profile_)->lazy_background_task_queue();
     if (!queue->ShouldEnqueueTask(profile_, extension)) {
       Done(false);
@@ skipping 60 matching lines @@
       extension_id_, std::string("fileBrowserHandler.onExecute"),
       json_args, profile_,
       GURL());
 
   Done(true);
 }
 
 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) {
     // Setup permission for file's absolute file.
     handler_host_permissions_.push_back(std::make_pair(
         iter->absolute_path,
         GetAccessPermissionsForHandler(handler_extension, action_id)));
 
-    if (!gdata::util::IsUnderGDataMountPoint(iter->absolute_path))
-      continue;
+    if (gdata::util::IsUnderGDataMountPoint(iter->absolute_path))
+      gdata_paths->push_back(iter->absolute_path);
+  }
 
-    // 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_);
-  }
+  // 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) {
   for (size_t i = 0; i < handler_host_permissions_.size(); i++) {
     content::ChildProcessSecurityPolicy::GetInstance()->GrantPermissionsForFile(
         handler_pid,
         handler_host_permissions_[i].first,
         handler_host_permissions_[i].second);
   }
 
   // We don't need this anymore.
   handler_host_permissions_.clear();
 }
 
 } // namespace file_handler_util