diff --git a/components/sync_driver/generic_change_processor.cc b/components/sync_driver/generic_change_processor.cc index 2c97b3e44ce2f7..56f7fcc17839f1 100644 --- a/components/sync_driver/generic_change_processor.cc +++ b/components/sync_driver/generic_change_processor.cc @@ -114,14 +114,14 @@ GenericChangeProcessor::GenericChangeProcessor( attachment_service_weak_ptr_factory_.reset( new base::WeakPtrFactory( 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())); + base::WeakPtr()); } } @@ -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 +GenericChangeProcessor::GetAttachmentService() const { + return scoped_ptr( + new syncer::AttachmentServiceProxy(attachment_service_proxy_)); +} + } // namespace sync_driver diff --git a/components/sync_driver/generic_change_processor.h b/components/sync_driver/generic_change_processor.h index d72947472e9fbd..8a94936472ad66 100644 --- a/components/sync_driver/generic_change_processor.h +++ b/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 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 > attachment_service_weak_ptr_factory_; - scoped_ptr attachment_service_proxy_; - + syncer::AttachmentServiceProxy attachment_service_proxy_; base::WeakPtrFactory weak_ptr_factory_; DISALLOW_COPY_AND_ASSIGN(GenericChangeProcessor); diff --git a/components/sync_driver/shared_change_processor.cc b/components/sync_driver/shared_change_processor.cc index ce494303414c8c..24e7136e43a8e3 100644 --- a/components/sync_driver/shared_change_processor.cc +++ b/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 SharedChangeProcessor::Connect( local_service, merge_result, sync_factory).release(); + // If available, propagate attachment service to the syncable service. + scoped_ptr attachment_service = + generic_change_processor_->GetAttachmentService(); + if (attachment_service) { + local_service->SetAttachmentService(attachment_service.Pass()); + } return local_service; } diff --git a/components/sync_driver/shared_change_processor_unittest.cc b/components/sync_driver/shared_change_processor_unittest.cc index 1b50458dd9960f..06d797b42e6855 100644 --- a/components/sync_driver/shared_change_processor_unittest.cc +++ b/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& 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())); 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 shared_change_processor_; StrictMock error_handler_; GenericChangeProcessorFactory processor_factory_; bool did_connect_; + bool has_attachment_service_; // Used only on DB thread. scoped_ptr 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 diff --git a/sync/api/fake_syncable_service.cc b/sync/api/fake_syncable_service.cc index 0d76f2ef07713e..ec7b90cf53be24 100644 --- a/sync/api/fake_syncable_service.cc +++ b/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& 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 FakeSyncableService::GetAttachmentStore() { + return attachment_store_; +} + +void FakeSyncableService::SetAttachmentService( + scoped_ptr attachment_service) { + attachment_service_ = attachment_service.Pass(); +} + } // namespace syncer diff --git a/sync/api/fake_syncable_service.h b/sync/api/fake_syncable_service.h index 8ecf05e2741b2c..7403ef3b811864 100644 --- a/sync/api/fake_syncable_service.h +++ b/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& 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,6 +44,9 @@ 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 GetAttachmentStore() override; + void SetAttachmentService( + scoped_ptr attachment_service) override; private: scoped_ptr sync_processor_; @@ -43,6 +54,8 @@ class FakeSyncableService : public SyncableService { SyncError process_sync_changes_error_; bool syncing_; ModelType type_; + scoped_refptr attachment_store_; + scoped_ptr attachment_service_; }; } // namespace syncer diff --git a/sync/api/syncable_service.cc b/sync/api/syncable_service.cc index 9d4773c7cb8bc2..7b0a9730a07708 100644 --- a/sync/api/syncable_service.cc +++ b/sync/api/syncable_service.cc @@ -12,4 +12,8 @@ scoped_refptr SyncableService::GetAttachmentStore() { return scoped_refptr(); } +void SyncableService::SetAttachmentService( + scoped_ptr attachment_service) { +} + } // namespace syncer diff --git a/sync/api/syncable_service.h b/sync/api/syncable_service.h index 17feb63bce44c5..960abc5781fa77 100644 --- a/sync/api/syncable_service.h +++ b/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 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 attachment_service); + protected: ~SyncableService() override; };