Sync: Handle empty (implicit) ParentId in GetPredecessorId and GetSuccessorId

GetSuccessorId and GetPredecessorId access all children of the
current node's parent in order to get to the next/previous node.
With implicit parent ID these functions can't get to the parent.
The reasons we didn't run into this problem before are
a) The server feature that will stop setting parent ID isn't
   enabled yet so there wasn't a chance to expose this yet.
b) There wasn't enough test coverage.

The fix is to use the ParentChildIndex to resolve the implicit
(unset) parent ID. Added a new method called GetSiblings at
the Directory level that does just that. Also added some
unit test coverage.

BUG=438313

Review URL: https://codereview.chromium.org/1008993004

Cr-Commit-Position: refs/heads/master@{#320922}

sync/syncable/directory.cc

@@ -338,7 +338,7 @@ int Directory::GetPositionIndex(
     BaseTransaction* trans,
     EntryKernel* kernel) const {
   const OrderedChildSet* siblings =
-      kernel_->parent_child_index.GetChildren(kernel->ref(PARENT_ID));
+      kernel_->parent_child_index.GetSiblings(kernel);
 
   OrderedChildSet::const_iterator it = siblings->find(kernel);
   return std::distance(siblings->begin(), it);
@@ -1364,13 +1364,11 @@ syncable::Id Directory::GetPredecessorId(EntryKernel* e) {
   ScopedKernelLock lock(this);
 
   DCHECK(ParentChildIndex::ShouldInclude(e));
-  const OrderedChildSet* children =
-      kernel_->parent_child_index.GetChildren(e->ref(PARENT_ID));
-  DCHECK(children && !children->empty());
-  OrderedChildSet::const_iterator i = children->find(e);
-  DCHECK(i != children->end());
+  const OrderedChildSet* siblings = kernel_->parent_child_index.GetSiblings(e);
+  OrderedChildSet::const_iterator i = siblings->find(e);
+  DCHECK(i != siblings->end());
 
-  if (i == children->begin()) {
+  if (i == siblings->begin()) {
     return Id();
   } else {
     i--;
@@ -1382,14 +1380,12 @@ syncable::Id Directory::GetSuccessorId(EntryKernel* e) {
   ScopedKernelLock lock(this);
 
   DCHECK(ParentChildIndex::ShouldInclude(e));
-  const OrderedChildSet* children =
-      kernel_->parent_child_index.GetChildren(e->ref(PARENT_ID));
-  DCHECK(children && !children->empty());
-  OrderedChildSet::const_iterator i = children->find(e);
-  DCHECK(i != children->end());
+  const OrderedChildSet* siblings = kernel_->parent_child_index.GetSiblings(e);
+  OrderedChildSet::const_iterator i = siblings->find(e);
+  DCHECK(i != siblings->end());
 
   i++;
-  if (i == children->end()) {
+  if (i == siblings->end()) {
     return Id();
   } else {
     return (*i)->ref(ID);

sync/syncable/directory_unittest.cc

@@ -1961,6 +1961,51 @@ TEST_F(SyncableDirectoryTest, MutableEntry_ImplicitParentId) {
   }
 }
 
+// Verify that the successor / predecessor navigation still works for
+// directory entries with unset Parent IDs.
+TEST_F(SyncableDirectoryTest, MutableEntry_ImplicitParentId_Siblings) {
+  TestIdFactory id_factory;
+  const Id root_id = TestIdFactory::root();
+  const Id p_root_id = id_factory.NewServerId();
+  const Id item1_id = id_factory.FromNumber(1);
+  const Id item2_id = id_factory.FromNumber(2);
+
+  // Create type root folder for PREFERENCES.
+  {
+    WriteTransaction trans(FROM_HERE, UNITTEST, dir().get());
+    MutableEntry p_root(&trans, CREATE, PREFERENCES, root_id, "P");
+    ASSERT_TRUE(p_root.good());
+    p_root.PutIsDir(true);
+    p_root.PutId(p_root_id);
+    p_root.PutBaseVersion(1);
+  }
+
+  // Create two PREFERENCES entries with implicit parent nodes.
+  {
+    WriteTransaction trans(FROM_HERE, UNITTEST, dir().get());
+    MutableEntry item1(&trans, CREATE, PREFERENCES, "P1");
+    item1.PutBaseVersion(1);
+    item1.PutId(item1_id);
+    MutableEntry item2(&trans, CREATE, PREFERENCES, "P2");
+    item2.PutBaseVersion(1);
+    item2.PutId(item2_id);
+  }
+
+  // Verify GetSuccessorId and GetPredecessorId calls for these items.
+  // Please note that items are sorted according to their ID, e.g.
+  // item1 first, then item2.
+  {
+    ReadTransaction trans(FROM_HERE, dir().get());
+    Entry item1(&trans, GET_BY_ID, item1_id);
+    EXPECT_EQ(Id(), item1.GetPredecessorId());
+    EXPECT_EQ(item2_id, item1.GetSuccessorId());
+
+    Entry item2(&trans, GET_BY_ID, item2_id);
+    EXPECT_EQ(item1_id, item2.GetPredecessorId());
+    EXPECT_EQ(Id(), item2.GetSuccessorId());
+  }
+}
+
 }  // namespace syncable
 
 }  // namespace syncer

sync/syncable/parent_child_index.cc

@@ -120,8 +120,10 @@ bool ParentChildIndex::Contains(EntryKernel *e) const {
   return children->count(e) > 0;
 }
 
-const OrderedChildSet* ParentChildIndex::GetChildren(const syncable::Id& id) {
-  ParentChildrenMap::iterator parent = parent_children_map_.find(id);
+const OrderedChildSet* ParentChildIndex::GetChildren(const Id& id) const {
+  DCHECK(!id.IsNull());
+
+  ParentChildrenMap::const_iterator parent = parent_children_map_.find(id);
   if (parent == parent_children_map_.end()) {
     return NULL;
   }
@@ -131,6 +133,17 @@ const OrderedChildSet* ParentChildIndex::GetChildren(const syncable::Id& id) {
   return parent->second;
 }
 
+const OrderedChildSet* ParentChildIndex::GetChildren(EntryKernel* e) const {
+  return GetChildren(e->ref(ID));
+}
+
+const OrderedChildSet* ParentChildIndex::GetSiblings(EntryKernel* e) const {
+  DCHECK(Contains(e));
+  const OrderedChildSet* siblings = GetChildren(GetParentId(e));
+  DCHECK(siblings && !siblings->empty());
+  return siblings;
+}
+
 const Id& ParentChildIndex::GetParentId(EntryKernel* e) const {
   const Id& parent_id = e->ref(PARENT_ID);
   if (!parent_id.IsNull()) {

sync/syncable/parent_child_index.h

@@ -49,7 +49,14 @@ class SYNC_EXPORT_PRIVATE ParentChildIndex {
 
   // Returns all children of the entry with the given Id.  Returns NULL if the
   // node has no children or the Id does not identify a valid directory node.
-  const OrderedChildSet* GetChildren(const Id& id);
+  const OrderedChildSet* GetChildren(const Id& id) const;
+
+  // Returns all children of the entry.  Returns NULL if the node has no
+  // children.
+  const OrderedChildSet* GetChildren(EntryKernel* e) const;
+
+  // Returns all siblings of the entry.
+  const OrderedChildSet* GetSiblings(EntryKernel* e) const;
 
  private:
   friend class ParentChildIndexTest;

sync/syncable/parent_child_index_unittest.cc

@@ -389,10 +389,13 @@ TEST_F(ParentChildIndexTest, NodesWithImplicitParentId) {
   EXPECT_TRUE(index_.Contains(p2));
 
   // Items should appear under the type root
-  const OrderedChildSet* children = index_.GetChildren(type_root_id);
+  const OrderedChildSet* children = index_.GetChildren(type_root);
   ASSERT_TRUE(children);
   EXPECT_EQ(2UL, children->size());
 
+  EXPECT_EQ(2UL, index_.GetSiblings(p1)->size());
+  EXPECT_EQ(2UL, index_.GetSiblings(p2)->size());
+
   index_.Remove(p1);
 
   EXPECT_FALSE(index_.Contains(p1));
@@ -404,7 +407,7 @@ TEST_F(ParentChildIndexTest, NodesWithImplicitParentId) {
   index_.Remove(p2);
 
   EXPECT_FALSE(index_.Contains(p2));
-  children = index_.GetChildren(type_root_id);
+  children = index_.GetChildren(type_root);
   ASSERT_EQ(nullptr, children);
 
   index_.Remove(type_root);