summaryrefslogtreecommitdiffstats
path: root/sync
diff options
context:
space:
mode:
authormaniscalco@chromium.org <maniscalco@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>2014-05-01 22:10:39 +0000
committermaniscalco@chromium.org <maniscalco@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98>2014-05-01 22:10:39 +0000
commitd9ff32719de688849812cb10cf232885d85ab298 (patch)
tree43757821f6a3d96d25983fb4d2708a2184f9e5db /sync
parentb6cd47221942b3b7352a87d03dcce2855bed0e79 (diff)
downloadchromium_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')
-rw-r--r--sync/api/attachments/attachment_id.cc7
-rw-r--r--sync/api/attachments/attachment_service.h23
-rw-r--r--sync/api/attachments/attachment_service_proxy.cc25
-rw-r--r--sync/api/attachments/attachment_service_proxy.h6
-rw-r--r--sync/api/attachments/attachment_service_proxy_unittest.cc24
-rw-r--r--sync/api/attachments/fake_attachment_service.cc23
-rw-r--r--sync/api/attachments/fake_attachment_service.h5
-rw-r--r--sync/api/sync_data.cc22
-rw-r--r--sync/api/sync_data.h15
-rw-r--r--sync/engine/get_commit_ids.cc10
-rw-r--r--sync/internal_api/base_node.cc11
-rw-r--r--sync/internal_api/public/base/attachment_id_proto.cc19
-rw-r--r--sync/internal_api/public/base/attachment_id_proto.h20
-rw-r--r--sync/internal_api/public/base_transaction.h3
-rw-r--r--sync/internal_api/public/write_node.h4
-rw-r--r--sync/internal_api/sync_manager_impl_unittest.cc239
-rw-r--r--sync/internal_api/write_node.cc5
-rw-r--r--sync/protocol/attachments.proto9
-rw-r--r--sync/sync_internal_api.gypi2
-rw-r--r--sync/syncable/directory.cc70
-rw-r--r--sync/syncable/directory.h44
-rw-r--r--sync/syncable/directory_backing_store.cc9
-rw-r--r--sync/syncable/directory_unittest.cc135
-rw-r--r--sync/syncable/directory_unittest.h19
-rw-r--r--sync/syncable/entry_kernel.h4
-rw-r--r--sync/syncable/mutable_entry.cc14
-rw-r--r--sync/syncable/mutable_entry.h3
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?