Merge 143559 - gdata: Stop getting GDataUploader via GDataSystemService

For writing upload related unit tests in gdata_file_system_unittest.cc,
we need to be able to inject a mock GDataUploader.
Otherwise, GDataFileSystem ends up using a production GDataUploader
that makes real HTTP requests to the gdata server.

Note that this is a stop-gap solution. The inter-dependencies between
GDataUploader and GDataSystemService should be eliminated.

BUG=127080,133860
TEST=uploading works as before


Review URL: https://chromiumcodereview.appspot.com/10640006

TBR=satorux@chromium.org
Review URL: https://chromiumcodereview.appspot.com/10698019

git-svn-id: svn://svn.chromium.org/chrome/branches/1180/src@144558 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 68 insertions, 22 deletions

diff --git a/chrome/browser/chromeos/gdata/gdata_file_system.cc b/chrome/browser/chromeos/gdata/gdata_file_system.cc
index 230ae9d..2d17cca 100644
--- a/chrome/browser/chromeos/gdata/gdata_file_system.cc
+++ b/chrome/browser/chromeos/gdata/gdata_file_system.cc
@@ -692,9 +692,11 @@ GDataFileSystem::GDataFileSystem(
     Profile* profile,
     GDataCache* cache,
     DocumentsServiceInterface* documents_service,
+    GDataUploaderInterface* uploader,
     const base::SequencedWorkerPool::SequenceToken& sequence_token)
     : profile_(profile),
       cache_(cache),
+      uploader_(uploader),
       documents_service_(documents_service),
       update_timer_(true /* retain_user_task */, true /* is_repeating */),
       hide_hosted_docs_(false),
