diff options
author | maniscalco@chromium.org <maniscalco@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-05-01 22:10:39 +0000 |
---|---|---|
committer | maniscalco@chromium.org <maniscalco@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-05-01 22:10:39 +0000 |
commit | d9ff32719de688849812cb10cf232885d85ab298 (patch) | |
tree | 43757821f6a3d96d25983fb4d2708a2184f9e5db /sync | |
parent | b6cd47221942b3b7352a87d03dcce2855bed0e79 (diff) | |
download | chromium_src-d9ff32719de688849812cb10cf232885d85ab298.zip chromium_src-d9ff32719de688849812cb10cf232885d85ab298.tar.gz chromium_src-d9ff32719de688849812cb10cf232885d85ab298.tar.bz2 |
Revert of Keep track of which attachments are referenced by which sync entries. (https://codereview.chromium.org/247983002/)
Reason for revert:
Looks like this may have introduced a memory leak:
http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28valgrind%29%282%29/builds/37232
Reverting. Will investigate and reland when appropriate.
Original issue's description:
> Keep track of which attachments are referenced by which sync entries.
>
> PutAttachmentMetadata on MutableEntry now notifies the Directory when
> the attachments associated with an entry change.
>
> Give Directory::InitializeIndices a ScopedKernelLock to be consistent
> and make it easier to ensure we are locking when we need to.
>
> GenericChangeProcess passes new attachments to AttachmentService for
> storage and upload.
>
> Add an output parameter to GenericChangeProcessor's HandleActionAdd and
> HandleActionUpdate methods so they can keep track of potentially new
> attachments and pass them to AttachmentService for storage/upload.
>
> Convert AttachmentService's OnSyncDataAdd to StoreAttachments.
>
> Implement HasAttachmentNotOnServer.
>
> BUG=348625,353303,354530,356266
>
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267617
TBR=pavely@chromium.org,tim@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=348625,353303,354530,356266
Review URL: https://codereview.chromium.org/265853004
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@267649 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'sync')
27 files changed, 130 insertions, 640 deletions
diff --git a/sync/api/attachments/attachment_id.cc b/sync/api/attachments/attachment_id.cc index e61705e..6f3e0a2 100644 --- a/sync/api/attachments/attachment_id.cc +++ b/sync/api/attachments/attachment_id.cc @@ -5,7 +5,7 @@ #include "sync/api/attachments/attachment_id.h" #include "base/logging.h" -#include "sync/internal_api/public/base/attachment_id_proto.h" +#include "base/rand_util.h" #include "sync/protocol/sync.pb.h" namespace syncer { @@ -53,7 +53,10 @@ bool AttachmentId::operator<(const AttachmentId& other) const { // Static. AttachmentId AttachmentId::Create() { - sync_pb::AttachmentIdProto proto = CreateAttachmentIdProto(); + // Only requirement here is that this id must be globally unique. + // TODO(maniscalco): Consider making this base64 encoded. + sync_pb::AttachmentIdProto proto; + proto.set_unique_id(base::RandBytesAsString(16)); return AttachmentId(&proto); } diff --git a/sync/api/attachments/attachment_service.h b/sync/api/attachments/attachment_service.h index 9e5e25d..81a1a63 100644 --- a/sync/api/attachments/attachment_service.h +++ b/sync/api/attachments/attachment_service.h @@ -38,22 +38,11 @@ class SYNC_EXPORT AttachmentService { // The result of a DropAttachments operation. enum DropResult { DROP_SUCCESS, // No error, all attachments dropped. - DROP_UNSPECIFIED_ERROR, // An unspecified error occurred. Some or all - // attachments may not have been dropped. + DROP_UNSPECIFIED_ERROR, // An unspecified error occurred. }; typedef base::Callback<void(const DropResult&)> DropCallback; - // The result of a StoreAttachments operation. - enum StoreResult { - STORE_SUCCESS, // No error, all attachments stored (at least - // locally). - STORE_UNSPECIFIED_ERROR, // An unspecified error occurred. Some or all - // attachments may not have been stored. - }; - - typedef base::Callback<void(const StoreResult&)> StoreCallback; - AttachmentService(); virtual ~AttachmentService(); @@ -66,12 +55,10 @@ class SYNC_EXPORT AttachmentService { virtual void DropAttachments(const AttachmentIdList& attachment_ids, const DropCallback& callback) = 0; - // Store |attachments| on device and (eventually) upload them to the server. - // - // Invokes |callback| once the attachments have been written to device - // storage. - virtual void StoreAttachments(const AttachmentList& attachments, - const StoreCallback& callback) = 0; + // This method should be called when a SyncData is about to be added to the + // sync database so we have a chance to persist the Attachment locally and + // schedule it for upload to the sync server. + virtual void OnSyncDataAdd(const SyncData& sync_data) = 0; // This method should be called when a SyncData is about to be deleted from // the sync database so we can remove any unreferenced attachments from local diff --git a/sync/api/attachments/attachment_service_proxy.cc b/sync/api/attachments/attachment_service_proxy.cc index 6590033..9bc40e5 100644 --- a/sync/api/attachments/attachment_service_proxy.cc +++ b/sync/api/attachments/attachment_service_proxy.cc @@ -35,15 +35,6 @@ void ProxyDropCallback( task_runner->PostTask(FROM_HERE, base::Bind(callback, result)); } -// Invokes |callback| with |result| and |attachments| in the |task_runner| -// thread. -void ProxyStoreCallback( - const scoped_refptr<base::SequencedTaskRunner>& task_runner, - const AttachmentService::StoreCallback& callback, - const AttachmentService::StoreResult& result) { - task_runner->PostTask(FROM_HERE, base::Bind(callback, result)); -} - } // namespace AttachmentServiceProxy::AttachmentServiceProxy() { @@ -94,17 +85,11 @@ void AttachmentServiceProxy::DropAttachments( proxy_callback)); } -void AttachmentServiceProxy::StoreAttachments(const AttachmentList& attachments, - const StoreCallback& callback) { +void AttachmentServiceProxy::OnSyncDataAdd(const SyncData& sync_data) { DCHECK(wrapped_task_runner_); - StoreCallback proxy_callback = base::Bind( - &ProxyStoreCallback, base::MessageLoopProxy::current(), callback); wrapped_task_runner_->PostTask( FROM_HERE, - base::Bind(&AttachmentService::StoreAttachments, - core_, - attachments, - proxy_callback)); + base::Bind(&AttachmentService::OnSyncDataAdd, core_, sync_data)); } void AttachmentServiceProxy::OnSyncDataDelete(const SyncData& sync_data) { @@ -152,13 +137,11 @@ void AttachmentServiceProxy::Core::DropAttachments( wrapped_->DropAttachments(attachment_ids, callback); } -void AttachmentServiceProxy::Core::StoreAttachments( - const AttachmentList& attachments, - const StoreCallback& callback) { +void AttachmentServiceProxy::Core::OnSyncDataAdd(const SyncData& sync_data) { if (!wrapped_) { return; } - wrapped_->StoreAttachments(attachments, callback); + wrapped_->OnSyncDataAdd(sync_data); } void AttachmentServiceProxy::Core::OnSyncDataDelete(const SyncData& sync_data) { diff --git a/sync/api/attachments/attachment_service_proxy.h b/sync/api/attachments/attachment_service_proxy.h index 462cdda..7085625 100644 --- a/sync/api/attachments/attachment_service_proxy.h +++ b/sync/api/attachments/attachment_service_proxy.h @@ -58,8 +58,7 @@ class SYNC_EXPORT AttachmentServiceProxy : public AttachmentService { const GetOrDownloadCallback& callback) OVERRIDE; virtual void DropAttachments(const AttachmentIdList& attachment_ids, const DropCallback& callback) OVERRIDE; - virtual void StoreAttachments(const AttachmentList& attachment, - const StoreCallback& callback) OVERRIDE; + virtual void OnSyncDataAdd(const SyncData& sync_data) OVERRIDE; virtual void OnSyncDataDelete(const SyncData& sync_data) OVERRIDE; virtual void OnSyncDataUpdate(const AttachmentIdList& old_attachment_ids, const SyncData& updated_sync_data) OVERRIDE; @@ -90,8 +89,7 @@ class SYNC_EXPORT AttachmentServiceProxy : public AttachmentService { const GetOrDownloadCallback& callback) OVERRIDE; virtual void DropAttachments(const AttachmentIdList& attachment_ids, const DropCallback& callback) OVERRIDE; - virtual void StoreAttachments(const AttachmentList& attachment, - const StoreCallback& callback) OVERRIDE; + virtual void OnSyncDataAdd(const SyncData& sync_data) OVERRIDE; virtual void OnSyncDataDelete(const SyncData& sync_data) OVERRIDE; virtual void OnSyncDataUpdate(const AttachmentIdList& old_attachment_ids, const SyncData& updated_sync_data) OVERRIDE; diff --git a/sync/api/attachments/attachment_service_proxy_unittest.cc b/sync/api/attachments/attachment_service_proxy_unittest.cc index 08c499e..f6e3502 100644 --- a/sync/api/attachments/attachment_service_proxy_unittest.cc +++ b/sync/api/attachments/attachment_service_proxy_unittest.cc @@ -56,12 +56,9 @@ class StubAttachmentService : public AttachmentService, FROM_HERE, base::Bind(callback, AttachmentService::DROP_SUCCESS)); } - virtual void StoreAttachments(const AttachmentList& attachments, - const StoreCallback& callback) OVERRIDE { + virtual void OnSyncDataAdd(const SyncData& sync_data) OVERRIDE { CalledOnValidThread(); Increment(); - base::MessageLoop::current()->PostTask( - FROM_HERE, base::Bind(callback, AttachmentService::STORE_SUCCESS)); } virtual void OnSyncDataDelete(const SyncData& sync_data) OVERRIDE { @@ -122,11 +119,8 @@ class AttachmentServiceProxyTest : public testing::Test, base::Unretained(this)); callback_drop = base::Bind(&AttachmentServiceProxyTest::IncrementDrop, base::Unretained(this)); - callback_store = base::Bind(&AttachmentServiceProxyTest::IncrementStore, - base::Unretained(this)); count_callback_get_or_download = 0; count_callback_drop = 0; - count_callback_store = 0; } virtual void TearDown() @@ -153,12 +147,6 @@ class AttachmentServiceProxyTest : public testing::Test, ++count_callback_drop; } - // a StoreCallback - void IncrementStore(const AttachmentService::StoreResult&) { - CalledOnValidThread(); - ++count_callback_store; - } - void WaitForStubThread() { base::WaitableEvent done(false, false); stub_thread->message_loop()->PostTask( @@ -177,23 +165,21 @@ class AttachmentServiceProxyTest : public testing::Test, AttachmentService::GetOrDownloadCallback callback_get_or_download; AttachmentService::DropCallback callback_drop; - AttachmentService::StoreCallback callback_store; // number of times callback_get_or_download was invoked int count_callback_get_or_download; // number of times callback_drop was invoked int count_callback_drop; - // number of times callback_store was invoked - int count_callback_store; }; // Verify that each of AttachmentServiceProxy's regular methods (those that // don't take callbacks) are invoked on the stub. TEST_F(AttachmentServiceProxyTest, MethodsAreProxied) { + proxy->OnSyncDataAdd(sync_data); proxy->OnSyncDataDelete(sync_data_delete); proxy->OnSyncDataUpdate(AttachmentIdList(), sync_data); WaitForStubThread(); - EXPECT_EQ(2, stub->GetCallCount()); + EXPECT_EQ(3, stub->GetCallCount()); } // Verify that each of AttachmentServiceProxy's callback methods (those that @@ -202,10 +188,9 @@ TEST_F(AttachmentServiceProxyTest, MethodsAreProxied) { TEST_F(AttachmentServiceProxyTest, MethodsWithCallbacksAreProxied) { proxy->GetOrDownloadAttachments(AttachmentIdList(), callback_get_or_download); proxy->DropAttachments(AttachmentIdList(), callback_drop); - proxy->StoreAttachments(AttachmentList(), callback_store); // Wait for the posted calls to execute in the stub thread. WaitForStubThread(); - EXPECT_EQ(3, stub->GetCallCount()); + EXPECT_EQ(2, stub->GetCallCount()); // At this point the stub thread has finished executed the calls. However, the // result callbacks it has posted may not have executed yet. Wait a second // time to ensure the stub thread has executed the posted result callbacks. @@ -214,7 +199,6 @@ TEST_F(AttachmentServiceProxyTest, MethodsWithCallbacksAreProxied) { loop.RunUntilIdle(); EXPECT_EQ(1, count_callback_get_or_download); EXPECT_EQ(1, count_callback_drop); - EXPECT_EQ(1, count_callback_store); } // Verify that it's safe to use an AttachmentServiceProxy even after its wrapped diff --git a/sync/api/attachments/fake_attachment_service.cc b/sync/api/attachments/fake_attachment_service.cc index 5cb01e5..b9eb88e 100644 --- a/sync/api/attachments/fake_attachment_service.cc +++ b/sync/api/attachments/fake_attachment_service.cc @@ -51,15 +51,10 @@ void FakeAttachmentService::DropAttachments( callback)); } -void FakeAttachmentService::StoreAttachments(const AttachmentList& attachments, - const StoreCallback& callback) { +void FakeAttachmentService::OnSyncDataAdd(const SyncData& sync_data) { DCHECK(CalledOnValidThread()); - attachment_store_->Write(attachments, - base::Bind(&FakeAttachmentService::WriteDone, - weak_ptr_factory_.GetWeakPtr(), - callback)); - // TODO(maniscalco): Ensure the linked attachments are schedule for upload to - // the server (bug 356351). + // TODO(maniscalco): Ensure the linked attachments get persisted in local + // storage and schedule them for upload to the server (bug 356351). } void FakeAttachmentService::OnSyncDataDelete(const SyncData& sync_data) { @@ -104,16 +99,4 @@ void FakeAttachmentService::DropDone(const DropCallback& callback, base::Bind(callback, drop_result)); } -void FakeAttachmentService::WriteDone(const StoreCallback& callback, - const AttachmentStore::Result& result) { - AttachmentService::StoreResult store_result = - AttachmentService::STORE_UNSPECIFIED_ERROR; - if (result == AttachmentStore::SUCCESS) { - store_result = AttachmentService::STORE_SUCCESS; - } - // TODO(maniscalco): Deal with case where an error occurred (bug 361251). - base::MessageLoop::current()->PostTask(FROM_HERE, - base::Bind(callback, store_result)); -} - } // namespace syncer diff --git a/sync/api/attachments/fake_attachment_service.h b/sync/api/attachments/fake_attachment_service.h index 5bc443a..eb97c52 100644 --- a/sync/api/attachments/fake_attachment_service.h +++ b/sync/api/attachments/fake_attachment_service.h @@ -29,8 +29,7 @@ class SYNC_EXPORT FakeAttachmentService : public AttachmentService, OVERRIDE; virtual void DropAttachments(const AttachmentIdList& attachment_ids, const DropCallback& callback) OVERRIDE; - virtual void StoreAttachments(const AttachmentList& attachments, - const StoreCallback& callback) OVERRIDE; + virtual void OnSyncDataAdd(const SyncData& sync_data) OVERRIDE; virtual void OnSyncDataDelete(const SyncData& sync_data) OVERRIDE; virtual void OnSyncDataUpdate(const AttachmentIdList& old_attachment_ids, const SyncData& updated_sync_data) OVERRIDE; @@ -41,8 +40,6 @@ class SYNC_EXPORT FakeAttachmentService : public AttachmentService, scoped_ptr<AttachmentMap> attachments); void DropDone(const DropCallback& callback, const AttachmentStore::Result& result); - void WriteDone(const StoreCallback& callback, - const AttachmentStore::Result& result); const scoped_ptr<AttachmentStore> attachment_store_; // Must be last data member. diff --git a/sync/api/sync_data.cc b/sync/api/sync_data.cc index 35a21f9..fa48ea5 100644 --- a/sync/api/sync_data.cc +++ b/sync/api/sync_data.cc @@ -36,21 +36,6 @@ 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; -} - } // namespace namespace syncer { @@ -111,13 +96,15 @@ SyncData SyncData::CreateLocalData(const std::string& sync_tag, sync_tag, non_unique_title, specifics, attachments); } +// TODO(maniscalco): What should happen if the same Attachment appears in the +// list twice? Document the behavior and add a test case. +// // Static. 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)); sync_pb::SyncEntity entity; entity.set_client_defined_unique_tag(sync_tag); entity.set_non_unique_name(non_unique_title); @@ -126,6 +113,9 @@ SyncData SyncData::CreateLocalDataWithAttachments( attachments.end(), RepeatedFieldBackInserter(entity.mutable_attachment_id()), AttachmentToProto); + // TODO(maniscalco): Actually pass the attachments to the ctor and make them + // available to the AttachmentService once this SyncData gets passed into + // GenericChangeProcesso::ProcessSyncChanges (bug 354530). AttachmentList copy_of_attachments(attachments); return SyncData(kInvalidId, &entity, diff --git a/sync/api/sync_data.h b/sync/api/sync_data.h index 8795141..bb99b74 100644 --- a/sync/api/sync_data.h +++ b/sync/api/sync_data.h @@ -43,19 +43,14 @@ class SYNC_EXPORT SyncData { // Default copy and assign welcome. // Helper methods for creating SyncData objects for local data. - // - // |sync_tag| Must be a string unique to this datatype and is used as a node + // The sync tag must be a string unique to this datatype and is used as a node // identifier server-side. - // // For deletes: |datatype| must specify the datatype who node is being // deleted. - // - // For adds/updates: |specifics| must be valid and |non_unique_title| (can be - // the same as |sync_tag|) must be specfied. Note: |non_unique_title| is - // primarily for debug purposes, and will be overwritten if the datatype is - // encrypted. - // - // For data with attachments: |attachments| must not contain duplicates. + // For adds/updates: the specifics must be valid and the non-unique title (can + // be the same as sync tag) must be specfied. + // Note: the non_unique_title is primarily for debug purposes, and will be + // overwritten if the datatype is encrypted. static SyncData CreateLocalDelete( const std::string& sync_tag, ModelType datatype); diff --git a/sync/engine/get_commit_ids.cc b/sync/engine/get_commit_ids.cc index 5f91915..80222ee 100644 --- a/sync/engine/get_commit_ids.cc +++ b/sync/engine/get_commit_ids.cc @@ -108,13 +108,9 @@ bool IsEntryInConflict(const syncable::Entry& entry) { // Return true if this entry has any attachments that haven't yet been uploaded // to the server. bool HasAttachmentNotOnServer(const syncable::Entry& entry) { - // TODO(maniscalco): Add test case (bug 356266). - const sync_pb::AttachmentMetadata& metadata = entry.GetAttachmentMetadata(); - for (int i = 0; i < metadata.record_size(); ++i) { - if (!metadata.record(i).is_on_server()) { - return true; - } - } + // TODO(maniscalco): Once AttachmentMetadata is fleshed out, implement this + // function to return true if any of the attachments haven't been uploaded to + // the server. Add test case (bug 356266). return false; } diff --git a/sync/internal_api/base_node.cc b/sync/internal_api/base_node.cc index 57dbb22..0788395 100644 --- a/sync/internal_api/base_node.cc +++ b/sync/internal_api/base_node.cc @@ -291,13 +291,10 @@ ModelType BaseNode::GetModelType() const { } const syncer::AttachmentIdList BaseNode::GetAttachmentIds() const { - AttachmentIdList result; - const sync_pb::AttachmentMetadata& metadata = - GetEntry()->GetAttachmentMetadata(); - for (int i = 0; i < metadata.record_size(); ++i) { - result.push_back(AttachmentId::CreateFromProto(metadata.record(i).id())); - } - return result; + // TODO(maniscalco): Once EntryKernel is capable of storing AttachmentIds, + // update this method to retrieve the list of AttachmentIds from read_node and + // pass it to CreateRemoteData (bug 348625). + return AttachmentIdList(); } void BaseNode::SetUnencryptedSpecifics( diff --git a/sync/internal_api/public/base/attachment_id_proto.cc b/sync/internal_api/public/base/attachment_id_proto.cc deleted file mode 100644 index d09c83e..0000000 --- a/sync/internal_api/public/base/attachment_id_proto.cc +++ /dev/null @@ -1,19 +0,0 @@ -// Copyright 2014 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#include "sync/internal_api/public/base/attachment_id_proto.h" - -#include "base/rand_util.h" - -namespace syncer { - -sync_pb::AttachmentIdProto CreateAttachmentIdProto() { - // Only requirement here is that this id must be globally unique. - // TODO(maniscalco): Consider making this base64 encoded. - sync_pb::AttachmentIdProto proto; - proto.set_unique_id(base::RandBytesAsString(16)); - return proto; -} - -} // namespace syncer diff --git a/sync/internal_api/public/base/attachment_id_proto.h b/sync/internal_api/public/base/attachment_id_proto.h deleted file mode 100644 index 3332d81..0000000 --- a/sync/internal_api/public/base/attachment_id_proto.h +++ /dev/null @@ -1,20 +0,0 @@ -// Copyright 2014 The Chromium Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. - -#ifndef SYNC_INTERNAL_API_PUBLIC_BASE_ATTACHMENT_ID_PROTO_H_ -#define SYNC_INTERNAL_API_PUBLIC_BASE_ATTACHMENT_ID_PROTO_H_ - -#include "sync/base/sync_export.h" -#include "sync/protocol/sync.pb.h" - -namespace syncer { - -// Helper functions that are logically part of sync_pb::AttachmentIdProto. - -// Creates a unique AttachmentIdProto. -SYNC_EXPORT_PRIVATE sync_pb::AttachmentIdProto CreateAttachmentIdProto(); - -} // namespace syncer - -#endif // SYNC_INTERNAL_API_PUBLIC_BASE_ATTACHMENT_ID_PROTO_H_ diff --git a/sync/internal_api/public/base_transaction.h b/sync/internal_api/public/base_transaction.h index c75086e..2f4fa3c 100644 --- a/sync/internal_api/public/base_transaction.h +++ b/sync/internal_api/public/base_transaction.h @@ -24,9 +24,6 @@ class Directory; // syncable, and are used in a similar way. Unlike syncable::BaseTransaction, // whose construction requires an explicit syncable::Directory, a sync // API BaseTransaction is created from a UserShare object. -// -// Note, these transactions are not atomic. Individual operations can -// fail. There is no built-in rollback or undo mechanism. class SYNC_EXPORT BaseTransaction { public: // Provide access to the underlying syncable objects from BaseNode. diff --git a/sync/internal_api/public/write_node.h b/sync/internal_api/public/write_node.h index 5b47bd6..e41c966 100644 --- a/sync/internal_api/public/write_node.h +++ b/sync/internal_api/public/write_node.h @@ -173,10 +173,6 @@ class SYNC_EXPORT WriteNode : public BaseNode { void SetPriorityPreferenceSpecifics( const sync_pb::PriorityPreferenceSpecifics& specifics); - // Set the attachment metadata. - void SetAttachmentMetadata( - const sync_pb::AttachmentMetadata& attachment_metadata); - // Implementation of BaseNode's abstract virtual accessors. virtual const syncable::Entry* GetEntry() const OVERRIDE; diff --git a/sync/internal_api/sync_manager_impl_unittest.cc b/sync/internal_api/sync_manager_impl_unittest.cc index 20b7685..a5c0c81 100644 --- a/sync/internal_api/sync_manager_impl_unittest.cc +++ b/sync/internal_api/sync_manager_impl_unittest.cc @@ -210,94 +210,22 @@ class SyncApiTest : public testing::Test { } protected: - // Create an entry with the given |model_type|, |client_tag| and - // |attachment_metadata|. - void CreateEntryWithAttachmentMetadata( - const ModelType& model_type, - const std::string& client_tag, - const sync_pb::AttachmentMetadata& attachment_metadata); - - // Attempts to load the entry specified by |model_type| and |client_tag| and - // returns the lookup result code. - BaseNode::InitByLookupResult LookupEntryByClientTag( - const ModelType& model_type, - const std::string& client_tag); - - // Replace the entry specified by |model_type| and |client_tag| with a - // tombstone. - void ReplaceWithTombstone(const ModelType& model_type, - const std::string& client_tag); - - // Save changes to the Directory, destroy it then reload it. - bool ReloadDir(); - - UserShare* user_share(); - syncable::Directory* dir(); - SyncEncryptionHandler* encryption_handler(); - - private: base::MessageLoop message_loop_; TestUserShare test_user_share_; }; -UserShare* SyncApiTest::user_share() { - return test_user_share_.user_share(); -} - -syncable::Directory* SyncApiTest::dir() { - return test_user_share_.user_share()->directory.get(); -} - -SyncEncryptionHandler* SyncApiTest::encryption_handler() { - return test_user_share_.encryption_handler(); -} - -bool SyncApiTest::ReloadDir() { - return test_user_share_.Reload(); -} - -void SyncApiTest::CreateEntryWithAttachmentMetadata( - const ModelType& model_type, - const std::string& client_tag, - const sync_pb::AttachmentMetadata& attachment_metadata) { - syncer::WriteTransaction trans(FROM_HERE, user_share()); - syncer::ReadNode root_node(&trans); - root_node.InitByRootLookup(); - syncer::WriteNode node(&trans); - ASSERT_EQ(node.InitUniqueByCreation(model_type, root_node, client_tag), - syncer::WriteNode::INIT_SUCCESS); - node.SetAttachmentMetadata(attachment_metadata); -} - -BaseNode::InitByLookupResult SyncApiTest::LookupEntryByClientTag( - const ModelType& model_type, - const std::string& client_tag) { - syncer::ReadTransaction trans(FROM_HERE, user_share()); - syncer::ReadNode node(&trans); - return node.InitByClientTagLookup(model_type, client_tag); -} - -void SyncApiTest::ReplaceWithTombstone(const ModelType& model_type, - const std::string& client_tag) { - syncer::WriteTransaction trans(FROM_HERE, user_share()); - syncer::WriteNode node(&trans); - ASSERT_EQ(node.InitByClientTagLookup(model_type, client_tag), - syncer::WriteNode::INIT_OK); - node.Tombstone(); -} - TEST_F(SyncApiTest, SanityCheckTest) { { - ReadTransaction trans(FROM_HERE, user_share()); + ReadTransaction trans(FROM_HERE, test_user_share_.user_share()); EXPECT_TRUE(trans.GetWrappedTrans()); } { - WriteTransaction trans(FROM_HERE, user_share()); + WriteTransaction trans(FROM_HERE, test_user_share_.user_share()); EXPECT_TRUE(trans.GetWrappedTrans()); } { // No entries but root should exist - ReadTransaction trans(FROM_HERE, user_share()); + ReadTransaction trans(FROM_HERE, test_user_share_.user_share()); ReadNode node(&trans); // Metahandle 1 can be root, sanity check 2 EXPECT_EQ(BaseNode::INIT_FAILED_ENTRY_NOT_GOOD, node.InitByIdLookup(2)); @@ -306,16 +234,17 @@ TEST_F(SyncApiTest, SanityCheckTest) { TEST_F(SyncApiTest, BasicTagWrite) { { - ReadTransaction trans(FROM_HERE, user_share()); + ReadTransaction trans(FROM_HERE, test_user_share_.user_share()); ReadNode root_node(&trans); root_node.InitByRootLookup(); EXPECT_EQ(root_node.GetFirstChildId(), 0); } - ignore_result(MakeNode(user_share(), BOOKMARKS, "testtag")); + ignore_result(MakeNode(test_user_share_.user_share(), + BOOKMARKS, "testtag")); { - ReadTransaction trans(FROM_HERE, user_share()); + ReadTransaction trans(FROM_HERE, test_user_share_.user_share()); ReadNode node(&trans); EXPECT_EQ(BaseNode::INIT_OK, node.InitByClientTagLookup(BOOKMARKS, "testtag")); @@ -329,18 +258,21 @@ TEST_F(SyncApiTest, BasicTagWrite) { TEST_F(SyncApiTest, ModelTypesSiloed) { { - WriteTransaction trans(FROM_HERE, user_share()); + WriteTransaction trans(FROM_HERE, test_user_share_.user_share()); ReadNode root_node(&trans); root_node.InitByRootLookup(); EXPECT_EQ(root_node.GetFirstChildId(), 0); } - ignore_result(MakeNode(user_share(), BOOKMARKS, "collideme")); - ignore_result(MakeNode(user_share(), PREFERENCES, "collideme")); - ignore_result(MakeNode(user_share(), AUTOFILL, "collideme")); + ignore_result(MakeNode(test_user_share_.user_share(), + BOOKMARKS, "collideme")); + ignore_result(MakeNode(test_user_share_.user_share(), + PREFERENCES, "collideme")); + ignore_result(MakeNode(test_user_share_.user_share(), + AUTOFILL, "collideme")); { - ReadTransaction trans(FROM_HERE, user_share()); + ReadTransaction trans(FROM_HERE, test_user_share_.user_share()); ReadNode bookmarknode(&trans); EXPECT_EQ(BaseNode::INIT_OK, @@ -365,14 +297,14 @@ TEST_F(SyncApiTest, ModelTypesSiloed) { TEST_F(SyncApiTest, ReadMissingTagsFails) { { - ReadTransaction trans(FROM_HERE, user_share()); + ReadTransaction trans(FROM_HERE, test_user_share_.user_share()); ReadNode node(&trans); EXPECT_EQ(BaseNode::INIT_FAILED_ENTRY_NOT_GOOD, node.InitByClientTagLookup(BOOKMARKS, "testtag")); } { - WriteTransaction trans(FROM_HERE, user_share()); + WriteTransaction trans(FROM_HERE, test_user_share_.user_share()); WriteNode node(&trans); EXPECT_EQ(BaseNode::INIT_FAILED_ENTRY_NOT_GOOD, node.InitByClientTagLookup(BOOKMARKS, @@ -388,7 +320,7 @@ TEST_F(SyncApiTest, TestDeleteBehavior) { std::string test_title("test1"); { - WriteTransaction trans(FROM_HERE, user_share()); + WriteTransaction trans(FROM_HERE, test_user_share_.user_share()); ReadNode root_node(&trans); root_node.InitByRootLookup(); @@ -409,7 +341,7 @@ TEST_F(SyncApiTest, TestDeleteBehavior) { // Ensure we can delete something with a tag. { - WriteTransaction trans(FROM_HERE, user_share()); + WriteTransaction trans(FROM_HERE, test_user_share_.user_share()); WriteNode wnode(&trans); EXPECT_EQ(BaseNode::INIT_OK, wnode.InitByClientTagLookup(BOOKMARKS, @@ -423,7 +355,7 @@ TEST_F(SyncApiTest, TestDeleteBehavior) { // Lookup of a node which was deleted should return failure, // but have found some data about the node. { - ReadTransaction trans(FROM_HERE, user_share()); + ReadTransaction trans(FROM_HERE, test_user_share_.user_share()); ReadNode node(&trans); EXPECT_EQ(BaseNode::INIT_FAILED_ENTRY_IS_DEL, node.InitByClientTagLookup(BOOKMARKS, @@ -434,7 +366,7 @@ TEST_F(SyncApiTest, TestDeleteBehavior) { } { - WriteTransaction trans(FROM_HERE, user_share()); + WriteTransaction trans(FROM_HERE, test_user_share_.user_share()); ReadNode folder_node(&trans); EXPECT_EQ(BaseNode::INIT_OK, folder_node.InitByIdLookup(folder_id)); @@ -452,7 +384,7 @@ TEST_F(SyncApiTest, TestDeleteBehavior) { // Now look up should work. { - ReadTransaction trans(FROM_HERE, user_share()); + ReadTransaction trans(FROM_HERE, test_user_share_.user_share()); ReadNode node(&trans); EXPECT_EQ(BaseNode::INIT_OK, node.InitByClientTagLookup(BOOKMARKS, @@ -465,11 +397,11 @@ TEST_F(SyncApiTest, TestDeleteBehavior) { TEST_F(SyncApiTest, WriteAndReadPassword) { KeyParams params = {"localhost", "username", "passphrase"}; { - ReadTransaction trans(FROM_HERE, user_share()); + ReadTransaction trans(FROM_HERE, test_user_share_.user_share()); trans.GetCryptographer()->AddKey(params); } { - WriteTransaction trans(FROM_HERE, user_share()); + WriteTransaction trans(FROM_HERE, test_user_share_.user_share()); ReadNode root_node(&trans); root_node.InitByRootLookup(); @@ -483,7 +415,7 @@ TEST_F(SyncApiTest, WriteAndReadPassword) { password_node.SetPasswordSpecifics(data); } { - ReadTransaction trans(FROM_HERE, user_share()); + ReadTransaction trans(FROM_HERE, test_user_share_.user_share()); ReadNode root_node(&trans); root_node.InitByRootLookup(); @@ -499,13 +431,13 @@ TEST_F(SyncApiTest, WriteAndReadPassword) { TEST_F(SyncApiTest, WriteEncryptedTitle) { KeyParams params = {"localhost", "username", "passphrase"}; { - ReadTransaction trans(FROM_HERE, user_share()); + ReadTransaction trans(FROM_HERE, test_user_share_.user_share()); trans.GetCryptographer()->AddKey(params); } - encryption_handler()->EnableEncryptEverything(); + test_user_share_.encryption_handler()->EnableEncryptEverything(); int bookmark_id; { - WriteTransaction trans(FROM_HERE, user_share()); + WriteTransaction trans(FROM_HERE, test_user_share_.user_share()); ReadNode root_node(&trans); root_node.InitByRootLookup(); @@ -521,7 +453,7 @@ TEST_F(SyncApiTest, WriteEncryptedTitle) { pref_node.SetTitle("bar"); } { - ReadTransaction trans(FROM_HERE, user_share()); + ReadTransaction trans(FROM_HERE, test_user_share_.user_share()); ReadNode root_node(&trans); root_node.InitByRootLookup(); @@ -540,8 +472,9 @@ TEST_F(SyncApiTest, WriteEncryptedTitle) { } TEST_F(SyncApiTest, BaseNodeSetSpecifics) { - int64 child_id = MakeNode(user_share(), BOOKMARKS, "testtag"); - WriteTransaction trans(FROM_HERE, user_share()); + int64 child_id = MakeNode(test_user_share_.user_share(), + BOOKMARKS, "testtag"); + WriteTransaction trans(FROM_HERE, test_user_share_.user_share()); WriteNode node(&trans); EXPECT_EQ(BaseNode::INIT_OK, node.InitByIdLookup(child_id)); @@ -556,8 +489,9 @@ TEST_F(SyncApiTest, BaseNodeSetSpecifics) { } TEST_F(SyncApiTest, BaseNodeSetSpecificsPreservesUnknownFields) { - int64 child_id = MakeNode(user_share(), BOOKMARKS, "testtag"); - WriteTransaction trans(FROM_HERE, user_share()); + int64 child_id = MakeNode(test_user_share_.user_share(), + BOOKMARKS, "testtag"); + WriteTransaction trans(FROM_HERE, test_user_share_.user_share()); WriteNode node(&trans); EXPECT_EQ(BaseNode::INIT_OK, node.InitByIdLookup(child_id)); EXPECT_TRUE(node.GetEntitySpecifics().unknown_fields().empty()); @@ -574,7 +508,7 @@ TEST_F(SyncApiTest, BaseNodeSetSpecificsPreservesUnknownFields) { } TEST_F(SyncApiTest, EmptyTags) { - WriteTransaction trans(FROM_HERE, user_share()); + WriteTransaction trans(FROM_HERE, test_user_share_.user_share()); ReadNode root_node(&trans); root_node.InitByRootLookup(); WriteNode node(&trans); @@ -588,9 +522,10 @@ TEST_F(SyncApiTest, EmptyTags) { // Test counting nodes when the type's root node has no children. TEST_F(SyncApiTest, GetTotalNodeCountEmpty) { - int64 type_root = MakeServerNodeForType(user_share(), BOOKMARKS); + int64 type_root = MakeServerNodeForType(test_user_share_.user_share(), + BOOKMARKS); { - ReadTransaction trans(FROM_HERE, user_share()); + ReadTransaction trans(FROM_HERE, test_user_share_.user_share()); ReadNode type_root_node(&trans); EXPECT_EQ(BaseNode::INIT_OK, type_root_node.InitByIdLookup(type_root)); @@ -600,10 +535,14 @@ TEST_F(SyncApiTest, GetTotalNodeCountEmpty) { // Test counting nodes when there is one child beneath the type's root. TEST_F(SyncApiTest, GetTotalNodeCountOneChild) { - int64 type_root = MakeServerNodeForType(user_share(), BOOKMARKS); - int64 parent = MakeFolderWithParent(user_share(), BOOKMARKS, type_root, NULL); + int64 type_root = MakeServerNodeForType(test_user_share_.user_share(), + BOOKMARKS); + int64 parent = MakeFolderWithParent(test_user_share_.user_share(), + BOOKMARKS, + type_root, + NULL); { - ReadTransaction trans(FROM_HERE, user_share()); + ReadTransaction trans(FROM_HERE, test_user_share_.user_share()); ReadNode type_root_node(&trans); EXPECT_EQ(BaseNode::INIT_OK, type_root_node.InitByIdLookup(type_root)); @@ -618,15 +557,32 @@ TEST_F(SyncApiTest, GetTotalNodeCountOneChild) { // Test counting nodes when there are multiple children beneath the type root, // and one of those children has children of its own. TEST_F(SyncApiTest, GetTotalNodeCountMultipleChildren) { - int64 type_root = MakeServerNodeForType(user_share(), BOOKMARKS); - int64 parent = MakeFolderWithParent(user_share(), BOOKMARKS, type_root, NULL); - ignore_result(MakeFolderWithParent(user_share(), BOOKMARKS, type_root, NULL)); - int64 child1 = MakeFolderWithParent(user_share(), BOOKMARKS, parent, NULL); - ignore_result(MakeBookmarkWithParent(user_share(), parent, NULL)); - ignore_result(MakeBookmarkWithParent(user_share(), child1, NULL)); - - { - ReadTransaction trans(FROM_HERE, user_share()); + int64 type_root = MakeServerNodeForType(test_user_share_.user_share(), + BOOKMARKS); + int64 parent = MakeFolderWithParent(test_user_share_.user_share(), + BOOKMARKS, + type_root, + NULL); + ignore_result(MakeFolderWithParent(test_user_share_.user_share(), + BOOKMARKS, + type_root, + NULL)); + int64 child1 = MakeFolderWithParent( + test_user_share_.user_share(), + BOOKMARKS, + parent, + NULL); + ignore_result(MakeBookmarkWithParent( + test_user_share_.user_share(), + parent, + NULL)); + ignore_result(MakeBookmarkWithParent( + test_user_share_.user_share(), + child1, + NULL)); + + { + ReadTransaction trans(FROM_HERE, test_user_share_.user_share()); ReadNode type_root_node(&trans); EXPECT_EQ(BaseNode::INIT_OK, type_root_node.InitByIdLookup(type_root)); @@ -638,57 +594,6 @@ TEST_F(SyncApiTest, GetTotalNodeCountMultipleChildren) { } } -// Verify that Directory keeps track of which attachments are referenced by -// which entries. -TEST_F(SyncApiTest, AttachmentLinking) { - // Add an entry with an attachment. - std::string tag1("some tag"); - syncer::AttachmentId attachment_id(syncer::AttachmentId::Create()); - sync_pb::AttachmentMetadata attachment_metadata; - sync_pb::AttachmentMetadataRecord* record = attachment_metadata.add_record(); - *record->mutable_id() = attachment_id.GetProto(); - ASSERT_FALSE(dir()->IsAttachmentLinked(attachment_id.GetProto())); - CreateEntryWithAttachmentMetadata(PREFERENCES, tag1, attachment_metadata); - - // See that the directory knows it's linked. - ASSERT_TRUE(dir()->IsAttachmentLinked(attachment_id.GetProto())); - - // Add a second entry referencing the same attachment. - std::string tag2("some other tag"); - CreateEntryWithAttachmentMetadata(PREFERENCES, tag2, attachment_metadata); - - // See that the directory knows it's still linked. - ASSERT_TRUE(dir()->IsAttachmentLinked(attachment_id.GetProto())); - - // Tombstone the first entry. - ReplaceWithTombstone(syncer::PREFERENCES, tag1); - - // See that the attachment is still considered linked because the entry hasn't - // been purged from the Directory. - ASSERT_TRUE(dir()->IsAttachmentLinked(attachment_id.GetProto())); - - // Save changes and see that the entry is truly gone. - ASSERT_TRUE(dir()->SaveChanges()); - ASSERT_EQ(LookupEntryByClientTag(PREFERENCES, tag1), - syncer::WriteNode::INIT_FAILED_ENTRY_NOT_GOOD); - - // However, the attachment is still linked. - ASSERT_TRUE(dir()->IsAttachmentLinked(attachment_id.GetProto())); - - // Save, destroy, and recreate the directory. See that it's still linked. - ASSERT_TRUE(ReloadDir()); - ASSERT_TRUE(dir()->IsAttachmentLinked(attachment_id.GetProto())); - - // Tombstone the second entry, save changes, see that it's truly gone. - ReplaceWithTombstone(syncer::PREFERENCES, tag2); - ASSERT_TRUE(dir()->SaveChanges()); - ASSERT_EQ(LookupEntryByClientTag(PREFERENCES, tag2), - syncer::WriteNode::INIT_FAILED_ENTRY_NOT_GOOD); - - // Finally, the attachment is no longer linked. - ASSERT_FALSE(dir()->IsAttachmentLinked(attachment_id.GetProto())); -} - namespace { class TestHttpPostProviderInterface : public HttpPostProviderInterface { diff --git a/sync/internal_api/write_node.cc b/sync/internal_api/write_node.cc index c3ef081..6a634eb 100644 --- a/sync/internal_api/write_node.cc +++ b/sync/internal_api/write_node.cc @@ -464,11 +464,6 @@ bool WriteNode::SetPosition(const BaseNode& new_parent, return PutPredecessor(predecessor); } -void WriteNode::SetAttachmentMetadata( - const sync_pb::AttachmentMetadata& attachment_metadata) { - entry_->PutAttachmentMetadata(attachment_metadata); -} - const syncable::Entry* WriteNode::GetEntry() const { return entry_; } diff --git a/sync/protocol/attachments.proto b/sync/protocol/attachments.proto index e08e2b0..50815d3 100644 --- a/sync/protocol/attachments.proto +++ b/sync/protocol/attachments.proto @@ -21,16 +21,7 @@ message AttachmentIdProto { optional string unique_id = 1; } -// Metadata for a single attachment. -message AttachmentMetadataRecord { - optional AttachmentIdProto id = 1; - // Indicates we know this attachment exists on the server. - optional bool is_on_server = 2; -} - // A collection of attachment metadata. This proto is part of EntryKernel's "on // disk" representation. Private to sync. message AttachmentMetadata { - // One record per attachment. - repeated AttachmentMetadataRecord record = 1; } diff --git a/sync/sync_internal_api.gypi b/sync/sync_internal_api.gypi index 6f9af0328..6078dd8 100644 --- a/sync/sync_internal_api.gypi +++ b/sync/sync_internal_api.gypi @@ -45,8 +45,6 @@ 'internal_api/protocol_event_buffer.h', 'internal_api/public/base/ack_handle.cc', 'internal_api/public/base/ack_handle.h', - 'internal_api/public/base/attachment_id_proto.cc', - 'internal_api/public/base/attachment_id_proto.h', 'internal_api/public/base/cancelation_observer.cc', 'internal_api/public/base/cancelation_observer.h', 'internal_api/public/base/cancelation_signal.cc', diff --git a/sync/syncable/directory.cc b/sync/syncable/directory.cc index cebb2d4..75c1bec 100644 --- a/sync/syncable/directory.cc +++ b/sync/syncable/directory.cc @@ -10,7 +10,6 @@ #include "base/debug/trace_event.h" #include "base/stl_util.h" #include "base/strings/string_number_conversions.h" -#include "sync/internal_api/public/base/attachment_id_proto.h" #include "sync/internal_api/public/base/unique_position.h" #include "sync/internal_api/public/util/unrecoverable_error_handler.h" #include "sync/syncable/entry.h" @@ -124,7 +123,6 @@ DirOpenResult Directory::Open( } void Directory::InitializeIndices(MetahandlesMap* handles_map) { - ScopedKernelLock lock(this); kernel_->metahandles_map.swap(*handles_map); for (MetahandlesMap::const_iterator it = kernel_->metahandles_map.begin(); it != kernel_->metahandles_map.end(); ++it) { @@ -154,7 +152,6 @@ void Directory::InitializeIndices(MetahandlesMap* handles_map) { kernel_->ids_map.end()) << "Unexpected duplicate use of ID"; kernel_->ids_map[entry->ref(ID).value()] = entry; DCHECK(!entry->is_dirty()); - AddToAttachmentIndex(metahandle, entry->ref(ATTACHMENT_METADATA), lock); } } @@ -365,8 +362,6 @@ bool Directory::InsertEntry(BaseWriteTransaction* trans, return false; } } - AddToAttachmentIndex( - entry->ref(META_HANDLE), entry->ref(ATTACHMENT_METADATA), *lock); // Should NEVER be created with a client tag or server tag. if (!SyncAssert(entry->ref(UNIQUE_SERVER_TAG).empty(), FROM_HERE, @@ -413,51 +408,6 @@ bool Directory::ReindexParentId(BaseWriteTransaction* trans, return true; } -void Directory::RemoveFromAttachmentIndex( - const int64 metahandle, - const sync_pb::AttachmentMetadata& attachment_metadata, - const ScopedKernelLock& lock) { - for (int i = 0; i < attachment_metadata.record_size(); ++i) { - AttachmentIdUniqueId unique_id = - attachment_metadata.record(i).id().unique_id(); - IndexByAttachmentId::iterator iter = - kernel_->index_by_attachment_id.find(unique_id); - if (iter != kernel_->index_by_attachment_id.end()) { - iter->second.erase(metahandle); - if (iter->second.empty()) { - kernel_->index_by_attachment_id.erase(iter); - } - } - } -} - -void Directory::AddToAttachmentIndex( - const int64 metahandle, - const sync_pb::AttachmentMetadata& attachment_metadata, - const ScopedKernelLock& lock) { - for (int i = 0; i < attachment_metadata.record_size(); ++i) { - AttachmentIdUniqueId unique_id = - attachment_metadata.record(i).id().unique_id(); - IndexByAttachmentId::iterator iter = - kernel_->index_by_attachment_id.find(unique_id); - if (iter == kernel_->index_by_attachment_id.end()) { - iter = kernel_->index_by_attachment_id.insert(std::make_pair( - unique_id, - MetahandleSet())).first; - } - iter->second.insert(metahandle); - } -} - -void Directory::UpdateAttachmentIndex( - const int64 metahandle, - const sync_pb::AttachmentMetadata& old_metadata, - const sync_pb::AttachmentMetadata& new_metadata) { - ScopedKernelLock lock(this); - RemoveFromAttachmentIndex(metahandle, old_metadata, lock); - AddToAttachmentIndex(metahandle, new_metadata, lock); -} - bool Directory::unrecoverable_error_set(const BaseTransaction* trans) const { DCHECK(trans != NULL); return unrecoverable_error_set_; @@ -598,9 +548,6 @@ bool Directory::VacuumAfterSaveChanges(const SaveChangesSnapshot& snapshot) { "Deleted entry still present", (&trans))) return false; - RemoveFromAttachmentIndex( - entry->ref(META_HANDLE), entry->ref(ATTACHMENT_METADATA), lock); - delete entry; } if (trans.unrecoverable_error_set()) @@ -660,8 +607,7 @@ void Directory::UnapplyEntry(EntryKernel* entry) { void Directory::DeleteEntry(bool save_to_journal, EntryKernel* entry, - EntryKernelSet* entries_to_journal, - const ScopedKernelLock& lock) { + EntryKernelSet* entries_to_journal) { int64 handle = entry->ref(META_HANDLE); ModelType server_type = GetModelTypeFromSpecifics( entry->ref(SERVER_SPECIFICS)); @@ -691,7 +637,6 @@ void Directory::DeleteEntry(bool save_to_journal, kernel_->server_tags_map.erase(entry->ref(UNIQUE_SERVER_TAG)); DCHECK_EQ(1u, num_erased); } - RemoveFromAttachmentIndex(handle, entry->ref(ATTACHMENT_METADATA), lock); if (save_to_journal) { entries_to_journal->insert(entry); @@ -759,7 +704,7 @@ bool Directory::PurgeEntriesWithTypeIn(ModelTypeSet disabled_types, types_to_journal.Has(server_type)) && (delete_journal_->IsDeleteJournalEnabled(local_type) || delete_journal_->IsDeleteJournalEnabled(server_type)); - DeleteEntry(save_to_journal, entry, &entries_to_journal, lock); + DeleteEntry(save_to_journal, entry, &entries_to_journal); } } @@ -817,17 +762,6 @@ bool Directory::ResetVersionsForType(BaseWriteTransaction* trans, return true; } -bool Directory::IsAttachmentLinked( - const sync_pb::AttachmentIdProto& attachment_id_proto) const { - ScopedKernelLock lock(this); - IndexByAttachmentId::const_iterator iter = - kernel_->index_by_attachment_id.find(attachment_id_proto.unique_id()); - if (iter != kernel_->index_by_attachment_id.end() && !iter->second.empty()) { - return true; - } - return false; -} - void Directory::HandleSaveChangesFailure(const SaveChangesSnapshot& snapshot) { WriteTransaction trans(FROM_HERE, HANDLE_SAVE_FAILURE, this); ScopedKernelLock lock(this); diff --git a/sync/syncable/directory.h b/sync/syncable/directory.h index 5d26bbd..fdac9b4 100644 --- a/sync/syncable/directory.h +++ b/sync/syncable/directory.h @@ -47,9 +47,6 @@ enum InvariantCheckLevel { FULL_DB_VERIFICATION = 2 // Check every entry. This can be expensive. }; -// Directory stores and manages EntryKernels. -// -// This class is tightly coupled to several other classes (see friends). class SYNC_EXPORT Directory { friend class BaseTransaction; friend class Entry; @@ -83,9 +80,6 @@ class SYNC_EXPORT Directory { typedef base::hash_map<int64, EntryKernel*> MetahandlesMap; typedef base::hash_map<std::string, EntryKernel*> IdsMap; typedef base::hash_map<std::string, EntryKernel*> TagsMap; - typedef std::string AttachmentIdUniqueId; - typedef base::hash_map<AttachmentIdUniqueId, MetahandleSet> - IndexByAttachmentId; static const base::FilePath::CharType kSyncDatabaseFilename[]; @@ -389,14 +383,6 @@ class SYNC_EXPORT Directory { // WARNING! This can be slow, as it iterates over all entries for a type. bool ResetVersionsForType(BaseWriteTransaction* trans, ModelType type); - // Returns true iff the attachment identified by |attachment_id_proto| is - // linked to an entry. - // - // An attachment linked to a deleted entry is still considered linked if the - // entry hasn't yet been purged. - bool IsAttachmentLinked( - const sync_pb::AttachmentIdProto& attachment_id_proto) const; - protected: // for friends, mainly used by Entry constructors virtual EntryKernel* GetEntryByHandle(int64 handle); virtual EntryKernel* GetEntryByHandle(int64 metahandle, @@ -408,11 +394,6 @@ class SYNC_EXPORT Directory { const Id& new_id); bool ReindexParentId(BaseWriteTransaction* trans, EntryKernel* const entry, const Id& new_parent_id); - // Update the attachment index for |metahandle| removing it from the index - // under |old_metadata| entries and add it under |new_metadata| entries. - void UpdateAttachmentIndex(const int64 metahandle, - const sync_pb::AttachmentMetadata& old_metadata, - const sync_pb::AttachmentMetadata& new_metadata); void ClearDirtyMetahandles(); DirOpenResult OpenImpl( @@ -470,17 +451,6 @@ class SYNC_EXPORT Directory { // within parent. Protected by the ScopedKernelLock. ParentChildIndex parent_child_index; - // This index keeps track of which metahandles refer to a given attachment. - // Think of it as the inverse of EntryKernel's AttachmentMetadata Records. - // - // Because entries can be undeleted (e.g. PutIsDel(false)), entries should - // not removed from the index until they are actually deleted from memory. - // - // All access should go through IsAttachmentLinked, - // RemoveFromAttachmentIndex, AddToAttachmentIndex, and - // UpdateAttachmentIndex methods to avoid iterator invalidation errors. - IndexByAttachmentId index_by_attachment_id; - // 3 in-memory indices on bits used extremely frequently by the syncer. // |unapplied_update_metahandles| is keyed by the server model type. MetahandleSet unapplied_update_metahandles[MODEL_TYPE_COUNT]; @@ -573,19 +543,7 @@ class SYNC_EXPORT Directory { void UnapplyEntry(EntryKernel* entry); void DeleteEntry(bool save_to_journal, EntryKernel* entry, - EntryKernelSet* entries_to_journal, - const ScopedKernelLock& lock); - - // Remove each of |metahandle|'s attachment ids from index_by_attachment_id. - void RemoveFromAttachmentIndex( - const int64 metahandle, - const sync_pb::AttachmentMetadata& attachment_metadata, - const ScopedKernelLock& lock); - // Add each of |metahandle|'s attachment ids to the index_by_attachment_id. - void AddToAttachmentIndex( - const int64 metahandle, - const sync_pb::AttachmentMetadata& attachment_metadata, - const ScopedKernelLock& lock); + EntryKernelSet* entries_to_journal); Kernel* kernel_; diff --git a/sync/syncable/directory_backing_store.cc b/sync/syncable/directory_backing_store.cc index ec28a53..e466548 100644 --- a/sync/syncable/directory_backing_store.cc +++ b/sync/syncable/directory_backing_store.cc @@ -70,11 +70,6 @@ void BindFields(const EntryKernel& entry, entry.ref(static_cast<UniquePositionField>(i)).SerializeToString(&temp); statement->BindBlob(index++, temp.data(), temp.length()); } - for (; i < ATTACHMENT_METADATA_FIELDS_END; ++i) { - std::string temp; - entry.ref(static_cast<AttachmentMetadataField>(i)).SerializeToString(&temp); - statement->BindBlob(index++, temp.data(), temp.length()); - } } // The caller owns the returned EntryKernel*. Assumes the statement currently @@ -119,10 +114,6 @@ scoped_ptr<EntryKernel> UnpackEntry(sql::Statement* statement) { kernel->mutable_ref(static_cast<UniquePositionField>(i)) = UniquePosition::FromProto(proto); } - for (; i < ATTACHMENT_METADATA_FIELDS_END; ++i) { - kernel->mutable_ref(static_cast<AttachmentMetadataField>(i)).ParseFromArray( - statement->ColumnBlob(i), statement->ColumnByteLength(i)); - } return kernel.Pass(); } diff --git a/sync/syncable/directory_unittest.cc b/sync/syncable/directory_unittest.cc index f58f54f..b2bff36 100644 --- a/sync/syncable/directory_unittest.cc +++ b/sync/syncable/directory_unittest.cc @@ -6,7 +6,6 @@ #include "base/strings/stringprintf.h" #include "base/test/values_test_util.h" -#include "sync/internal_api/public/base/attachment_id_proto.h" #include "sync/syncable/syncable_proto_util.h" #include "sync/syncable/syncable_util.h" #include "sync/syncable/syncable_write_transaction.h" @@ -85,45 +84,23 @@ DirOpenResult SyncableDirectoryTest::ReopenDirectory() { } // Creates an empty entry and sets the ID field to a default one. -void SyncableDirectoryTest::CreateEntry(const ModelType& model_type, - const std::string& entryname) { - CreateEntry(model_type, entryname, TestIdFactory::FromNumber(-99)); +void SyncableDirectoryTest::CreateEntry(const std::string& entryname) { + CreateEntry(entryname, TestIdFactory::FromNumber(-99)); } // Creates an empty entry and sets the ID field to id. -void SyncableDirectoryTest::CreateEntry(const ModelType& model_type, - const std::string& entryname, +void SyncableDirectoryTest::CreateEntry(const std::string& entryname, const int id) { - CreateEntry(model_type, entryname, TestIdFactory::FromNumber(id)); + CreateEntry(entryname, TestIdFactory::FromNumber(id)); } - -void SyncableDirectoryTest::CreateEntry(const ModelType& model_type, - const std::string& entryname, - const Id& id) { - CreateEntryWithAttachmentMetadata( - model_type, entryname, id, sync_pb::AttachmentMetadata()); -} - -void SyncableDirectoryTest::CreateEntryWithAttachmentMetadata( - const ModelType& model_type, - const std::string& entryname, - const Id& id, - const sync_pb::AttachmentMetadata& attachment_metadata) { +void SyncableDirectoryTest::CreateEntry(const std::string& entryname, Id id) { WriteTransaction wtrans(FROM_HERE, UNITTEST, dir_.get()); - MutableEntry me(&wtrans, CREATE, model_type, wtrans.root_id(), entryname); + MutableEntry me(&wtrans, CREATE, BOOKMARKS, wtrans.root_id(), entryname); ASSERT_TRUE(me.good()); me.PutId(id); - me.PutAttachmentMetadata(attachment_metadata); me.PutIsUnsynced(true); } -void SyncableDirectoryTest::DeleteEntry(const Id& id) { - WriteTransaction trans(FROM_HERE, UNITTEST, dir().get()); - MutableEntry entry(&trans, GET_BY_ID, id); - ASSERT_TRUE(entry.good()); - entry.PutIsDel(true); -} - DirOpenResult SyncableDirectoryTest::SimulateSaveAndReloadDir() { if (!dir_->SaveChanges()) return FAILED_IN_UNITTEST; @@ -434,8 +411,8 @@ TEST_F(SyncableDirectoryTest, ManageDeleteJournals) { int64 handle2 = 0; { // Create two bookmark entries and save in database. - CreateEntry(BOOKMARKS, "item1", id1); - CreateEntry(BOOKMARKS, "item2", id2); + CreateEntry("item1", id1); + CreateEntry("item2", id2); { WriteTransaction trans(FROM_HERE, UNITTEST, dir().get()); MutableEntry item1(&trans, GET_BY_ID, id1); @@ -552,7 +529,7 @@ TEST_F(SyncableDirectoryTest, TestBasicLookupNonExistantID) { } TEST_F(SyncableDirectoryTest, TestBasicLookupValidID) { - CreateEntry(BOOKMARKS, "rtc"); + CreateEntry("rtc"); ReadTransaction rtrans(FROM_HERE, dir().get()); Entry e(&rtrans, GET_BY_ID, TestIdFactory::FromNumber(-99)); ASSERT_TRUE(e.good()); @@ -1544,100 +1521,6 @@ TEST_F(SyncableDirectoryTest, StressTransactions) { } } -// Verify that Directory is notifed when a MutableEntry's AttachmentMetadata -// changes. -TEST_F(SyncableDirectoryTest, MutableEntry_PutAttachmentMetadata) { - sync_pb::AttachmentMetadata attachment_metadata; - sync_pb::AttachmentMetadataRecord* record = attachment_metadata.add_record(); - sync_pb::AttachmentIdProto attachment_id_proto = - syncer::CreateAttachmentIdProto(); - *record->mutable_id() = attachment_id_proto; - ASSERT_FALSE(dir()->IsAttachmentLinked(attachment_id_proto)); - { - WriteTransaction trans(FROM_HERE, UNITTEST, dir().get()); - - // Create an entry with attachment metadata and see that the attachment id - // is not linked. - MutableEntry entry( - &trans, CREATE, PREFERENCES, trans.root_id(), "some entry"); - entry.PutId(TestIdFactory::FromNumber(-1)); - entry.PutIsUnsynced(true); - ASSERT_FALSE(dir()->IsAttachmentLinked(attachment_id_proto)); - - // Now add the attachment metadata and see that Directory believes it is - // linked. - entry.PutAttachmentMetadata(attachment_metadata); - ASSERT_TRUE(dir()->IsAttachmentLinked(attachment_id_proto)); - - // Clear out the attachment metadata and see that it's no longer linked. - sync_pb::AttachmentMetadata empty_attachment_metadata; - entry.PutAttachmentMetadata(empty_attachment_metadata); - ASSERT_FALSE(dir()->IsAttachmentLinked(attachment_id_proto)); - } - ASSERT_FALSE(dir()->IsAttachmentLinked(attachment_id_proto)); -} - -// Verify that deleted entries with attachments will retain the attachments. -TEST_F(SyncableDirectoryTest, Directory_DeleteDoesNotUnlinkAttachments) { - sync_pb::AttachmentMetadata attachment_metadata; - sync_pb::AttachmentMetadataRecord* record = attachment_metadata.add_record(); - sync_pb::AttachmentIdProto attachment_id_proto = - syncer::CreateAttachmentIdProto(); - *record->mutable_id() = attachment_id_proto; - ASSERT_FALSE(dir()->IsAttachmentLinked(attachment_id_proto)); - const Id id = TestIdFactory::FromNumber(-1); - - // Create an entry with attachment metadata and see that the attachment id - // is linked. - CreateEntryWithAttachmentMetadata( - PREFERENCES, "some entry", id, attachment_metadata); - ASSERT_TRUE(dir()->IsAttachmentLinked(attachment_id_proto)); - - // Delete the entry and see that it's still linked because the entry hasn't - // yet been purged. - DeleteEntry(id); - ASSERT_TRUE(dir()->IsAttachmentLinked(attachment_id_proto)); - - // Reload the Directory, purging the deleted entry, and see that the - // attachment is no longer linked. - SimulateSaveAndReloadDir(); - ASSERT_FALSE(dir()->IsAttachmentLinked(attachment_id_proto)); -} - -// Verify that a given attachment can be referenced by multiple entries and that -// any one of the references is sufficient to ensure it remains linked. -TEST_F(SyncableDirectoryTest, Directory_LastReferenceUnlinksAttachments) { - // Create one attachment. - sync_pb::AttachmentMetadata attachment_metadata; - sync_pb::AttachmentMetadataRecord* record = attachment_metadata.add_record(); - sync_pb::AttachmentIdProto attachment_id_proto = - syncer::CreateAttachmentIdProto(); - *record->mutable_id() = attachment_id_proto; - - // Create two entries, each referencing the attachment. - const Id id1 = TestIdFactory::FromNumber(-1); - const Id id2 = TestIdFactory::FromNumber(-2); - CreateEntryWithAttachmentMetadata( - PREFERENCES, "some entry", id1, attachment_metadata); - CreateEntryWithAttachmentMetadata( - PREFERENCES, "some other entry", id2, attachment_metadata); - - // See that the attachment is considered linked. - ASSERT_TRUE(dir()->IsAttachmentLinked(attachment_id_proto)); - - // Delete the first entry, reload the Directory, see that the attachment is - // still linked. - DeleteEntry(id1); - SimulateSaveAndReloadDir(); - ASSERT_TRUE(dir()->IsAttachmentLinked(attachment_id_proto)); - - // Delete the second entry, reload the Directory, see that the attachment is - // no loner linked. - DeleteEntry(id2); - SimulateSaveAndReloadDir(); - ASSERT_FALSE(dir()->IsAttachmentLinked(attachment_id_proto)); -} - } // namespace syncable } // namespace syncer diff --git a/sync/syncable/directory_unittest.h b/sync/syncable/directory_unittest.h index 81e3888..14e1a99 100644 --- a/sync/syncable/directory_unittest.h +++ b/sync/syncable/directory_unittest.h @@ -31,6 +31,7 @@ class BaseTransaction; // // Serves as base class for several other test fixtures. class SyncableDirectoryTest : public testing::Test { + private: protected: static const char kDirectoryName[]; @@ -46,24 +47,12 @@ class SyncableDirectoryTest : public testing::Test { DirOpenResult ReopenDirectory(); // Creates an empty entry and sets the ID field to a default one. - void CreateEntry(const ModelType& model_type, const std::string& entryname); + void CreateEntry(const std::string& entryname); // Creates an empty entry and sets the ID field to id. - void CreateEntry(const ModelType& model_type, - const std::string& entryname, - const int id); - - void CreateEntry(const ModelType& model_type, - const std::string& entryname, - const Id& id); - - void CreateEntryWithAttachmentMetadata( - const ModelType& model_type, - const std::string& entryname, - const Id& id, - const sync_pb::AttachmentMetadata& attachment_metadata); + void CreateEntry(const std::string& entryname, const int id); - void DeleteEntry(const Id& id); + void CreateEntry(const std::string& entryname, Id id); // When a directory is saved then loaded from disk, it will pass through // DropDeletedEntries(). This will remove some entries from the directory. diff --git a/sync/syncable/entry_kernel.h b/sync/syncable/entry_kernel.h index 6380420..b6bc2ab 100644 --- a/sync/syncable/entry_kernel.h +++ b/sync/syncable/entry_kernel.h @@ -333,10 +333,6 @@ struct SYNC_EXPORT_PRIVATE EntryKernel { inline UniquePosition& mutable_ref(UniquePositionField field) { return unique_position_fields[field - UNIQUE_POSITION_FIELDS_BEGIN]; } - inline sync_pb::AttachmentMetadata& mutable_ref( - AttachmentMetadataField field) { - return attachment_metadata_fields[field - ATTACHMENT_METADATA_FIELDS_BEGIN]; - } ModelType GetModelType() const; ModelType GetServerModelType() const; diff --git a/sync/syncable/mutable_entry.cc b/sync/syncable/mutable_entry.cc index d9d5daa..863e65b 100644 --- a/sync/syncable/mutable_entry.cc +++ b/sync/syncable/mutable_entry.cc @@ -233,20 +233,6 @@ bool MutableEntry::PutPredecessor(const Id& predecessor_id) { return true; } -void MutableEntry::PutAttachmentMetadata( - const sync_pb::AttachmentMetadata& attachment_metadata) { - DCHECK(kernel_); - write_transaction()->TrackChangesTo(kernel_); - if (kernel_->ref(ATTACHMENT_METADATA).SerializeAsString() != - attachment_metadata.SerializeAsString()) { - dir()->UpdateAttachmentIndex(GetMetahandle(), - kernel_->ref(ATTACHMENT_METADATA), - attachment_metadata); - kernel_->put(ATTACHMENT_METADATA, attachment_metadata); - kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles); - } -} - // This function sets only the flags needed to get this entry to sync. bool MarkForSyncing(MutableEntry* e) { DCHECK_NE(static_cast<MutableEntry*>(NULL), e); diff --git a/sync/syncable/mutable_entry.h b/sync/syncable/mutable_entry.h index c064358..8c2f2ab 100644 --- a/sync/syncable/mutable_entry.h +++ b/sync/syncable/mutable_entry.h @@ -59,9 +59,6 @@ class SYNC_EXPORT_PRIVATE MutableEntry : public ModelNeutralMutableEntry { // ID to put the node in first position. bool PutPredecessor(const Id& predecessor_id); - void PutAttachmentMetadata( - const sync_pb::AttachmentMetadata& attachment_metadata); - private: // Kind of redundant. We should reduce the number of pointers // floating around if at all possible. Could we store this in Directory? |