[SyncFS] Split SyncEngine and SyncEngineContext

Following member values of SyncEngine class migrate to
SyncEngineContext, and they are accessed through getters.

-scoped_ptr<drive::DriveServiceInterface> drive_service_;
-scoped_ptr<drive::DriveUploaderInterface> drive_uploader_;
-scoped_ptr<MetadataDatabase> metadata_database_;
-RemoteChangeProcessor* remote_change_processor_;
-scoped_refptr<base::SequencedTaskRunner> task_runner_;



BUG=347425
NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260166

[peria, 2014-03-27 07:02:43]

tzik@
would you take a look, as a starting point?

[tzik, 2014-03-27 07:41:03]

https://codereview.chromium.org/213473008/diff/80001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/sync_engine.h (right):

https://codereview.chromium.org/213473008/diff/80001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_engine.h:133: //
SyncEngineContext overrides.
drop this comment?

https://codereview.chromium.org/213473008/diff/80001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/sync_engine_context.h
(right):

https://codereview.chromium.org/213473008/diff/80001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_engine_context.h:14: }  //
namespace base
We usually don't add "// namespace xxx" for forward decl only namespace.
Could you drop this for consistency?

https://codereview.chromium.org/213473008/diff/80001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_engine_context.h:32:
~SyncEngineContext() {}
I think these ctor and dtor need to be out-of-line now.
Then, we can revert new #include's, e.g. conflict_resolver.cc.

https://codereview.chromium.org/213473008/diff/80001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_engine_context.h:40: //
TODO(peria): Remove those setters.
Could you address this in this CL?

[peria, 2014-03-27 10:14:36]

https://codereview.chromium.org/213473008/diff/80001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/sync_engine.h (right):

https://codereview.chromium.org/213473008/diff/80001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_engine.h:133: //
SyncEngineContext overrides.
On 2014/03/27 07:41:03, tzik wrote:
> drop this comment?

Done.

https://codereview.chromium.org/213473008/diff/80001/chrome/browser/sync_file...
File chrome/browser/sync_file_system/drive_backend/sync_engine_context.h
(right):

https://codereview.chromium.org/213473008/diff/80001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_engine_context.h:14: }  //
namespace base
On 2014/03/27 07:41:03, tzik wrote:
> We usually don't add "// namespace xxx" for forward decl only namespace.
> Could you drop this for consistency?

Done.

https://codereview.chromium.org/213473008/diff/80001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_engine_context.h:32:
~SyncEngineContext() {}
On 2014/03/27 07:41:03, tzik wrote:
> I think these ctor and dtor need to be out-of-line now.
> Then, we can revert new #include's, e.g. conflict_resolver.cc.

Done.

https://codereview.chromium.org/213473008/diff/80001/chrome/browser/sync_file...
chrome/browser/sync_file_system/drive_backend/sync_engine_context.h:40: //
TODO(peria): Remove those setters.
On 2014/03/27 07:41:03, tzik wrote:
> Could you address this in this CL?

Done.
Setter for MetadataDatabase is needed in the product,
and setter RemoteChangeProcessor is needed in unit tests.

[tzik, 2014-03-27 11:24:17]

https://codereview.chromium.org/213473008/diff/200001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/conflict_resolver.cc (right):

https://codereview.chromium.org/213473008/diff/200001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/conflict_resolver.cc:11: #include
"base/sequenced_task_runner.h"
I think all #include additions are not needed.

https://codereview.chromium.org/213473008/diff/200001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/conflict_resolver_unittest.cc
(right):

https://codereview.chromium.org/213473008/diff/200001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/conflict_resolver_unittest.cc:59:
fake_drive_service.reset(new FakeDriveServiceWrapper);
Just like below should work:
scoped_ptr<FakeDriveServiceWrapper> fake_drive_service(
  new FakeDriveServiceWrapper);

https://codereview.chromium.org/213473008/diff/200001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/conflict_resolver_unittest.cc:66:
drive_uploader.reset(new FakeDriveUploader(fake_drive_service.get()));
How about rewrite like this? So that we don't need to use PassAs for this.

scoped_ptr<DriveUploaderInterface> drive_uploader(
  new FakeDriveUploader(fake_drive_service.get()));

https://codereview.chromium.org/213473008/diff/200001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/list_changes_task_unittest.cc
(right):

https://codereview.chromium.org/213473008/diff/200001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/list_changes_task_unittest.cc:45:
fake_drive_service.reset(new drive::FakeDriveService);
ditto to conflict_resolver_unittest.cc.

https://codereview.chromium.org/213473008/diff/200001/chrome/browser/sync_fil...
File
chrome/browser/sync_file_system/drive_backend/local_to_remote_syncer_unittest.cc
(right):

