gdata: Convert GDataFileSystem::AddUploadFile() to asynchronous.

chrome/browser/chromeos/gdata/gdata_file_system.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/gdata/gdata_file_system.h"
 
 #include <set>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 249 matching lines @@
     const std::string& resource_id,
     const std::string& md5,
     FilePath* cache_file_path) {
   DCHECK(error);
   DCHECK(cache_file_path);
 
   if (!callback.is_null())
     callback.Run(*error, resource_id, md5, *cache_file_path);
 }
 
+// Callback for StoreToCache invoked by AddUploadedFileOnUIThread.
+void OnStoreToCacheForAddUploadedFile(
+    const base::Closure& callback,
+    base::PlatformFileError /* error */,
+    const std::string& /* resource_id */,
+    const std::string& /* md5 */) {
+  if (!callback.is_null())

[achuithb, 2012/06/14 22:27:18]

could we add a DCHECK for UI thread here?

[hshi1, 2012/06/15 00:16:28]

Done.

+    callback.Run();
+}
+
 void RunGetCacheStateCallbackHelper(
     const GetCacheStateCallback& callback,
     base::PlatformFileError* error,
     int* cache_state) {
   DCHECK(error);
   DCHECK(cache_state);
 
   if (!callback.is_null())
     callback.Run(*error, *cache_state);
 }
@@ skipping 884 matching lines @@
       base::Bind(&GDataFileSystem::OnTransferCompleted,
                  ui_weak_ptr_,
                  callback);
 
   service->uploader()->UploadFile(scoped_ptr<UploadFileInfo>(upload_file_info));
 }
 
 void GDataFileSystem::OnTransferCompleted(
     const FileOperationCallback& callback,
     base::PlatformFileError error,
     UploadFileInfo* upload_file_info) {

[achuithb, 2012/06/14 22:27:18]

We are getting ownership of upload_file_info here. Could we use scoped_ptr
instead?

[hshi1, 2012/06/15 00:16:28]

Done.

   DCHECK(upload_file_info);
   if (error == base::PLATFORM_FILE_OK && upload_file_info->entry.get()) {
     AddUploadedFile(upload_file_info->gdata_path.DirName(),
                     upload_file_info->entry.get(),
                     upload_file_info->file_path,
-                    GDataCache::FILE_OPERATION_COPY);
+                    GDataCache::FILE_OPERATION_COPY,
+                    base::Bind(&GDataFileSystem::OnAddUploadFileCompleted,
+                               ui_weak_ptr_,
+                               callback,
+                               error,
+                               upload_file_info));
+  } else {
+    OnAddUploadFileCompleted(callback, error, upload_file_info);
   }
