Make AttachmentService accessible directly from

SyncableService without having to have SyncData.

Added a new method SetAttachmentService to SyncableService
interface. It should be implemented by a syncable service that
supports attachments. By default this method does nothing.

This method is called SharedChangeProcessor when it gets
connected to an attachment enabled syncable service (which
provides a non-default implementation GetAttachmentStore).

Added a new test case that tests attachment specific
integration code in GenericChangeProcessor and SyncableService.

BUG=439510

Review URL: https://codereview.chromium.org/769913003

Cr-Commit-Position: refs/heads/master@{#307520}

components/sync_driver/generic_change_processor.cc

@@ -114,14 +114,14 @@ GenericChangeProcessor::GenericChangeProcessor(
     attachment_service_weak_ptr_factory_.reset(
         new base::WeakPtrFactory<syncer::AttachmentService>(
             attachment_service_.get()));
-    attachment_service_proxy_.reset(new syncer::AttachmentServiceProxy(
+    attachment_service_proxy_ = syncer::AttachmentServiceProxy(
         base::MessageLoopProxy::current(),
-        attachment_service_weak_ptr_factory_->GetWeakPtr()));
+        attachment_service_weak_ptr_factory_->GetWeakPtr());
     UploadAllAttachmentsNotOnServer();
   } else {
-    attachment_service_proxy_.reset(new syncer::AttachmentServiceProxy(
+    attachment_service_proxy_ = syncer::AttachmentServiceProxy(
         base::MessageLoopProxy::current(),
-        base::WeakPtr<syncer::AttachmentService>()));
+        base::WeakPtr<syncer::AttachmentService>());
   }
 }
 
@@ -146,15 +146,11 @@ void GenericChangeProcessor::ApplyChangesFromSyncModel(
             CopyFrom(it->extra->unencrypted());
       }
       const syncer::AttachmentIdList empty_list_of_attachment_ids;
-      syncer_changes_.push_back(
-          syncer::SyncChange(FROM_HERE,
-                             syncer::SyncChange::ACTION_DELETE,
-                             syncer::SyncData::CreateRemoteData(
-                                 it->id,
-                                 specifics ? *specifics : it->specifics,
-                                 base::Time(),
-                                 empty_list_of_attachment_ids,
-                                 *attachment_service_proxy_)));
+      syncer_changes_.push_back(syncer::SyncChange(
+          FROM_HERE, syncer::SyncChange::ACTION_DELETE,
+          syncer::SyncData::CreateRemoteData(
+              it->id, specifics ? *specifics : it->specifics, base::Time(),
+              empty_list_of_attachment_ids, attachment_service_proxy_)));
     } else {
       syncer::SyncChange::SyncChangeType action =
           (it->action == syncer::ChangeRecord::ACTION_ADD) ?
@@ -172,9 +168,8 @@ void GenericChangeProcessor::ApplyChangesFromSyncModel(
         return;
       }
       syncer_changes_.push_back(syncer::SyncChange(
-          FROM_HERE,
-          action,
-          BuildRemoteSyncData(it->id, read_node, *attachment_service_proxy_)));
+          FROM_HERE, action,
+          BuildRemoteSyncData(it->id, read_node, attachment_service_proxy_)));
     }
   }
 }
@@ -272,7 +267,7 @@ syncer::SyncError GenericChangeProcessor::GetAllSyncDataReturnError(
       return error;
     }
     current_sync_data->push_back(BuildRemoteSyncData(
-        sync_child_node.GetId(), sync_child_node, *attachment_service_proxy_));
+        sync_child_node.GetId(), sync_child_node, attachment_service_proxy_));
   }
   return syncer::SyncError();
 }
@@ -720,4 +715,10 @@ void GenericChangeProcessor::UploadAllAttachmentsNotOnServer() {
   }
 }
 
+scoped_ptr<syncer::AttachmentService>
+GenericChangeProcessor::GetAttachmentService() const {
+  return scoped_ptr<syncer::AttachmentService>(
+      new syncer::AttachmentServiceProxy(attachment_service_proxy_));
+}
+
 }  // namespace sync_driver