https://codereview.chromium.org/213473008/diff/200001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/local_to_remote_syncer_unittest.cc:59:
fake_drive_service.reset(new FakeDriveServiceWrapper);
ditto

https://codereview.chromium.org/213473008/diff/200001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_engine_context.h
(right):

https://codereview.chromium.org/213473008/diff/200001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine_context.h:52:
scoped_refptr<base::SequencedTaskRunner> task_runner_;  // Do not own
this comment seems not accurate.

[peria, 2014-03-28 03:20:23]

https://codereview.chromium.org/213473008/diff/200001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/conflict_resolver.cc (right):

https://codereview.chromium.org/213473008/diff/200001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/conflict_resolver.cc:11: #include
"base/sequenced_task_runner.h"
On 2014/03/27 11:24:17, tzik wrote:
> I think all #include additions are not needed.

Done.

https://codereview.chromium.org/213473008/diff/200001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/conflict_resolver_unittest.cc
(right):

https://codereview.chromium.org/213473008/diff/200001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/conflict_resolver_unittest.cc:59:
fake_drive_service.reset(new FakeDriveServiceWrapper);
On 2014/03/27 11:24:17, tzik wrote:
> Just like below should work:
> scoped_ptr<FakeDriveServiceWrapper> fake_drive_service(
>   new FakeDriveServiceWrapper);

Done.

https://codereview.chromium.org/213473008/diff/200001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/conflict_resolver_unittest.cc:66:
drive_uploader.reset(new FakeDriveUploader(fake_drive_service.get()));
On 2014/03/27 11:24:17, tzik wrote:
> How about rewrite like this? So that we don't need to use PassAs for this.
> 
> scoped_ptr<DriveUploaderInterface> drive_uploader(
>   new FakeDriveUploader(fake_drive_service.get()));

Done.

https://codereview.chromium.org/213473008/diff/200001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/list_changes_task_unittest.cc
(right):

https://codereview.chromium.org/213473008/diff/200001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/list_changes_task_unittest.cc:45:
fake_drive_service.reset(new drive::FakeDriveService);
On 2014/03/27 11:24:17, tzik wrote:
> ditto to conflict_resolver_unittest.cc.

Done.

https://codereview.chromium.org/213473008/diff/200001/chrome/browser/sync_fil...
File
chrome/browser/sync_file_system/drive_backend/local_to_remote_syncer_unittest.cc
(right):

https://codereview.chromium.org/213473008/diff/200001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/local_to_remote_syncer_unittest.cc:59:
fake_drive_service.reset(new FakeDriveServiceWrapper);
On 2014/03/27 11:24:17, tzik wrote:
> ditto

Done.

https://codereview.chromium.org/213473008/diff/200001/chrome/browser/sync_fil...
File chrome/browser/sync_file_system/drive_backend/sync_engine_context.h
(right):

https://codereview.chromium.org/213473008/diff/200001/chrome/browser/sync_fil...
chrome/browser/sync_file_system/drive_backend/sync_engine_context.h:52:
scoped_refptr<base::SequencedTaskRunner> task_runner_;  // Do not own
On 2014/03/27 11:24:17, tzik wrote:
> this comment seems not accurate.

Done.

[tzik, 2014-03-28 03:24:03]

Thank! LGTM!

[peria, 2014-03-28 03:36:21]

The CQ bit was checked by peria@chromium.org