+}
+
+void GDataFileSystem::OnAddUploadFileCompleted(
+    const FileOperationCallback& callback,
+    base::PlatformFileError error,
+    UploadFileInfo* upload_file_info) {

[achuithb, 2012/06/14 22:27:18]

could we used scoped_ptr instead?

[hshi1, 2012/06/15 00:16:28]

Done.

+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
   if (!callback.is_null())
     callback.Run(error);
 
   // In case of error upload_file_info will be deleted by the uploader.

[achuithb, 2012/06/14 22:27:18]

This is really weird (deletion by uploader in case of error, and by us in case
of success). Is there a way to fix this? If not, add a TODO and just release?

[hshi1, 2012/06/15 00:16:28]

This is no longer necessary as the pointer gets automatically deleted when the
scoped_ptr is no longer passed along.

   if (error != base::PLATFORM_FILE_OK)
     return;
 
-  // TODO(achuith): GDataFileSystem should not have to call DeleteUpload.
-  GDataSystemService* service =
-      GDataSystemServiceFactory::GetForProfile(profile_);
-  if (service)
-    service->uploader()->DeleteUpload(upload_file_info);
+  delete upload_file_info;
 }
 
 void GDataFileSystem::Copy(const FilePath& src_file_path,
                            const FilePath& dest_file_path,
                            const FileOperationCallback& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI) ||
          BrowserThread::CurrentlyOn(BrowserThread::IO));
   RunTaskOnUIThread(base::Bind(&GDataFileSystem::CopyOnUIThread,
                                ui_weak_ptr_,
                                src_file_path,
@@ skipping 2319 matching lines @@
     return base::PLATFORM_FILE_ERROR_NOT_FOUND;
 
   NotifyDirectoryChanged(parent_dir->GetFilePath());
   return base::PLATFORM_FILE_OK;
 }
 
 void GDataFileSystem::AddUploadedFile(
     const FilePath& virtual_dir_path,
     DocumentEntry* entry,
     const FilePath& file_content_path,
-    GDataCache::FileOperationType cache_operation) {
+    GDataCache::FileOperationType cache_operation,
+    const base::Closure& callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  // Post a task to the same thread, rather than calling it here, as

[achuithb, 2012/06/14 22:33:33]

I'm confused. Why are we doing this?

[hshi1, 2012/06/15 00:16:28]

This is the purpose of this CL and this bug - to make AddUploadedFile an
asynchronous function. By posting a task to AddUploadedFileOnUIThread(), we are
able to return immediately while the task can run in the background.

[achuithb, 2012/06/15 01:27:33]

I've lost sight of the big picture on why we're doing this?

+  // AddUploadedFile() is asynchronous.
+  base::MessageLoopProxy::current()->PostTask(
+      FROM_HERE,
+      base::Bind(&GDataFileSystem::AddUploadedFileOnUIThread,
+                 ui_weak_ptr_,
+                 virtual_dir_path,
+                 entry,
+                 file_content_path,
+                 cache_operation,
+                 callback));
+}
+
+void GDataFileSystem::AddUploadedFileOnUIThread(
+    const FilePath& virtual_dir_path,
+    DocumentEntry* entry,
+    const FilePath& file_content_path,
+    GDataCache::FileOperationType cache_operation,
+    const base::Closure& callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(!callback.is_null());
+
   if (!entry) {
     NOTREACHED();
+    callback.Run();
     return;
   }
 
   std::string resource_id;
   std::string md5;
   {
     base::AutoLock lock(lock_);
     GDataEntry* dir_entry = GetGDataEntryByPath(virtual_dir_path);
-    if (!dir_entry)
+    if (!dir_entry) {
+      callback.Run();
       return;
+    }
 
     GDataDirectory* parent_dir  = dir_entry->AsGDataDirectory();
-    if (!parent_dir)
+    if (!parent_dir) {
+      callback.Run();
       return;
+    }
 
     scoped_ptr<GDataEntry> new_entry(
         GDataEntry::FromDocumentEntry(parent_dir, entry, root_.get()));
-    if (!new_entry.get())
+    if (!new_entry.get()) {
+      callback.Run();
       return;
+    }
 
     GDataFile* file = new_entry->AsGDataFile();
     DCHECK(file);
     resource_id = file->resource_id();
     md5 = file->file_md5();
     parent_dir->AddEntry(new_entry.release());
   }
   NotifyDirectoryChanged(virtual_dir_path);
 
   StoreToCache(resource_id, md5, file_content_path, cache_operation,
-               CacheOperationCallback());
+               base::Bind(&OnStoreToCacheForAddUploadedFile,
+                          callback));
 }
 
 void GDataFileSystem::Observe(int type,
                               const content::NotificationSource& source,
                               const content::NotificationDetails& details) {
   if (type == chrome::NOTIFICATION_PREF_CHANGED) {
     PrefService* pref_service = profile_->GetPrefs();
     std::string* pref_name = content::Details<std::string>(details).ptr();
     if (*pref_name == prefs::kDisableGDataHostedFiles) {
       SetHideHostedDocuments(
@@ skipping 574 matching lines @@
     base::PlatformFileError error,
     const std::string& resource_id,
     const std::string& md5) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (!callback.is_null())
     callback.Run(error);
 }
 
 }  // namespace gdata