components/sync_driver/generic_change_processor.h

@@ -98,6 +98,9 @@ class GenericChangeProcessor : public ChangeProcessor,
   virtual bool SyncModelHasUserCreatedNodes(bool* has_nodes);
   virtual bool CryptoReadyIfNecessary();
 
+  // Gets AttachmentService proxy for datatype.
+  scoped_ptr<syncer::AttachmentService> GetAttachmentService() const;
+
  protected:
   // ChangeProcessor interface.
   void StartImpl() override;  // Does nothing.
@@ -162,8 +165,7 @@ class GenericChangeProcessor : public ChangeProcessor,
   // Can be NULL if attachment_service_ is NULL;
   scoped_ptr<base::WeakPtrFactory<syncer::AttachmentService> >
       attachment_service_weak_ptr_factory_;
-  scoped_ptr<syncer::AttachmentServiceProxy> attachment_service_proxy_;
-
+  syncer::AttachmentServiceProxy attachment_service_proxy_;
   base::WeakPtrFactory<GenericChangeProcessor> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(GenericChangeProcessor);

components/sync_driver/shared_change_processor.cc

@@ -12,6 +12,10 @@
 
 using base::AutoLock;
 
+namespace syncer {
+class AttachmentService;
+}
+
 namespace sync_driver {
 
 SharedChangeProcessor::SharedChangeProcessor()
@@ -72,6 +76,12 @@ base::WeakPtr<syncer::SyncableService> SharedChangeProcessor::Connect(
                                                       local_service,
                                                       merge_result,
                                                       sync_factory).release();
+  // If available, propagate attachment service to the syncable service.
+  scoped_ptr<syncer::AttachmentService> attachment_service =
+      generic_change_processor_->GetAttachmentService();
+  if (attachment_service) {
+    local_service->SetAttachmentService(attachment_service.Pass());
+  }
   return local_service;
 }
 

components/sync_driver/shared_change_processor_unittest.cc

@@ -10,13 +10,17 @@
 #include "base/bind_helpers.h"
 #include "base/compiler_specific.h"
 #include "base/message_loop/message_loop.h"
+#include "base/synchronization/waitable_event.h"
 #include "base/threading/thread.h"
 #include "components/sync_driver/data_type_error_handler_mock.h"
 #include "components/sync_driver/generic_change_processor.h"
 #include "components/sync_driver/generic_change_processor_factory.h"
 #include "components/sync_driver/sync_api_component_factory.h"
+#include "sync/api/attachments/attachment_id.h"
+#include "sync/api/attachments/attachment_store.h"
 #include "sync/api/fake_syncable_service.h"
 #include "sync/internal_api/public/attachments/attachment_service_impl.h"
+#include "sync/internal_api/public/test/test_user_share.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -31,8 +35,10 @@ class SyncSharedChangeProcessorTest :
     public testing::Test,
     public SyncApiComponentFactory {
  public:
-  SyncSharedChangeProcessorTest() : backend_thread_("dbthread"),
-                                    did_connect_(false) {}
+  SyncSharedChangeProcessorTest()
+      : backend_thread_("dbthread"),
+        did_connect_(false),
+        has_attachment_service_(false) {}
 
   virtual ~SyncSharedChangeProcessorTest() {
     EXPECT_FALSE(db_syncable_service_.get());
@@ -53,6 +59,7 @@ class SyncSharedChangeProcessorTest :
 
  protected:
   virtual void SetUp() override {
+    test_user_share_.SetUp();
     shared_change_processor_ = new SharedChangeProcessor();
     ASSERT_TRUE(backend_thread_.Start());
     ASSERT_TRUE(backend_thread_.message_loop_proxy()->PostTask(
@@ -80,6 +87,7 @@ class SyncSharedChangeProcessorTest :
     // would race with the write in ConnectOnDBThread (because by this time,
     // everything that could have run on |backend_thread_| has done so).
     ASSERT_TRUE(did_connect_);
+    test_user_share_.TearDown();
   }
 
   // Connect |shared_change_processor_| on the DB thread.
@@ -91,6 +99,24 @@ class SyncSharedChangeProcessorTest :
                    shared_change_processor_)));
   }
 