[commit-bot: I haz the power, 2014-03-28 03:36:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/peria@chromium.org/213473008/260001

[commit-bot: I haz the power, 2014-03-28 03:37:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-28 03:37:06]

Failed to apply patch for
chrome/browser/sync_file_system/drive_backend/conflict_resolver_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
chrome/browser/sync_file_system/drive_backend/conflict_resolver_unittest.cc
  Hunk #1 succeeded at 44 (offset 1 line).
  Hunk #2 succeeded at 56 with fuzz 2 (offset 1 line).
  Hunk #3 succeeded at 88 (offset 8 lines).
  Hunk #4 succeeded at 153 (offset 8 lines).
  Hunk #5 succeeded at 176 (offset 8 lines).
  Hunk #6 succeeded at 191 (offset 8 lines).
  Hunk #7 FAILED at 201.
  Hunk #8 succeeded at 259 with fuzz 1 (offset 11 lines).
  1 out of 8 hunks FAILED -- saving rejects to file
chrome/browser/sync_file_system/drive_backend/conflict_resolver_unittest.cc.rej

Patch:      
chrome/browser/sync_file_system/drive_backend/conflict_resolver_unittest.cc
Index:
chrome/browser/sync_file_system/drive_backend/conflict_resolver_unittest.cc
diff --git
a/chrome/browser/sync_file_system/drive_backend/conflict_resolver_unittest.cc
b/chrome/browser/sync_file_system/drive_backend/conflict_resolver_unittest.cc
index
6f3349028c928195080d7ed3d8075c435f08391f..c7e4a5cca030081f40677e8a1f079e9806270269
100644
---
a/chrome/browser/sync_file_system/drive_backend/conflict_resolver_unittest.cc
+++
b/chrome/browser/sync_file_system/drive_backend/conflict_resolver_unittest.cc
@@ -43,8 +43,7 @@ fileapi::FileSystemURL URL(const GURL& origin,
 
 }  // namespace
 
-class ConflictResolverTest : public testing::Test,
-                             public SyncEngineContext {
+class ConflictResolverTest : public testing::Test {
  public:
   typedef FakeRemoteChangeProcessor::URLToFileChangesMap URLToFileChangesMap;
 
@@ -56,18 +55,24 @@ class ConflictResolverTest : public testing::Test,
     ASSERT_TRUE(database_dir_.CreateUniqueTempDir());
     in_memory_env_.reset(leveldb::NewMemEnv(leveldb::Env::Default()));
 
-    fake_drive_service_.reset(new FakeDriveServiceWrapper);
-    ASSERT_TRUE(fake_drive_service_->LoadAccountMetadataForWapi(
+    scoped_ptr<FakeDriveServiceWrapper>
+        fake_drive_service(new FakeDriveServiceWrapper);
+    ASSERT_TRUE(fake_drive_service->LoadAccountMetadataForWapi(
         "sync_file_system/account_metadata.json"));
-    ASSERT_TRUE(fake_drive_service_->LoadResourceListForWapi(
+    ASSERT_TRUE(fake_drive_service->LoadResourceListForWapi(
         "gdata/empty_feed.json"));
 
-    drive_uploader_.reset(new FakeDriveUploader(fake_drive_service_.get()));
+    scoped_ptr<drive::DriveUploaderInterface>
+        drive_uploader(new FakeDriveUploader(fake_drive_service.get()));
     fake_drive_helper_.reset(new FakeDriveServiceHelper(
-        fake_drive_service_.get(), drive_uploader_.get(),
-        kSyncRootFolderTitle));
+        fake_drive_service.get(), drive_uploader.get(), kSyncRootFolderTitle));
     fake_remote_change_processor_.reset(new FakeRemoteChangeProcessor);
 
+    context_.reset(new SyncEngineContext(
+        fake_drive_service.PassAs<drive::DriveServiceInterface>(),
+        drive_uploader.Pass(), base::MessageLoopProxy::current()));
+    context_->SetRemoteChangeProcessor(fake_remote_change_processor_.get());
+
     RegisterSyncableFileSystem();
   }
 
@@ -75,55 +80,33 @@ class ConflictResolverTest : public testing::Test,
     RevokeSyncableFileSystem();
 
     fake_remote_change_processor_.reset();
-    metadata_database_.reset();
     fake_drive_helper_.reset();
-    drive_uploader_.reset();
-    fake_drive_service_.reset();
+    context_.reset();
     base::RunLoop().RunUntilIdle();
   }
 
   void InitializeMetadataDatabase() {
-    SyncEngineInitializer initializer(this,
+    SyncEngineInitializer initializer(context_.get(),
                                       base::MessageLoopProxy::current(),
-                                      fake_drive_service_.get(),
+                                      context_->GetDriveService(),
                                       database_dir_.path(),
                                       in_memory_env_.get());
     SyncStatusCode status = SYNC_STATUS_UNKNOWN;
     initializer.RunSequential(CreateResultReceiver(&status));
     base::RunLoop().RunUntilIdle();
     EXPECT_EQ(SYNC_STATUS_OK, status);
-    metadata_database_ = initializer.PassMetadataDatabase();
+    context_->SetMetadataDatabase(initializer.PassMetadataDatabase());
   }
 
   void RegisterApp(const std::string& app_id,
                    const std::string& app_root_folder_id) {
     SyncStatusCode status = SYNC_STATUS_FAILED;
-    metadata_database_->RegisterApp(app_id, app_root_folder_id,
-                                    CreateResultReceiver(&status));
+    context_->GetMetadataDatabase()->RegisterApp(app_id, app_root_folder_id,
+                                                
CreateResultReceiver(&status));
     base::RunLoop().RunUntilIdle();
     EXPECT_EQ(SYNC_STATUS_OK, status);
   }
 
-  virtual drive::DriveServiceInterface* GetDriveService() OVERRIDE {
-    return fake_drive_service_.get();
-  }
-
-  virtual drive::DriveUploaderInterface* GetDriveUploader() OVERRIDE {
-    return drive_uploader_.get();
-  }
-
-  virtual MetadataDatabase* GetMetadataDatabase() OVERRIDE {
-    return metadata_database_.get();
-  }
-
-  virtual RemoteChangeProcessor* GetRemoteChangeProcessor() OVERRIDE {
-    return fake_remote_change_processor_.get();
-  }
-
-  virtual base::SequencedTaskRunner* GetBlockingTaskRunner() OVERRIDE {
-    return base::MessageLoopProxy::current().get();
-  }
-
  protected:
   std::string CreateSyncRoot() {
     std::string sync_root_folder_id;
@@ -162,7 +145,7 @@ class ConflictResolverTest : public testing::Test,
       const std::string& parent_folder_id,
       const std::string& file_id) {
     google_apis::GDataErrorCode error = google_apis::GDATA_OTHER_ERROR;
-    fake_drive_service_->AddResourceToDirectory(
+    context_->GetDriveService()->AddResourceToDirectory(
         parent_folder_id, file_id,
         CreateResultReceiver(&error));
     base::RunLoop().RunUntilIdle();
@@ -185,7 +168,8 @@ class ConflictResolverTest : public testing::Test,
 
   SyncStatusCode RunRemoteToLocalSyncer() {
     SyncStatusCode status = SYNC_STATUS_UNKNOWN;
-    scoped_ptr<RemoteToLocalSyncer> syncer(new RemoteToLocalSyncer(this));
+    scoped_ptr<RemoteToLocalSyncer> syncer(
+        new RemoteToLocalSyncer(context_.get()));
     syncer->RunSequential(CreateResultReceiver(&status));
     base::RunLoop().RunUntilIdle();
     return status;
@@ -199,7 +183,8 @@ class ConflictResolverTest : public testing::Test,
     if (file_change.IsAddOrUpdate())
       CreateTemporaryFileInDir(database_dir_.path(), &local_path);
     scoped_ptr<LocalToRemoteSyncer> syncer(new LocalToRemoteSyncer(
-        this, SyncFileMetadata(file_change.file_type(), 0, base::Time()),
+        context_.get(),
+        SyncFileMetadata(file_change.file_type(), 0, base::Time()),
         file_change, local_path, url));
     syncer->RunSequential(CreateResultReceiver(&status));
     base::RunLoop().RunUntilIdle();
@@ -216,14 +201,14 @@ class ConflictResolverTest : public testing::Test,
 
   SyncStatusCode RunConflictResolver() {
     SyncStatusCode status = SYNC_STATUS_UNKNOWN;
-    ConflictResolver resolver(this);
+    ConflictResolver resolver(context_.get());
     resolver.RunSequential(CreateResultReceiver(&status));
     base::RunLoop().RunUntilIdle();
     return status;
   }
 
   SyncStatusCode ListChanges() {
-    ListChangesTask list_changes(this);
+    ListChangesTask list_changes(context_.get());
     SyncStatusCode status = SYNC_STATUS_UNKNOWN;
     list_changes.RunSequential(CreateResultReceiver(&status));
     base::RunLoop().RunUntilIdle();
@@ -263,10 +248,8 @@ class ConflictResolverTest : public testing::Test,
   base::ScopedTempDir database_dir_;
   scoped_ptr<leveldb::Env> in_memory_env_;
 
-  scoped_ptr<FakeDriveServiceWrapper> fake_drive_service_;
-  scoped_ptr<FakeDriveUploader> drive_uploader_;
+  scoped_ptr<SyncEngineContext> context_;
   scoped_ptr<FakeDriveServiceHelper> fake_drive_helper_;
-  scoped_ptr<MetadataDatabase> metadata_database_;
   scoped_ptr<FakeRemoteChangeProcessor> fake_remote_change_processor_;
 
   DISALLOW_COPY_AND_ASSIGN(ConflictResolverTest);

[peria, 2014-03-28 04:12:33]

The CQ bit was checked by peria@chromium.org

[commit-bot: I haz the power, 2014-03-28 04:13:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/peria@chromium.org/213473008/280001

[commit-bot: I haz the power, 2014-03-28 07:24:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-28 07:24:10]

Retried try job too often on ios_dbg_simulator for step(s) base_unittests,
components_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[peria, 2014-03-28 07:26:58]

The CQ bit was checked by peria@chromium.org

[commit-bot: I haz the power, 2014-03-28 07:28:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/peria@chromium.org/213473008/280001

[commit-bot: I haz the power, 2014-03-28 12:19:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-28 12:19:15]

Retried try job too often on ios_dbg_simulator for step(s) base_unittests,
components_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[tzik, 2014-03-28 16:41:53]

The CQ bit was checked by tzik@chromium.org

[commit-bot: I haz the power, 2014-03-28 16:45:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/peria@chromium.org/213473008/280001

[commit-bot: I haz the power, 2014-03-28 16:49:45]

Change committed as 260166