@@ -1127,14 +1129,6 @@ void GDataFileSystem::StartFileUploadOnUIThread(
   DCHECK(error);
   DCHECK(upload_file_info);
 
-  GDataSystemService* service =
-      GDataSystemServiceFactory::GetForProfile(profile_);
-
-  if (*error == base::PLATFORM_FILE_OK) {
-    if (!service)
-      *error = base::PLATFORM_FILE_ERROR_FAILED;
-  }
-
   if (*error != base::PLATFORM_FILE_OK) {
     if (!callback.is_null())
       callback.Run(*error);
@@ -1147,7 +1141,7 @@ void GDataFileSystem::StartFileUploadOnUIThread(
                  ui_weak_ptr_,
                  callback);
 
-  service->uploader()->UploadFile(scoped_ptr<UploadFileInfo>(upload_file_info));
+  uploader_->UploadFile(scoped_ptr<UploadFileInfo>(upload_file_info));
 }
 
 void GDataFileSystem::OnTransferCompleted(
diff --git a/chrome/browser/chromeos/gdata/gdata_file_system.h b/chrome/browser/chromeos/gdata/gdata_file_system.h
index aedff7b4..f47a98e 100644
--- a/chrome/browser/chromeos/gdata/gdata_file_system.h
+++ b/chrome/browser/chromeos/gdata/gdata_file_system.h
@@ -362,6 +362,7 @@ class GDataFileSystem : public GDataFileSystemInterface,
       Profile* profile,
       GDataCache* cache,
       DocumentsServiceInterface* documents_service,
+      GDataUploaderInterface* uploader,
       const base::SequencedWorkerPool::SequenceToken& sequence_token);
   virtual ~GDataFileSystem();
 
@@ -1135,6 +1136,9 @@ class GDataFileSystem : public GDataFileSystemInterface,
   // The cache owned by GDataSystemService.
   GDataCache* cache_;
 
+  // The uploader owned by GDataSystemService.
+  GDataUploaderInterface* uploader_;
+
   // The document service owned by GDataSystemService.
   DocumentsServiceInterface* documents_service_;
 
diff --git a/chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc b/chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc
index 2795e68..a2c59ee 100644
--- a/chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc
+++ b/chrome/browser/chromeos/gdata/gdata_file_system_unittest.cc
@@ -159,6 +159,20 @@ class MockFreeDiskSpaceGetter : public FreeDiskSpaceGetterInterface {
   MOCK_CONST_METHOD0(AmountOfFreeDiskSpace, int64());
 };
 
+class MockGDataUploader : public GDataUploaderInterface {
+ public:
+  virtual ~MockGDataUploader() {}
+  // This function is not mockable by gmock.
+  virtual int UploadFile(
+      scoped_ptr<UploadFileInfo> upload_file_info) OVERRIDE {
+    return -1;
+  }
+
+  MOCK_METHOD2(UpdateUpload, void(int upload_id,
+                                  content::DownloadItem* download));
+  MOCK_CONST_METHOD1(GetUploadedBytes, int64(int upload_id));
+};
+
 class GDataFileSystemTest : public testing::Test {
  protected:
   GDataFileSystemTest()
@@ -166,6 +180,7 @@ class GDataFileSystemTest : public testing::Test {
         io_thread_(content::BrowserThread::IO),
         sequence_token_(
             content::BrowserThread::GetBlockingPool()->GetSequenceToken()),
+        cache_(NULL),
         file_system_(NULL),
         mock_doc_service_(NULL),
         num_callback_invocations_(0),
@@ -199,10 +214,13 @@ class GDataFileSystemTest : public testing::Test {
         content::BrowserThread::GetBlockingPool(),
         sequence_token_);
 
+    mock_uploader_.reset(new StrictMock<MockGDataUploader>);
+
     ASSERT_FALSE(file_system_);
     file_system_ = new GDataFileSystem(profile_.get(),
                                        cache_,
                                        mock_doc_service_,
+                                       mock_uploader_.get(),
                                        sequence_token_);
 
     mock_sync_client_.reset(new StrictMock<MockGDataSyncClient>);
@@ -1269,6 +1287,7 @@ class GDataFileSystemTest : public testing::Test {
   scoped_ptr<TestingProfile> profile_;
   scoped_refptr<CallbackHelper> callback_helper_;
   GDataCache* cache_;
+  scoped_ptr<StrictMock<MockGDataUploader> > mock_uploader_;
   GDataFileSystem* file_system_;
   MockDocumentsService* mock_doc_service_;
   MockFreeDiskSpaceGetter* mock_free_disk_space_checker_;
diff --git a/chrome/browser/chromeos/gdata/gdata_system_service.cc b/chrome/browser/chromeos/gdata/gdata_system_service.cc
index 3fd362e..53f021c 100644
--- a/chrome/browser/chromeos/gdata/gdata_system_service.cc
+++ b/chrome/browser/chromeos/gdata/gdata_system_service.cc
@@ -34,15 +34,20 @@ GDataSystemService::GDataSystemService(Profile* profile)
           BrowserThread::GetBlockingPool(),
           sequence_token_)),
       documents_service_(new DocumentsService),
+      uploader_(new GDataUploader(docs_service())),
       file_system_(new GDataFileSystem(profile,
                                        cache(),
                                        docs_service(),
+                                       uploader(),
                                        sequence_token_)),
-      uploader_(new GDataUploader(file_system(), docs_service())),
       download_observer_(new GDataDownloadObserver),
       sync_client_(new GDataSyncClient(profile, file_system(), cache())),
       webapps_registry_(new DriveWebAppsRegistry) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  // TODO(satorux): The dependency to GDataFileSystem should be
+  // eliminated. http://crbug.com/133860
+  uploader_->set_file_system(file_system());
 }
 
 GDataSystemService::~GDataSystemService() {
@@ -74,8 +79,8 @@ void GDataSystemService::Shutdown() {
   webapps_registry_.reset();
   sync_client_.reset();
   download_observer_.reset();
-  uploader_.reset();
   file_system_.reset();
+  uploader_.reset();
   documents_service_.reset();
 }
 
diff --git a/chrome/browser/chromeos/gdata/gdata_system_service.h b/chrome/browser/chromeos/gdata/gdata_system_service.h
index 833806e2..cb21314 100644
--- a/chrome/browser/chromeos/gdata/gdata_system_service.h
+++ b/chrome/browser/chromeos/gdata/gdata_system_service.h
@@ -60,8 +60,8 @@ class GDataSystemService : public ProfileKeyedService  {
   const base::SequencedWorkerPool::SequenceToken sequence_token_;
   GDataCache* cache_;
   scoped_ptr<DocumentsServiceInterface> documents_service_;
-  scoped_ptr<GDataFileSystem> file_system_;
   scoped_ptr<GDataUploader> uploader_;
+  scoped_ptr<GDataFileSystem> file_system_;
   scoped_ptr<GDataDownloadObserver> download_observer_;
   scoped_ptr<GDataSyncClient> sync_client_;
   scoped_ptr<DriveWebAppsRegistry> webapps_registry_;
diff --git a/chrome/browser/chromeos/gdata/gdata_uploader.cc b/chrome/browser/chromeos/gdata/gdata_uploader.cc
index b0bffd1..366128d 100644
--- a/chrome/browser/chromeos/gdata/gdata_uploader.cc
+++ b/chrome/browser/chromeos/gdata/gdata_uploader.cc
@@ -31,9 +31,8 @@ const int kMaxFileOpenTries = 5;
 
 namespace gdata {
 
-GDataUploader::GDataUploader(GDataFileSystem* file_system,
-                             DocumentsServiceInterface* documents_service)
-  : file_system_(file_system),
+GDataUploader::GDataUploader(DocumentsServiceInterface* documents_service)
+  : file_system_(NULL),
     documents_service_(documents_service),
     next_upload_id_(0),
     ALLOW_THIS_IN_INITIALIZER_LIST(uploader_factory_(this)) {
diff --git a/chrome/browser/chromeos/gdata/gdata_uploader.h b/chrome/browser/chromeos/gdata/gdata_uploader.h
index 6a6be7a..7e9a47b 100644
--- a/chrome/browser/chromeos/gdata/gdata_uploader.h
+++ b/chrome/browser/chromeos/gdata/gdata_uploader.h
@@ -27,21 +27,46 @@ class GDataFileSystem;
 class DocumentsServiceInterface;
 struct UploadFileInfo;
 
-class GDataUploader {
+class GDataUploaderInterface {
  public:
-  explicit GDataUploader(GDataFileSystem* file_system,
-                         DocumentsServiceInterface* documents_service);
-  virtual ~GDataUploader();
+  ~GDataUploaderInterface() {}
 
   // Uploads a file specified by |upload_file_info|. Transfers ownership.
   // Returns the upload_id.
-  int UploadFile(scoped_ptr<UploadFileInfo> upload_file_info);
+  //
+  // WARNING: This is not mockable by gmock because it takes scoped_ptr<>.
+  // See "Announcing scoped_ptr<>::Pass(). The latest in pointer ownership
+  // technology!" thread on chromium-dev.
+  virtual int UploadFile(scoped_ptr<UploadFileInfo> upload_file_info) = 0;
 
   // Updates attributes of streaming upload.
-  void UpdateUpload(int upload_id, content::DownloadItem* download);
+  virtual void UpdateUpload(int upload_id,
+                            content::DownloadItem* download) = 0;
 
   // Returns the count of bytes confirmed as uploaded so far.
-  int64 GetUploadedBytes(int upload_id) const;
+  virtual int64 GetUploadedBytes(int upload_id) const = 0;
+};
+
+class GDataUploader : public GDataUploaderInterface {
+ public:
+  explicit GDataUploader(DocumentsServiceInterface* documents_service);
+  virtual ~GDataUploader();
+
+  // Sets the file system. This must be called before calling other member
+  // functions.
+  //
+  // TODO(satorux): The dependency to GDataFileSystem should be
+  // eliminated. http://crbug.com/133860
+  void set_file_system(GDataFileSystem* file_system) {
+    file_system_ = file_system;
+  }
+
+  // GDataUploaderInterface overrides.
+  virtual int UploadFile(
+      scoped_ptr<UploadFileInfo> upload_file_info) OVERRIDE;
+  virtual void UpdateUpload(
+      int upload_id, content::DownloadItem* download) OVERRIDE;
+  virtual int64 GetUploadedBytes(int upload_id) const OVERRIDE;
 
  private:
   // Lookup UploadFileInfo* in pending_uploads_.