+  void SetAttachmentStore() {
+    EXPECT_TRUE(backend_thread_.message_loop_proxy()->PostTask(
+        FROM_HERE,
+        base::Bind(&SyncSharedChangeProcessorTest::SetAttachmentStoreOnDBThread,
+                   base::Unretained(this))));
+  }
+
+  bool HasAttachmentService() {
+    base::WaitableEvent event(false, false);
+    EXPECT_TRUE(backend_thread_.message_loop_proxy()->PostTask(
+        FROM_HERE,
+        base::Bind(
+            &SyncSharedChangeProcessorTest::CheckAttachmentServiceOnDBThread,
+            base::Unretained(this), base::Unretained(&event))));
+    event.Wait();
+    return has_attachment_service_;
+  }
+
  private:
   // Used by SetUp().
   void SetUpDBSyncableService() {
@@ -106,31 +132,43 @@ class SyncSharedChangeProcessorTest :
     db_syncable_service_.reset();
   }
 
+  void SetAttachmentStoreOnDBThread() {
+    DCHECK(backend_thread_.message_loop_proxy()->BelongsToCurrentThread());
+    DCHECK(db_syncable_service_.get());
+    db_syncable_service_->set_attachment_store(
+        syncer::AttachmentStore::CreateInMemoryStore());
+  }
+
   // Used by Connect().  The SharedChangeProcessor is passed in
   // because we modify |shared_change_processor_| on the main thread
   // (in TearDown()).
   void ConnectOnDBThread(
       const scoped_refptr<SharedChangeProcessor>& shared_change_processor) {
     DCHECK(backend_thread_.message_loop_proxy()->BelongsToCurrentThread());
-    syncer::UserShare share;
     EXPECT_TRUE(shared_change_processor->Connect(
-        this,
-        &processor_factory_,
-        &share,
-        &error_handler_,
-        syncer::AUTOFILL,
+        this, &processor_factory_, test_user_share_.user_share(),
+        &error_handler_, syncer::AUTOFILL,
         base::WeakPtr<syncer::SyncMergeResult>()));
     did_connect_ = true;
   }
 
+  void CheckAttachmentServiceOnDBThread(base::WaitableEvent* event) {
+    DCHECK(backend_thread_.message_loop_proxy()->BelongsToCurrentThread());
+    DCHECK(db_syncable_service_.get());
+    has_attachment_service_ = !!db_syncable_service_->attachment_service();
+    event->Signal();
+  }
+
   base::MessageLoop frontend_loop_;
   base::Thread backend_thread_;
+  syncer::TestUserShare test_user_share_;
 
   scoped_refptr<SharedChangeProcessor> shared_change_processor_;
   StrictMock<DataTypeErrorHandlerMock> error_handler_;
 
   GenericChangeProcessorFactory processor_factory_;
   bool did_connect_;
+  bool has_attachment_service_;
 
   // Used only on DB thread.
   scoped_ptr<syncer::FakeSyncableService> db_syncable_service_;
@@ -142,6 +180,15 @@ TEST_F(SyncSharedChangeProcessorTest, Basic) {
   Connect();
 }
 
+// Connect the shared change processor to a syncable service with
+// AttachmentStore. Verify that shared change processor implementation
+// creates AttachmentService and passes it back to the syncable service.
+TEST_F(SyncSharedChangeProcessorTest, ConnectWithAttachmentStore) {
+  SetAttachmentStore();
+  Connect();
+  EXPECT_TRUE(HasAttachmentService());
+}
+
 }  // namespace
 
 }  // namespace sync_driver

sync/api/fake_syncable_service.cc

