diff options
author | stanisc <stanisc@chromium.org> | 2014-12-22 16:02:38 -0800 |
---|---|---|
committer | Commit bot <commit-bot@chromium.org> | 2014-12-23 00:03:31 +0000 |
commit | 60c25aebaf6c3bbc90c9c82f83a752ca58c7ddce (patch) | |
tree | ba1e9704812c4df07936e70948e3e52aa0c532b2 /sync/syncable | |
parent | 5794dbf611e14653d3ccfc8778b9052b0c129909 (diff) | |
download | chromium_src-60c25aebaf6c3bbc90c9c82f83a752ca58c7ddce.zip chromium_src-60c25aebaf6c3bbc90c9c82f83a752ca58c7ddce.tar.gz chromium_src-60c25aebaf6c3bbc90c9c82f83a752ca58c7ddce.tar.bz2 |
Enable Null Syncable ID which is different than Root ID.
This patch prepares sync codebase for supporting datatypes with
implicit permanent forlders by introducing an empty/unset
syncable ID.
There was a notion of Null ID before but it was equivalent
to the root ID which caused some confusion in the code - a Null
ID would sometimes be used where a "root" would be appropriate
and vice versa.
See the following code fragment for an example:
// TODO(sync): We could use null here, but to ease conversion we use "r".
// fix this, this is madness :)
inline bool IsNull() const {
return IsRoot();
}
This change will be followed by one or two separate changes
that will actually remove dependencies on PARENT_ID and
SERVER_PARENT_ID and use a different way to organize nodes
in the directory based on node's datatype rather than parent ID.
This change impacts the following:
1) The default ID value in EntryKernel ID fields and elsewhere is
Null ID and not Root ID as it used to be.
2) This means that some tests that depended on Root ID being
set implicitly
in PARENT_ID and SERVER_PARENT_ID now need to set these IDs explicitly.
3) Node's GetPredecessorId, GetSuccessorId, and GetFirstChildId
methods now return a null ID rather than a root ID when there
is no predecessor, successor, or children.
4) Small changed throughout the code to make sure that
null IDs and root IDs are used accordingly (now that they
are no longer equivalent).
5) Changed some validation methods to accept Null IDs
to avoid large changes in the test code.
BUG=438313
Review URL: https://codereview.chromium.org/805633004
Cr-Commit-Position: refs/heads/master@{#309495}
Diffstat (limited to 'sync/syncable')
-rw-r--r-- | sync/syncable/directory.cc | 5 | ||||
-rw-r--r-- | sync/syncable/directory_backing_store.cc | 8 | ||||
-rw-r--r-- | sync/syncable/directory_unittest.cc | 4 | ||||
-rw-r--r-- | sync/syncable/entry_kernel.h | 1 | ||||
-rw-r--r-- | sync/syncable/mutable_entry.cc | 12 | ||||
-rw-r--r-- | sync/syncable/nigori_util.cc | 2 | ||||
-rw-r--r-- | sync/syncable/parent_child_index_unittest.cc | 14 | ||||
-rw-r--r-- | sync/syncable/syncable_base_transaction.cc | 2 | ||||
-rw-r--r-- | sync/syncable/syncable_id.cc | 12 | ||||
-rw-r--r-- | sync/syncable/syncable_id.h | 19 |
10 files changed, 40 insertions, 39 deletions
diff --git a/sync/syncable/directory.cc b/sync/syncable/directory.cc index 9c59409..22c3f46 100644 --- a/sync/syncable/directory.cc +++ b/sync/syncable/directory.cc @@ -1192,6 +1192,7 @@ bool Directory::CheckTreeInvariants(syncable::BaseTransaction* trans, trans)) return false; int safety_count = handles.size() + 1; + // TODO(stanisc): handle items with Null parentid while (!parentid.IsRoot()) { Entry parent(trans, GET_BY_ID, parentid); if (!SyncAssert(parent.good(), FROM_HERE, @@ -1392,13 +1393,13 @@ void Directory::PutPredecessor(EntryKernel* e, EntryKernel* predecessor) { if (!siblings) { // This parent currently has no other children. - DCHECK(predecessor->ref(ID).IsRoot()); + DCHECK(predecessor == NULL); UniquePosition pos = UniquePosition::InitialPosition(suffix); e->put(UNIQUE_POSITION, pos); return; } - if (predecessor->ref(ID).IsRoot()) { + if (predecessor == NULL) { // We have at least one sibling, and we're inserting to the left of them. UniquePosition successor_pos = (*siblings->begin())->ref(UNIQUE_POSITION); diff --git a/sync/syncable/directory_backing_store.cc b/sync/syncable/directory_backing_store.cc index 6d08b75..536af35 100644 --- a/sync/syncable/directory_backing_store.cc +++ b/sync/syncable/directory_backing_store.cc @@ -1548,9 +1548,11 @@ bool DirectoryBackingStore::VerifyReferenceIntegrity( for (Directory::MetahandlesMap::const_iterator it = handles_map->begin(); it != handles_map->end(); ++it) { EntryKernel* entry = it->second; - bool parent_exists = (ids_set.find(entry->ref(PARENT_ID).value()) != end); - if (!parent_exists) { - return false; + if (!entry->ref(PARENT_ID).IsNull()) { + bool parent_exists = (ids_set.find(entry->ref(PARENT_ID).value()) != end); + if (!parent_exists) { + return false; + } } } return is_ok; diff --git a/sync/syncable/directory_unittest.cc b/sync/syncable/directory_unittest.cc index 6aaf054..b891dd7 100644 --- a/sync/syncable/directory_unittest.cc +++ b/sync/syncable/directory_unittest.cc @@ -1349,7 +1349,7 @@ TEST_F(SyncableDirectoryTest, ChildrenOps) { Entry root(&rtrans, GET_BY_ID, rtrans.root_id()); ASSERT_TRUE(root.good()); EXPECT_FALSE(dir()->HasChildren(&rtrans, rtrans.root_id())); - EXPECT_TRUE(root.GetFirstChildId().IsRoot()); + EXPECT_TRUE(root.GetFirstChildId().IsNull()); } { @@ -1392,7 +1392,7 @@ TEST_F(SyncableDirectoryTest, ChildrenOps) { Entry root(&rtrans, GET_BY_ID, rtrans.root_id()); ASSERT_TRUE(root.good()); EXPECT_FALSE(dir()->HasChildren(&rtrans, rtrans.root_id())); - EXPECT_TRUE(root.GetFirstChildId().IsRoot()); + EXPECT_TRUE(root.GetFirstChildId().IsNull()); } dir()->SaveChanges(); diff --git a/sync/syncable/entry_kernel.h b/sync/syncable/entry_kernel.h index 0c2f471..f5c4017 100644 --- a/sync/syncable/entry_kernel.h +++ b/sync/syncable/entry_kernel.h @@ -247,6 +247,7 @@ struct SYNC_EXPORT_PRIVATE EntryKernel { ProtoTimeToTime(TimeToProtoTime(value)); } inline void put(IdField field, const Id& value) { + DCHECK(!value.IsNull()); id_fields[field - ID_FIELDS_BEGIN] = value; } inline void put(BaseVersion field, int64 value) { diff --git a/sync/syncable/mutable_entry.cc b/sync/syncable/mutable_entry.cc index 0aa37e7..0cc06f9 100644 --- a/sync/syncable/mutable_entry.cc +++ b/sync/syncable/mutable_entry.cc @@ -227,10 +227,14 @@ void MutableEntry::PutUniquePosition(const UniquePosition& value) { } bool MutableEntry::PutPredecessor(const Id& predecessor_id) { - MutableEntry predecessor(write_transaction(), GET_BY_ID, predecessor_id); - if (!predecessor.good()) - return false; - dir()->PutPredecessor(kernel_, predecessor.kernel_); + if (predecessor_id.IsNull()) { + dir()->PutPredecessor(kernel_, NULL); + } else { + MutableEntry predecessor(write_transaction(), GET_BY_ID, predecessor_id); + if (!predecessor.good()) + return false; + dir()->PutPredecessor(kernel_, predecessor.kernel_); + } return true; } diff --git a/sync/syncable/nigori_util.cc b/sync/syncable/nigori_util.cc index 2731e7f..b43acfa 100644 --- a/sync/syncable/nigori_util.cc +++ b/sync/syncable/nigori_util.cc @@ -112,7 +112,7 @@ bool VerifyDataTypeEncryptionForTest( while (!to_visit.empty()) { id_string = to_visit.front(); to_visit.pop(); - if (id_string.IsRoot()) + if (id_string.IsNull()) continue; Entry child(trans, GET_BY_ID, id_string); diff --git a/sync/syncable/parent_child_index_unittest.cc b/sync/syncable/parent_child_index_unittest.cc index c0c9e82..256e2dc 100644 --- a/sync/syncable/parent_child_index_unittest.cc +++ b/sync/syncable/parent_child_index_unittest.cc @@ -49,9 +49,8 @@ class ParentChildIndexTest : public testing::Test { root->put(BASE_VERSION, -1); root->put(SERVER_VERSION, 0); root->put(IS_DIR, true); - root->put(ID, syncable::Id()); - root->put(PARENT_ID, syncable::Id()); - root->put(SERVER_PARENT_ID, syncable::Id()); + root->put(ID, syncable::Id::GetRoot()); + root->put(PARENT_ID, syncable::Id::GetRoot()); owned_entry_kernels_.push_back(root); return root; @@ -65,8 +64,7 @@ class ParentChildIndexTest : public testing::Test { folder->put(SERVER_VERSION, 9); folder->put(IS_DIR, true); folder->put(ID, GetBookmarkRootId()); - folder->put(SERVER_PARENT_ID, syncable::Id()); - folder->put(PARENT_ID, syncable::Id()); + folder->put(PARENT_ID, syncable::Id::GetRoot()); folder->put(UNIQUE_SERVER_TAG, "google_chrome_bookmarks"); owned_entry_kernels_.push_back(folder); @@ -82,7 +80,6 @@ class ParentChildIndexTest : public testing::Test { bm->put(IS_DIR, is_dir); bm->put(ID, GetBookmarkId(n)); bm->put(PARENT_ID, GetBookmarkRootId()); - bm->put(SERVER_PARENT_ID, GetBookmarkRootId()); bm->put(UNIQUE_BOOKMARK_TAG, syncable::GenerateSyncableBookmarkHash(kCacheGuid, @@ -104,8 +101,7 @@ class ParentChildIndexTest : public testing::Test { item->put(SERVER_VERSION, 10); item->put(IS_DIR, false); item->put(ID, GetClientUniqueId(n)); - item->put(PARENT_ID, syncable::Id()); - item->put(SERVER_PARENT_ID, syncable::Id()); + item->put(PARENT_ID, syncable::Id::GetRoot()); item->put(UNIQUE_CLIENT_TAG, base::IntToString(n)); owned_entry_kernels_.push_back(item); @@ -302,7 +298,7 @@ TEST_F(ParentChildIndexTest, UnorderedChildren) { index_.Insert(u1); index_.Insert(u2); - const OrderedChildSet* children = index_.GetChildren(syncable::Id()); + const OrderedChildSet* children = index_.GetChildren(syncable::Id::GetRoot()); EXPECT_EQ(children->count(u1), 1UL); EXPECT_EQ(children->count(u2), 1UL); EXPECT_EQ(children->size(), 2UL); diff --git a/sync/syncable/syncable_base_transaction.cc b/sync/syncable/syncable_base_transaction.cc index a1d3e85..8bcaf44 100644 --- a/sync/syncable/syncable_base_transaction.cc +++ b/sync/syncable/syncable_base_transaction.cc @@ -12,7 +12,7 @@ namespace syncable { // static Id BaseTransaction::root_id() { - return Id(); + return Id::GetRoot(); } Directory* BaseTransaction::directory() const { diff --git a/sync/syncable/syncable_id.cc b/sync/syncable/syncable_id.cc index 614fd3e..5cd777f 100644 --- a/sync/syncable/syncable_id.cc +++ b/sync/syncable/syncable_id.cc @@ -6,6 +6,7 @@ #include <iosfwd> +#include "base/logging.h" #include "base/values.h" using std::ostream; @@ -26,6 +27,7 @@ base::StringValue* Id::ToValue() const { string Id::GetServerId() const { // Currently root is the string "0". We need to decide on a true value. // "" would be convenient here, as the IsRoot call would not be needed. + DCHECK(!IsNull()); if (IsRoot()) return "0"; return s_.substr(1); @@ -49,6 +51,12 @@ Id Id::CreateFromClientString(const string& local_id) { return id; } +Id Id::GetRoot() { + Id id; + id.s_ = "r"; + return id; +} + Id Id::GetLexicographicSuccessor() const { // The successor of a string is given by appending the least // character in the alphabet. @@ -64,9 +72,5 @@ Id Id::GetLeastIdForLexicographicComparison() { return id; } -Id GetNullId() { - return Id(); // Currently == root. -} - } // namespace syncable } // namespace syncer diff --git a/sync/syncable/syncable_id.h b/sync/syncable/syncable_id.h index bbfeb93..5b40114 100644 --- a/sync/syncable/syncable_id.h +++ b/sync/syncable/syncable_id.h @@ -43,9 +43,7 @@ SYNC_EXPORT_PRIVATE std::ostream& operator<<(std::ostream& out, const Id& id); // 3. s<server provided opaque id> for items that the server knows about. class SYNC_EXPORT Id { public: - // This constructor will be handy even when we move away from int64s, just - // for unit tests. - inline Id() : s_("r") { } + inline Id() {} inline Id(const Id& that) { Copy(that); } @@ -63,14 +61,8 @@ class SYNC_EXPORT Id { return s_[0] == 's' || s_ == "r"; } - // TODO(sync): We could use null here, but to ease conversion we use "r". - // fix this, this is madness :) - inline bool IsNull() const { - return IsRoot(); - } - inline void Clear() { - s_ = "r"; - } + inline bool IsNull() const { return s_.empty(); } + inline void Clear() { s_.clear(); } inline int compare(const Id& that) const { return s_.compare(that.s_); } @@ -112,6 +104,9 @@ class SYNC_EXPORT Id { // computing lower bounds on std::sets that are ordered by operator<. static Id GetLeastIdForLexicographicComparison(); + // Gets root ID. + static Id GetRoot(); + private: friend scoped_ptr<EntryKernel> UnpackEntry(sql::Statement* statement); friend void BindFields(const EntryKernel& entry, @@ -124,8 +119,6 @@ class SYNC_EXPORT Id { std::string s_; }; -SYNC_EXPORT_PRIVATE Id GetNullId(); - } // namespace syncable } // namespace syncer |