gdata: Move GDataFileSystem::AddUploadedFile() call out of uploader and into GDataDownloadObserver.

chrome/browser/chromeos/gdata/gdata_download_observer.cc

Index: chrome/browser/chromeos/gdata/gdata_download_observer.cc
diff --git a/chrome/browser/chromeos/gdata/gdata_download_observer.cc b/chrome/browser/chromeos/gdata/gdata_download_observer.cc
index d3989fdf569d4a26dbebf7a144cbd4790f55cafb..4362e28f000d319c4d436a9b1c12baee065fa8cc 100644
--- a/chrome/browser/chromeos/gdata/gdata_download_observer.cc
+++ b/chrome/browser/chromeos/gdata/gdata_download_observer.cc
@@ -30,6 +30,7 @@ const int64 kStreamingFileSize = 1 << 20;  // 1MB
 // Keys for DownloadItem::ExternalData.
 const char kUploadingKey[] = "Uploading";
 const char kGDataPathKey[] = "GDataPath";
+const char kCachePathKey[] = "CachePath";
 
 // External Data stored in DownloadItem for ongoing uploads.
 class UploadingExternalData : public DownloadCompletionBlocker {
@@ -62,6 +63,33 @@ class GDataExternalData : public DownloadItem::ExternalData {
   FilePath file_path_;
 };
 
+// External Data stored in DownloadItem for information required to move the
+// downloaded file to gdata cache.
+class GDataCacheExternalData : public DownloadItem::ExternalData {
+ public:
+  explicit GDataCacheExternalData(const FilePath& virtual_dir_path,
+                                  const FilePath& file_content_path,
+                                  scoped_ptr<DocumentEntry> entry)
+      : virtual_dir_path_(virtual_dir_path),
+        file_content_path_(file_content_path),
+        entry_(entry.release()),
+        move_started_(false) {
+  }
+  virtual ~GDataCacheExternalData() {}
+
+  const FilePath& virtual_dir_path() const { return virtual_dir_path_; }
+  const FilePath& file_content_path() const { return file_content_path_; }
+  DocumentEntry* entry() const { return entry_.get(); }
+  bool move_started() const { return move_started_; }
+  void set_move_started(bool move_started) { move_started_ = move_started; }
+
+ private:
+  FilePath virtual_dir_path_;
+  FilePath file_content_path_;
+  scoped_ptr<DocumentEntry> entry_;
+  bool move_started_;
+};
+
 // Extracts UploadingExternalData* from |download|.
 UploadingExternalData* GetUploadingExternalData(DownloadItem* download) {
   return static_cast<UploadingExternalData*>(
@@ -74,6 +102,12 @@ GDataExternalData* GetGDataExternalData(DownloadItem* download) {
       download->GetExternalData(&kGDataPathKey));
 }
 
+// Extracts GDataCacheExternalData* from |download|.
+GDataCacheExternalData* GetGDataCacheExternalData(DownloadItem* download) {
+  return static_cast<GDataCacheExternalData*>(
+      download->GetExternalData(&kCachePathKey));
+}
+
 void RunSubstituteGDataDownloadCallback(
     const GDataDownloadObserver::SubstituteGDataDownloadPathCallback& callback,
     const FilePath* file_path) {
@@ -169,8 +203,11 @@ void OnAuthenticate(Profile* profile,
 
 }  // namespace
 
-GDataDownloadObserver::GDataDownloadObserver()
-    : gdata_uploader_(NULL),
+GDataDownloadObserver::GDataDownloadObserver(
+    GDataUploader* uploader,
+    GDataFileSystem* file_system)
+    : gdata_uploader_(uploader),
+      file_system_(file_system),
       download_manager_(NULL),
       ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)) {
 }
@@ -186,12 +223,9 @@ GDataDownloadObserver::~GDataDownloadObserver() {
 }
 
 void GDataDownloadObserver::Initialize(
-    GDataUploader* gdata_uploader,
     DownloadManager* download_manager,
     const FilePath& gdata_tmp_download_path) {
-  DCHECK(gdata_uploader);
   DCHECK(!gdata_tmp_download_path.empty());
-  gdata_uploader_ = gdata_uploader;
   download_manager_ = download_manager;
   if (download_manager_)
     download_manager_->AddObserver(this);
@@ -353,7 +387,11 @@ void GDataDownloadObserver::OnDownloadUpdated(DownloadItem* download) {
 
     case DownloadItem::COMPLETE:
       UploadDownloadItem(download);
-      RemovePendingDownload(download);
+      MoveDownloadedFileToCache(
+          download,
+          base::Bind(&GDataDownloadObserver::RemovePendingDownload,
+                     weak_ptr_factory_.GetWeakPtr(),
+                     download));
       break;
 
     // TODO(achuith): Stop the pending upload and delete the file.
@@ -366,7 +404,6 @@ void GDataDownloadObserver::OnDownloadUpdated(DownloadItem* download) {
     default:
       NOTREACHED();
   }
-  DVLOG(1) << "Number of pending downloads=" << pending_downloads_.size();
 }
 
 void GDataDownloadObserver::AddPendingDownload(DownloadItem* download) {
@@ -380,6 +417,8 @@ void GDataDownloadObserver::AddPendingDownload(DownloadItem* download) {
              << ", full path=" << download->GetFullPath().value()
              << ", mime type=" << download->GetMimeType();
   }
+
+  DVLOG(1) << "Number of pending downloads=" << pending_downloads_.size();
 }
 
 void GDataDownloadObserver::RemovePendingDownload(DownloadItem* download) {
@@ -391,11 +430,14 @@ void GDataDownloadObserver::RemovePendingDownload(DownloadItem* download) {
     DetachFromDownload(download);
     pending_downloads_.erase(it);
   }
+
+  DVLOG(1) << "Number of pending downloads=" << pending_downloads_.size();
 }
 
 void GDataDownloadObserver::DetachFromDownload(DownloadItem* download) {
   download->SetExternalData(&kUploadingKey, NULL);
   download->SetExternalData(&kGDataPathKey, NULL);
+  download->SetExternalData(&kCachePathKey, NULL);
   download->RemoveObserver(this);
 }
 
@@ -474,6 +516,9 @@ void GDataDownloadObserver::OnUploadComplete(
     base::PlatformFileError error,
     scoped_ptr<UploadFileInfo> upload_file_info) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(upload_file_info.get());
+  DCHECK(upload_file_info->entry.get());
+
   DownloadMap::iterator iter = pending_downloads_.find(download_id);
   if (iter == pending_downloads_.end()) {
     DVLOG(1) << "Pending download not found" << download_id;
@@ -484,6 +529,41 @@ void GDataDownloadObserver::OnUploadComplete(
   UploadingExternalData* upload_data = GetUploadingExternalData(download_item);
   DCHECK(upload_data);
   upload_data->CompleteDownload();
+
+  // Fill GDataCacheExternalData struct for use by MoveDownloadedFileToCache().
+  // Note that |content_file_path| should use the final target path.
+  download_item->SetExternalData(&kCachePathKey,
+                                 new GDataCacheExternalData(
+                                     upload_file_info->gdata_path.DirName(),
+                                     download_item->GetTargetFilePath(),

[asanka, 2012/07/10 21:11:59]

This doesn't affect GData uploads currently, but it is possible for the target
path to change during the completion stage (i.e. if there's a name collision for
some types of downloads). So if you want to read the downloaded file, you want
to look at GetTargetFilePath() when the download item is in the COMPLETE state.

In other words, you could just call GetTargetFilePath() directly in
MoveDownloadedFileToCache().

[hshi1, 2012/07/10 21:32:55]

I see, I've moved the GetTargetFilePath() call to inside
MoveDownloadedFileToCache().

On 2012/07/10 21:11:59, asanka wrote:

+                                     upload_file_info->entry.Pass()));
+}
+
+void GDataDownloadObserver::MoveDownloadedFileToCache(
+    DownloadItem* download,
+    const base::Closure& callback) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(!callback.is_null());
+
+  GDataCacheExternalData* data = GetGDataCacheExternalData(download);
+  DCHECK(data);
+  if (data) {
+    // The observer may receive multiple COMPLETE notifications, so only

[asanka, 2012/07/10 21:11:59]

COMPLETE is a state. So you may receive multiple OnDownloadUpdated()
notifications while it is in this state. 

If possible, why not call RemovePendingDownload() after calling
AddUploadedFile()? That way you don't have to worry about multiple
notifications.

[hshi1, 2012/07/10 21:32:55]

Good point! Done.

On 2012/07/10 21:11:59, asanka wrote:

+    // initiate the move if it is not already started.
+    if (!data->move_started()) {
+      // Move downloaded file to gdata cache. Remove the pending download upon
+      // comletion of the move.
+      data->set_move_started(true);
+      file_system_->AddUploadedFile(UPLOAD_NEW_FILE,
+                                    data->virtual_dir_path(),
+                                    data->entry(),
+                                    data->file_content_path(),
+                                    GDataCache::FILE_OPERATION_MOVE,
+                                    callback);
+    }
+  } else {
+    callback.Run();
+  }
 }
 
 }  // namespace gdata