diff options
| -rw-r--r-- | components/sync_driver/generic_change_processor.cc | 69 | ||||
| -rw-r--r-- | components/sync_driver/generic_change_processor.h | 31 | ||||
| -rw-r--r-- | components/sync_driver/generic_change_processor_unittest.cc | 43 | ||||
| -rw-r--r-- | sync/api/sync_data.cc | 54 | ||||
| -rw-r--r-- | sync/api/sync_data.h | 12 | ||||
| -rw-r--r-- | sync/api/sync_data_unittest.cc | 19 |
6 files changed, 71 insertions, 157 deletions
diff --git a/components/sync_driver/generic_change_processor.cc b/components/sync_driver/generic_change_processor.cc index 30b215e..9bfd0c2 100644 --- a/components/sync_driver/generic_change_processor.cc +++ b/components/sync_driver/generic_change_processor.cc @@ -85,11 +85,6 @@ syncer::SyncData BuildRemoteSyncData( attachment_service_proxy); } -const syncer::AttachmentId& AttachmentToAttachmentId( - const syncer::Attachment& attachment) { - return attachment.GetId(); -} - } // namespace GenericChangeProcessor::GenericChangeProcessor( @@ -410,7 +405,7 @@ syncer::SyncError GenericChangeProcessor::ProcessSyncChanges( // Keep track of brand new attachments so we can persist them on this device // and upload them to the server. - syncer::AttachmentList new_attachments; + syncer::AttachmentIdList new_attachments; syncer::WriteTransaction trans(from_here, share_handle()); @@ -476,7 +471,7 @@ syncer::SyncError GenericChangeProcessor::ProcessSyncChanges( NOTREACHED(); return error; } - StoreAndUploadAttachments(new_attachments); + UploadAttachments(new_attachments); } return syncer::SyncError(); @@ -492,7 +487,7 @@ syncer::SyncError GenericChangeProcessor::HandleActionAdd( const syncer::ModelType& type, const syncer::WriteTransaction& trans, syncer::WriteNode* sync_node, - syncer::AttachmentList* new_attachments) { + syncer::AttachmentIdList* new_attachments) { // TODO(sync): Handle other types of creation (custom parents, folders, // etc.). syncer::ReadNode root_node(&trans); @@ -560,12 +555,8 @@ syncer::SyncError GenericChangeProcessor::HandleActionAdd( SetAttachmentMetadata(attachment_ids, sync_node); // Return any newly added attachments. - const syncer::AttachmentList& local_attachments_for_upload = - sync_data_local.GetLocalAttachmentsForUpload(); - new_attachments->insert(new_attachments->end(), - local_attachments_for_upload.begin(), - local_attachments_for_upload.end()); - + new_attachments->insert( + new_attachments->end(), attachment_ids.begin(), attachment_ids.end()); if (merge_result_.get()) { merge_result_->set_num_items_added(merge_result_->num_items_added() + 1); } @@ -581,7 +572,7 @@ syncer::SyncError GenericChangeProcessor::HandleActionUpdate( const syncer::ModelType& type, const syncer::WriteTransaction& trans, syncer::WriteNode* sync_node, - syncer::AttachmentList* new_attachments) { + syncer::AttachmentIdList* new_attachments) { // TODO(zea): consider having this logic for all possible changes? const syncer::SyncDataLocal sync_data_local(change.sync_data()); @@ -663,14 +654,12 @@ syncer::SyncError GenericChangeProcessor::HandleActionUpdate( sync_node->SetTitle(change.sync_data().GetTitle()); SetNodeSpecifics(sync_data_local.GetSpecifics(), sync_node); - SetAttachmentMetadata(sync_data_local.GetAttachmentIds(), sync_node); + syncer::AttachmentIdList attachment_ids = sync_data_local.GetAttachmentIds(); + SetAttachmentMetadata(attachment_ids, sync_node); // Return any newly added attachments. - const syncer::AttachmentList& local_attachments_for_upload = - sync_data_local.GetLocalAttachmentsForUpload(); - new_attachments->insert(new_attachments->end(), - local_attachments_for_upload.begin(), - local_attachments_for_upload.end()); + new_attachments->insert( + new_attachments->end(), attachment_ids.begin(), attachment_ids.end()); if (merge_result_.get()) { merge_result_->set_num_items_modified(merge_result_->num_items_modified() + @@ -722,42 +711,14 @@ syncer::UserShare* GenericChangeProcessor::share_handle() const { return share_handle_; } -void GenericChangeProcessor::StoreAndUploadAttachments( - const syncer::AttachmentList& attachments) { +void GenericChangeProcessor::UploadAttachments( + const syncer::AttachmentIdList& attachment_ids) { DCHECK(CalledOnValidThread()); DCHECK(attachment_service_.get() != NULL); - attachment_service_->GetStore()->Write( - attachments, - base::Bind(&GenericChangeProcessor::WriteAttachmentsDone, - weak_ptr_factory_.GetWeakPtr(), - attachments)); -} - -void GenericChangeProcessor::WriteAttachmentsDone( - const syncer::AttachmentList& attachments, - const syncer::AttachmentStore::Result& result) { - DCHECK(CalledOnValidThread()); - DCHECK(attachment_service_.get() != NULL); - if (result != syncer::AttachmentStore::SUCCESS) { - // TODO(maniscalco): Deal with case where an error occurred (bug 361251). - return; - } - // TODO(maniscalco): Here is where we're going to update the in-directory - // entry to indicate that the attachments have been successfully stored on - // disk. Why do we care? Because we might crash after persisting the - // directory to disk, but before we have persisted its attachments, leaving us - // with danging attachment ids. Having a flag that indicates we've stored the - // entry will allow us to detect and filter entries with dangling attachment - // ids (bug 368353). - - // Begin uploading the attachments now that they are safe on disk. - syncer::AttachmentIdSet attachment_ids; - std::transform(attachments.begin(), - attachments.end(), - std::inserter(attachment_ids, attachment_ids.end()), - AttachmentToAttachmentId); - attachment_service_->UploadAttachments(attachment_ids); + syncer::AttachmentIdSet attachment_id_set; + attachment_id_set.insert(attachment_ids.begin(), attachment_ids.end()); + attachment_service_->UploadAttachments(attachment_id_set); } } // namespace sync_driver diff --git a/components/sync_driver/generic_change_processor.h b/components/sync_driver/generic_change_processor.h index afd1ba3..f76fba5e 100644 --- a/components/sync_driver/generic_change_processor.h +++ b/components/sync_driver/generic_change_processor.h @@ -117,30 +117,25 @@ class GenericChangeProcessor : public ChangeProcessor, const syncer::ModelType& type, const syncer::WriteTransaction& trans, syncer::WriteNode* sync_node, - syncer::AttachmentList* new_attachments); + syncer::AttachmentIdList* new_attachments); // Logically part of ProcessSyncChanges. // // |new_attachments| is an output parameter containing newly added attachments // that need to be stored. This method will append to it. - syncer::SyncError HandleActionUpdate(const syncer::SyncChange& change, - const std::string& type_str, - const syncer::ModelType& type, - const syncer::WriteTransaction& trans, - syncer::WriteNode* sync_node, - syncer::AttachmentList* new_attachments); - - // Store |attachments| locally then upload them to the sync server. + syncer::SyncError HandleActionUpdate( + const syncer::SyncChange& change, + const std::string& type_str, + const syncer::ModelType& type, + const syncer::WriteTransaction& trans, + syncer::WriteNode* sync_node, + syncer::AttachmentIdList* new_attachments); + + // Upload |attachments| to the sync server. // - // Store and uploading are asynchronous operations. |WriteAttachmentsDone| - // will be invoked once the attachments have been stored on the local device. - void StoreAndUploadAttachments(const syncer::AttachmentList& attachments); - - // Invoked once attachments have been stored locally. - // - // See also AttachmentStore::WriteCallback. - void WriteAttachmentsDone(const syncer::AttachmentList& attachments, - const syncer::AttachmentStore::Result& result); + // This function assumes that attachments were already stored in + // AttachmentStore. + void UploadAttachments(const syncer::AttachmentIdList& attachment_ids); // The SyncableService this change processor will forward changes on to. const base::WeakPtr<syncer::SyncableService> local_service_; diff --git a/components/sync_driver/generic_change_processor_unittest.cc b/components/sync_driver/generic_change_processor_unittest.cc index 9ea4e43..ce20575 100644 --- a/components/sync_driver/generic_change_processor_unittest.cc +++ b/components/sync_driver/generic_change_processor_unittest.cc @@ -33,8 +33,6 @@ namespace sync_driver { namespace { -const char kTestData[] = "some data"; - // A mock that keeps track of attachments passed to UploadAttachments. class MockAttachmentService : public syncer::AttachmentServiceImpl { public: @@ -347,12 +345,10 @@ TEST_F(SyncGenericChangeProcessorTest, sync_pb::EntitySpecifics specifics; sync_pb::PreferenceSpecifics* pref_specifics = specifics.mutable_preference(); pref_specifics->set_name("test"); - syncer::AttachmentList attachments; - scoped_refptr<base::RefCountedString> attachment_data = - new base::RefCountedString; - attachment_data->data() = kTestData; - attachments.push_back(syncer::Attachment::Create(attachment_data)); - attachments.push_back(syncer::Attachment::Create(attachment_data)); + + syncer::AttachmentIdList attachment_ids; + attachment_ids.push_back(syncer::AttachmentId::Create()); + attachment_ids.push_back(syncer::AttachmentId::Create()); // Add a SyncData with two attachments. syncer::SyncChangeList change_list; @@ -360,7 +356,7 @@ TEST_F(SyncGenericChangeProcessorTest, syncer::SyncChange(FROM_HERE, syncer::SyncChange::ACTION_ADD, syncer::SyncData::CreateLocalDataWithAttachments( - tag, title, specifics, attachments))); + tag, title, specifics, attachment_ids))); ASSERT_FALSE( change_processor()->ProcessSyncChanges(FROM_HERE, change_list).IsSet()); RunLoop(); @@ -369,20 +365,20 @@ TEST_F(SyncGenericChangeProcessorTest, ASSERT_EQ(mock_attachment_service()->attachment_id_sets()->size(), 1U); const syncer::AttachmentIdSet& attachments_added = mock_attachment_service()->attachment_id_sets()->front(); - ASSERT_THAT(attachments_added, - testing::UnorderedElementsAre(attachments[0].GetId(), - attachments[1].GetId())); + ASSERT_THAT( + attachments_added, + testing::UnorderedElementsAre(attachment_ids[0], attachment_ids[1])); // Update the SyncData, replacing its two attachments with one new attachment. - syncer::AttachmentList new_attachments; - new_attachments.push_back(syncer::Attachment::Create(attachment_data)); + syncer::AttachmentIdList new_attachment_ids; + new_attachment_ids.push_back(syncer::AttachmentId::Create()); mock_attachment_service()->attachment_id_sets()->clear(); change_list.clear(); change_list.push_back( syncer::SyncChange(FROM_HERE, syncer::SyncChange::ACTION_UPDATE, syncer::SyncData::CreateLocalDataWithAttachments( - tag, title, specifics, new_attachments))); + tag, title, specifics, new_attachment_ids))); ASSERT_FALSE( change_processor()->ProcessSyncChanges(FROM_HERE, change_list).IsSet()); RunLoop(); @@ -392,7 +388,7 @@ TEST_F(SyncGenericChangeProcessorTest, const syncer::AttachmentIdSet& new_attachments_added = mock_attachment_service()->attachment_id_sets()->front(); ASSERT_THAT(new_attachments_added, - testing::UnorderedElementsAre(new_attachments[0].GetId())); + testing::UnorderedElementsAre(new_attachment_ids[0])); } // Verify that after attachment is uploaded GenericChangeProcessor updates @@ -403,11 +399,9 @@ TEST_F(SyncGenericChangeProcessorTest, AttachmentUploaded) { sync_pb::EntitySpecifics specifics; sync_pb::PreferenceSpecifics* pref_specifics = specifics.mutable_preference(); pref_specifics->set_name("test"); - syncer::AttachmentList attachments; - scoped_refptr<base::RefCountedString> attachment_data = - new base::RefCountedString; - attachment_data->data() = kTestData; - attachments.push_back(syncer::Attachment::Create(attachment_data)); + + syncer::AttachmentIdList attachment_ids; + attachment_ids.push_back(syncer::AttachmentId::Create()); // Add a SyncData with two attachments. syncer::SyncChangeList change_list; @@ -415,12 +409,11 @@ TEST_F(SyncGenericChangeProcessorTest, AttachmentUploaded) { syncer::SyncChange(FROM_HERE, syncer::SyncChange::ACTION_ADD, syncer::SyncData::CreateLocalDataWithAttachments( - tag, title, specifics, attachments))); + tag, title, specifics, attachment_ids))); ASSERT_FALSE( change_processor()->ProcessSyncChanges(FROM_HERE, change_list).IsSet()); - sync_pb::AttachmentIdProto attachment_id_proto = - attachments[0].GetId().GetProto(); + sync_pb::AttachmentIdProto attachment_id_proto = attachment_ids[0].GetProto(); syncer::AttachmentId attachment_id = syncer::AttachmentId::CreateFromProto(attachment_id_proto); @@ -429,7 +422,7 @@ TEST_F(SyncGenericChangeProcessorTest, AttachmentUploaded) { syncer::ReadNode node(&read_transaction); ASSERT_EQ(node.InitByClientTagLookup(syncer::PREFERENCES, tag), syncer::BaseNode::INIT_OK); - syncer::AttachmentIdList attachment_ids = node.GetAttachmentIds(); + attachment_ids = node.GetAttachmentIds(); EXPECT_EQ(1U, attachment_ids.size()); } diff --git a/sync/api/sync_data.cc b/sync/api/sync_data.cc index 5ddbecb..7fbdbc2 100644 --- a/sync/api/sync_data.cc +++ b/sync/api/sync_data.cc @@ -16,17 +16,8 @@ #include "sync/protocol/proto_value_conversions.h" #include "sync/protocol/sync.pb.h" -using syncer::Attachment; -using syncer::AttachmentIdList; -using syncer::AttachmentList; - namespace { -sync_pb::AttachmentIdProto AttachmentToProto( - const syncer::Attachment& attachment) { - return attachment.GetId().GetProto(); -} - sync_pb::AttachmentIdProto IdToProto( const syncer::AttachmentId& attachment_id) { return attachment_id.GetProto(); @@ -36,19 +27,12 @@ syncer::AttachmentId ProtoToId(const sync_pb::AttachmentIdProto& proto) { return syncer::AttachmentId::CreateFromProto(proto); } -// Return true iff |attachments| contains one or more elements with the same -// AttachmentId. -bool ContainsDuplicateAttachments(const syncer::AttachmentList& attachments) { - std::set<syncer::AttachmentId> id_set; - AttachmentList::const_iterator iter = attachments.begin(); - AttachmentList::const_iterator end = attachments.end(); - for (; iter != end; ++iter) { - if (id_set.find(iter->GetId()) != id_set.end()) { - return true; - } - id_set.insert(iter->GetId()); - } - return false; +// Return true iff |attachment_ids| contains duplicates. +bool ContainsDuplicateAttachments( + const syncer::AttachmentIdList& attachment_ids) { + syncer::AttachmentIdSet id_set; + id_set.insert(attachment_ids.begin(), attachment_ids.end()); + return id_set.size() != attachment_ids.size(); } } // namespace @@ -82,13 +66,11 @@ SyncData::SyncData() : id_(kInvalidId), is_valid_(false) {} SyncData::SyncData(int64 id, sync_pb::SyncEntity* entity, - AttachmentList* attachments, const base::Time& remote_modification_time, const syncer::AttachmentServiceProxy& attachment_service) : id_(id), remote_modification_time_(remote_modification_time), immutable_entity_(entity), - attachments_(attachments), attachment_service_(attachment_service), is_valid_(true) {} @@ -106,9 +88,9 @@ SyncData SyncData::CreateLocalDelete(const std::string& sync_tag, SyncData SyncData::CreateLocalData(const std::string& sync_tag, const std::string& non_unique_title, const sync_pb::EntitySpecifics& specifics) { - syncer::AttachmentList attachments; + syncer::AttachmentIdList attachment_ids; return CreateLocalDataWithAttachments( - sync_tag, non_unique_title, specifics, attachments); + sync_tag, non_unique_title, specifics, attachment_ids); } // Static. @@ -116,20 +98,18 @@ SyncData SyncData::CreateLocalDataWithAttachments( const std::string& sync_tag, const std::string& non_unique_title, const sync_pb::EntitySpecifics& specifics, - const AttachmentList& attachments) { - DCHECK(!ContainsDuplicateAttachments(attachments)); + const AttachmentIdList& attachment_ids) { + DCHECK(!ContainsDuplicateAttachments(attachment_ids)); sync_pb::SyncEntity entity; entity.set_client_defined_unique_tag(sync_tag); entity.set_non_unique_name(non_unique_title); entity.mutable_specifics()->CopyFrom(specifics); - std::transform(attachments.begin(), - attachments.end(), + std::transform(attachment_ids.begin(), + attachment_ids.end(), RepeatedFieldBackInserter(entity.mutable_attachment_id()), - AttachmentToProto); - AttachmentList copy_of_attachments(attachments); + IdToProto); return SyncData(kInvalidId, &entity, - ©_of_attachments, base::Time(), AttachmentServiceProxy()); } @@ -148,9 +128,7 @@ SyncData SyncData::CreateRemoteData( attachment_ids.end(), RepeatedFieldBackInserter(entity.mutable_attachment_id()), IdToProto); - AttachmentList attachments; - return SyncData( - id, &entity, &attachments, modification_time, attachment_service); + return SyncData(id, &entity, modification_time, attachment_service); } bool SyncData::IsValid() const { return is_valid_; } @@ -215,10 +193,6 @@ SyncDataLocal::SyncDataLocal(const SyncData& sync_data) : SyncData(sync_data) { SyncDataLocal::~SyncDataLocal() {} -const AttachmentList& SyncDataLocal::GetLocalAttachmentsForUpload() const { - return attachments_.Get(); -} - const std::string& SyncDataLocal::GetTag() const { return immutable_entity_.Get().client_defined_unique_tag(); } diff --git a/sync/api/sync_data.h b/sync/api/sync_data.h index 7a0c210..4e5acd3 100644 --- a/sync/api/sync_data.h +++ b/sync/api/sync_data.h @@ -14,7 +14,7 @@ #include "base/memory/ref_counted.h" #include "base/stl_util.h" #include "base/time/time.h" -#include "sync/api/attachments/attachment.h" +#include "sync/api/attachments/attachment_id.h" #include "sync/base/sync_export.h" #include "sync/internal_api/public/attachments/attachment_service_proxy.h" #include "sync/internal_api/public/base/model_type.h" @@ -55,7 +55,7 @@ class SYNC_EXPORT SyncData { // primarily for debug purposes, and will be overwritten if the datatype is // encrypted. // - // For data with attachments: |attachments| must not contain duplicates. + // For data with attachments: |attachment_ids| must not contain duplicates. static SyncData CreateLocalDelete( const std::string& sync_tag, ModelType datatype); @@ -67,7 +67,7 @@ class SYNC_EXPORT SyncData { const std::string& sync_tag, const std::string& non_unique_title, const sync_pb::EntitySpecifics& specifics, - const AttachmentList& attachments); + const AttachmentIdList& attachment_ids); // Helper method for creating SyncData objects originating from the syncer. static SyncData CreateRemoteData( @@ -136,8 +136,6 @@ class SYNC_EXPORT SyncData { // The actual shared sync entity being held. ImmutableSyncEntity immutable_entity_; - Immutable<AttachmentList> attachments_; - AttachmentServiceProxy attachment_service_; private: @@ -147,7 +145,6 @@ class SYNC_EXPORT SyncData { // Clears |entity| and |attachments|. SyncData(int64 id, sync_pb::SyncEntity* entity, - AttachmentList* attachments, const base::Time& remote_modification_time, const syncer::AttachmentServiceProxy& attachment_service); }; @@ -161,9 +158,6 @@ class SYNC_EXPORT SyncDataLocal : public SyncData { explicit SyncDataLocal(const SyncData& sync_data); ~SyncDataLocal(); - // Return a list of this SyncData's attachments. - const AttachmentList& GetLocalAttachmentsForUpload() const; - // Return the value of the unique client tag. This is only set for data going // TO the syncer, not coming from. const std::string& GetTag() const; diff --git a/sync/api/sync_data_unittest.cc b/sync/api/sync_data_unittest.cc index 92aa687..a0a83cd 100644 --- a/sync/api/sync_data_unittest.cc +++ b/sync/api/sync_data_unittest.cc @@ -71,30 +71,28 @@ TEST_F(SyncDataTest, CreateLocalData) { TEST_F(SyncDataTest, CreateLocalDataWithAttachments) { specifics.mutable_preference(); - scoped_refptr<base::RefCountedMemory> bytes(new base::RefCountedString); - AttachmentList attachments; - attachments.push_back(Attachment::Create(bytes)); - attachments.push_back(Attachment::Create(bytes)); - attachments.push_back(Attachment::Create(bytes)); + AttachmentIdList attachment_ids; + attachment_ids.push_back(AttachmentId::Create()); + attachment_ids.push_back(AttachmentId::Create()); + attachment_ids.push_back(AttachmentId::Create()); SyncData data = SyncData::CreateLocalDataWithAttachments( - kSyncTag, kNonUniqueTitle, specifics, attachments); + kSyncTag, kNonUniqueTitle, specifics, attachment_ids); EXPECT_TRUE(data.IsValid()); EXPECT_TRUE(data.IsLocal()); EXPECT_EQ(kSyncTag, SyncDataLocal(data).GetTag()); EXPECT_EQ(kDatatype, data.GetDataType()); EXPECT_EQ(kNonUniqueTitle, data.GetTitle()); EXPECT_TRUE(data.GetSpecifics().has_preference()); - AttachmentIdList attachment_ids = data.GetAttachmentIds(); + attachment_ids = data.GetAttachmentIds(); EXPECT_EQ(3U, attachment_ids.size()); - EXPECT_EQ(3U, SyncDataLocal(data).GetLocalAttachmentsForUpload().size()); } TEST_F(SyncDataTest, CreateLocalDataWithAttachments_EmptyListOfAttachments) { specifics.mutable_preference(); - AttachmentList attachments; + AttachmentIdList attachment_ids; SyncData data = SyncData::CreateLocalDataWithAttachments( - kSyncTag, kNonUniqueTitle, specifics, attachments); + kSyncTag, kNonUniqueTitle, specifics, attachment_ids); EXPECT_TRUE(data.IsValid()); EXPECT_TRUE(data.IsLocal()); EXPECT_EQ(kSyncTag, SyncDataLocal(data).GetTag()); @@ -102,7 +100,6 @@ TEST_F(SyncDataTest, CreateLocalDataWithAttachments_EmptyListOfAttachments) { EXPECT_EQ(kNonUniqueTitle, data.GetTitle()); EXPECT_TRUE(data.GetSpecifics().has_preference()); EXPECT_TRUE(data.GetAttachmentIds().empty()); - EXPECT_TRUE(SyncDataLocal(data).GetLocalAttachmentsForUpload().empty()); } TEST_F(SyncDataTest, CreateRemoteData) { |