@@ -25,6 +25,15 @@ void FakeSyncableService::set_process_sync_changes_error(
   process_sync_changes_error_ = error;
 }
 
+void FakeSyncableService::set_attachment_store(
+    const scoped_refptr<AttachmentStore>& attachment_store) {
+  attachment_store_ = attachment_store;
+}
+
+const AttachmentService* FakeSyncableService::attachment_service() const {
+  return attachment_service_.get();
+}
+
 bool FakeSyncableService::syncing() const {
   return syncing_;
 }
@@ -61,4 +70,13 @@ SyncError FakeSyncableService::ProcessSyncChanges(
   return process_sync_changes_error_;
 }
 
+scoped_refptr<AttachmentStore> FakeSyncableService::GetAttachmentStore() {
+  return attachment_store_;
+}
+
+void FakeSyncableService::SetAttachmentService(
+    scoped_ptr<AttachmentService> attachment_service) {
+  attachment_service_ = attachment_service.Pass();
+}
+
 }  // namespace syncer

sync/api/fake_syncable_service.h

@@ -22,6 +22,14 @@ class FakeSyncableService : public SyncableService {
   void set_merge_data_and_start_syncing_error(const SyncError& error);
   void set_process_sync_changes_error(const SyncError& error);
 
+  // Setter for AttachmentStore.
+  void set_attachment_store(
+      const scoped_refptr<AttachmentStore>& attachment_store);
+
+  // AttachmentService should be set when this syncable service is connected,
+  // just before MergeDataAndStartSyncing. NULL is returned by default.
+  const AttachmentService* attachment_service() const;
+
   // Whether we're syncing or not. Set on a successful MergeDataAndStartSyncing,
   // unset on StopSyncing. False by default.
   bool syncing() const;
@@ -36,13 +44,18 @@ class FakeSyncableService : public SyncableService {
   SyncDataList GetAllSyncData(ModelType type) const override;
   SyncError ProcessSyncChanges(const tracked_objects::Location& from_here,
                                const SyncChangeList& change_list) override;
+  scoped_refptr<AttachmentStore> GetAttachmentStore() override;
+  void SetAttachmentService(
+      scoped_ptr<AttachmentService> attachment_service) override;
 
  private:
   scoped_ptr<SyncChangeProcessor> sync_processor_;
   SyncError merge_data_and_start_syncing_error_;
   SyncError process_sync_changes_error_;
   bool syncing_;
   ModelType type_;
+  scoped_refptr<AttachmentStore> attachment_store_;
+  scoped_ptr<AttachmentService> attachment_service_;
 };
 
 }  // namespace syncer

sync/api/syncable_service.cc

@@ -12,4 +12,8 @@ scoped_refptr<AttachmentStore> SyncableService::GetAttachmentStore() {
   return scoped_refptr<AttachmentStore>();
 }
 
+void SyncableService::SetAttachmentService(
+    scoped_ptr<AttachmentService> attachment_service) {
+}
+
 }  // namespace syncer

sync/api/syncable_service.h

@@ -21,6 +21,7 @@
 
 namespace syncer {
 
+class AttachmentService;
 class SyncErrorFactory;
 
 // TODO(zea): remove SupportsWeakPtr in favor of having all SyncableService
@@ -76,6 +77,16 @@ class SYNC_EXPORT SyncableService
   // to return pointer to it.
   virtual scoped_refptr<AttachmentStore> GetAttachmentStore();
 
+  // Called by sync to provide AttachmentService to be used to download
+  // attachments.
+  // SetAttachmentService is called after GetAttachmentStore and right before
+  // MergeDataAndStartSyncing and only if GetAttachmentStore has returned a
+  // non-NULL store instance. Default implementation does nothing.
+  // Datatype that uses attachments must take ownerhip of the provided
+  // AttachmentService instance.
+  virtual void SetAttachmentService(
+      scoped_ptr<AttachmentService> attachment_service);
+
  protected:
   ~SyncableService() override;
 };