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}

22 files changed, 86 insertions, 86 deletions

diff --git a/sync/engine/commit_util.cc b/sync/engine/commit_util.cc
index c913edd..a408703 100644
--- a/sync/engine/commit_util.cc
+++ b/sync/engine/commit_util.cc
@@ -150,11 +150,11 @@ void BuildCommitItem(
   // TODO(nick): With the server keeping track of the primary sync parent,
   // it should not be necessary to provide the old_parent_id: the version
   // number should suffice.
-  if (new_parent_id != meta_entry.GetServerParentId() &&
+  Id server_parent_id = meta_entry.GetServerParentId();
+  if (new_parent_id != server_parent_id && !server_parent_id.IsNull() &&
       0 != meta_entry.GetBaseVersion() &&
       syncable::CHANGES_VERSION != meta_entry.GetBaseVersion()) {
-    sync_entry->set_old_parent_id(
-        SyncableIdToProto(meta_entry.GetServerParentId()));
+    sync_entry->set_old_parent_id(SyncableIdToProto(server_parent_id));
   }
 
   int64 version = meta_entry.GetBaseVersion();
@@ -184,7 +184,7 @@ void BuildCommitItem(
       // for legacy reasons.  See comments in sync.proto for more information.
       const Id& prev_id = meta_entry.GetPredecessorId();
       string prev_id_string =
-          prev_id.IsRoot() ? string() : prev_id.GetServerId();
+          prev_id.IsNull() ? string() : prev_id.GetServerId();
       sync_entry->set_insert_after_item_id(prev_id_string);
       sync_entry->set_position_in_parent(
           meta_entry.GetUniquePosition().ToInt64());
diff --git a/sync/engine/directory_update_handler_unittest.cc b/sync/engine/directory_update_handler_unittest.cc
index 8b6e6ff..10eead9 100644
--- a/sync/engine/directory_update_handler_unittest.cc
+++ b/sync/engine/directory_update_handler_unittest.cc
@@ -29,6 +29,7 @@
 
 namespace syncer {
 
+using syncable::Id;
 using syncable::UNITTEST;
 
 static const int64 kDefaultVersion = 1000;
@@ -79,7 +80,7 @@ class DirectoryUpdateHandlerProcessUpdateTest : public ::testing::Test {
   bool EntryExists(const std::string& id) {
     syncable::ReadTransaction trans(FROM_HERE, dir());
     syncable::Entry e(&trans, syncable::GET_BY_ID,
-                      syncable::Id::CreateFromServerId(id));
+                      Id::CreateFromServerId(id));
     return e.good() && !e.GetIsDel();
   }
 
@@ -132,13 +133,13 @@ TEST_F(DirectoryUpdateHandlerProcessUpdateTest, NewBookmarkTag) {
   sessions::StatusController status;
 
   // Add a bookmark item to the update message.
-  std::string root = syncable::GetNullId().GetServerId();
-  syncable::Id server_id = syncable::Id::CreateFromServerId("b1");
+  std::string root = Id::GetRoot().GetServerId();
+  Id server_id = Id::CreateFromServerId("b1");
   scoped_ptr<sync_pb::SyncEntity> e =
       CreateUpdate(SyncableIdToProto(server_id), root, BOOKMARKS);
   e->set_originator_cache_guid(
       std::string(kCacheGuid, arraysize(kCacheGuid)-1));
-  syncable::Id client_id = syncable::Id::CreateFromClientString("-2");
+  Id client_id = Id::CreateFromClientString("-2");
   e->set_originator_client_item_id(client_id.GetServerId());
   e->set_position_in_parent(0);
 
@@ -171,8 +172,8 @@ TEST_F(DirectoryUpdateHandlerProcessUpdateTest,
   sessions::StatusController status;
 
   // Create an update that mimics the bookmark root.
-  syncable::Id server_id = syncable::Id::CreateFromServerId("xyz");
-  std::string root = syncable::GetNullId().GetServerId();
+  Id server_id = Id::CreateFromServerId("xyz");
+  std::string root = Id::GetRoot().GetServerId();
   scoped_ptr<sync_pb::SyncEntity> e =
       CreateUpdate(SyncableIdToProto(server_id), root, BOOKMARKS);
   e->set_server_defined_unique_tag("google_chrome_bookmarks");
@@ -205,8 +206,8 @@ TEST_F(DirectoryUpdateHandlerProcessUpdateTest, ReceiveNonBookmarkItem) {
   sync_pb::GetUpdatesResponse gu_response;
   sessions::StatusController status;
 
-  std::string root = syncable::GetNullId().GetServerId();
-  syncable::Id server_id = syncable::Id::CreateFromServerId("xyz");
+  std::string root = Id::GetRoot().GetServerId();
+  Id server_id = Id::CreateFromServerId("xyz");
   scoped_ptr<sync_pb::SyncEntity> e =
       CreateUpdate(SyncableIdToProto(server_id), root, AUTOFILL);
   e->set_server_defined_unique_tag("9PGRuKdX5sHyGMB17CvYTXuC43I=");
@@ -267,20 +268,19 @@ TEST_F(DirectoryUpdateHandlerProcessUpdateTest, GarbageCollectionByVersion) {
   context.set_version(1);
 
   scoped_ptr<sync_pb::SyncEntity> type_root =
-      CreateUpdate(SyncableIdToProto(syncable::Id::CreateFromServerId("root")),
-                   syncable::GetNullId().GetServerId(),
-                   SYNCED_NOTIFICATIONS);
+      CreateUpdate(SyncableIdToProto(Id::CreateFromServerId("root")),
+                   Id::GetRoot().GetServerId(), SYNCED_NOTIFICATIONS);
   type_root->set_server_defined_unique_tag(
       ModelTypeToRootTag(SYNCED_NOTIFICATIONS));
   type_root->set_folder(true);
 
   scoped_ptr<sync_pb::SyncEntity> e1 =
-      CreateUpdate(SyncableIdToProto(syncable::Id::CreateFromServerId("e1")),
+      CreateUpdate(SyncableIdToProto(Id::CreateFromServerId("e1")),
                    type_root->id_string(),
                    SYNCED_NOTIFICATIONS);
 
   scoped_ptr<sync_pb::SyncEntity> e2 =
-      CreateUpdate(SyncableIdToProto(syncable::Id::CreateFromServerId("e2")),
+      CreateUpdate(SyncableIdToProto(Id::CreateFromServerId("e2")),
                    type_root->id_string(),
                    SYNCED_NOTIFICATIONS);
   e2->set_version(kDefaultVersion + 100);
@@ -331,14 +331,13 @@ TEST_F(DirectoryUpdateHandlerProcessUpdateTest, ContextVersion) {
   old_context.set_data_type_id(field_number);
 
   scoped_ptr<sync_pb::SyncEntity> type_root =
-      CreateUpdate(SyncableIdToProto(syncable::Id::CreateFromServerId("root")),
-                   syncable::GetNullId().GetServerId(),
-                   SYNCED_NOTIFICATIONS);
+      CreateUpdate(SyncableIdToProto(Id::CreateFromServerId("root")),
+                   Id::GetRoot().GetServerId(), SYNCED_NOTIFICATIONS);
   type_root->set_server_defined_unique_tag(
       ModelTypeToRootTag(SYNCED_NOTIFICATIONS));
   type_root->set_folder(true);
   scoped_ptr<sync_pb::SyncEntity> e1 =
-      CreateUpdate(SyncableIdToProto(syncable::Id::CreateFromServerId("e1")),
+      CreateUpdate(SyncableIdToProto(Id::CreateFromServerId("e1")),
                    type_root->id_string(),
                    SYNCED_NOTIFICATIONS);
 
@@ -369,7 +368,7 @@ TEST_F(DirectoryUpdateHandlerProcessUpdateTest, ContextVersion) {
   new_context.set_data_type_id(field_number);
 
   scoped_ptr<sync_pb::SyncEntity> e2 =
-      CreateUpdate(SyncableIdToProto(syncable::Id::CreateFromServerId("e2")),
+      CreateUpdate(SyncableIdToProto(Id::CreateFromServerId("e2")),
                    type_root->id_string(),
                    SYNCED_NOTIFICATIONS);
   updates.clear();
@@ -412,14 +411,13 @@ TEST_F(DirectoryUpdateHandlerProcessUpdateTest,
   context.set_version(1);
 
   scoped_ptr<sync_pb::SyncEntity> type_root =
-      CreateUpdate(SyncableIdToProto(syncable::Id::CreateFromServerId("root")),
-                   syncable::GetNullId().GetServerId(),
-                   ARTICLES);
+      CreateUpdate(SyncableIdToProto(Id::CreateFromServerId("root")),
+                   Id::GetRoot().GetServerId(), ARTICLES);
   type_root->set_server_defined_unique_tag(ModelTypeToRootTag(ARTICLES));
   type_root->set_folder(true);
 
   scoped_ptr<sync_pb::SyncEntity> e1 =
-      CreateUpdate(SyncableIdToProto(syncable::Id::CreateFromServerId("e1")),
+      CreateUpdate(SyncableIdToProto(Id::CreateFromServerId("e1")),
                    type_root->id_string(),
                    ARTICLES);
   sync_pb::AttachmentIdProto* attachment_id = e1->add_attachment_id();
@@ -441,7 +439,7 @@ TEST_F(DirectoryUpdateHandlerProcessUpdateTest,
     syncable::ReadTransaction trans(FROM_HERE, dir());
     syncable::Entry e(&trans,
                       syncable::GET_BY_ID,
-                      syncable::Id::CreateFromServerId(e1->id_string()));
+                      Id::CreateFromServerId(e1->id_string()));
 
     // See that the attachment_metadata is correct.
     sync_pb::AttachmentMetadata attachment_metadata = e.GetAttachmentMetadata();
@@ -569,7 +567,7 @@ sync_pb::EntitySpecifics DefaultBookmarkSpecifics() {
 TEST_F(DirectoryUpdateHandlerApplyUpdateTest, SimpleBookmark) {
   sessions::StatusController status;
 
-  std::string root_server_id = syncable::GetNullId().GetServerId();
+  std::string root_server_id = Id::GetRoot().GetServerId();
   int64 parent_handle =
       entry_factory()->CreateUnappliedNewBookmarkItemWithParent(
           "parent", DefaultBookmarkSpecifics(), root_server_id);
@@ -607,7 +605,7 @@ TEST_F(DirectoryUpdateHandlerApplyUpdateTest, SimpleBookmark) {
 TEST_F(DirectoryUpdateHandlerApplyUpdateTest,
        BookmarkChildrenBeforeParent) {
   // Start with some bookmarks whose parents are unknown.
-  std::string root_server_id = syncable::GetNullId().GetServerId();
+  std::string root_server_id = Id::GetRoot().GetServerId();
   int64 a_handle = entry_factory()->CreateUnappliedNewBookmarkItemWithParent(
       "a_child_created_first", DefaultBookmarkSpecifics(), "parent");
   int64 x_handle = entry_factory()->CreateUnappliedNewBookmarkItemWithParent(
@@ -784,7 +782,7 @@ TEST_F(DirectoryUpdateHandlerApplyUpdateTest,
   // Create a locally deleted parent item.
   int64 parent_handle;
   entry_factory()->CreateUnsyncedItem(
-      syncable::Id::CreateFromServerId("parent"), TestIdFactory::root(),
+      Id::CreateFromServerId("parent"), TestIdFactory::root(),
       "parent", true, BOOKMARKS, &parent_handle);
   {
     syncable::WriteTransaction trans(FROM_HERE, UNITTEST, directory());
@@ -897,7 +895,7 @@ TEST_F(DirectoryUpdateHandlerApplyUpdateTest,
 // fail due to hierarchy conflicts.  Others should succeed.
 TEST_F(DirectoryUpdateHandlerApplyUpdateTest, ItemsBothKnownAndUnknown) {
   // See what happens when there's a mixture of good and bad updates.
-  std::string root_server_id = syncable::GetNullId().GetServerId();
+  std::string root_server_id = Id::GetRoot().GetServerId();
   int64 u1_handle = entry_factory()->CreateUnappliedNewItemWithParent(
       "first_unknown_item", DefaultBookmarkSpecifics(), "unknown_parent");
   int64 k1_handle = entry_factory()->CreateUnappliedNewItemWithParent(
@@ -988,7 +986,7 @@ TEST_F(DirectoryUpdateHandlerApplyUpdateTest, UndecryptableData) {
   sync_pb::EntitySpecifics encrypted_bookmark;
   encrypted_bookmark.mutable_encrypted();
   AddDefaultFieldValue(BOOKMARKS, &encrypted_bookmark);
-  std::string root_server_id = syncable::GetNullId().GetServerId();
+  std::string root_server_id = Id::GetRoot().GetServerId();
   int64 folder_handle = entry_factory()->CreateUnappliedNewItemWithParent(
       "folder",
       encrypted_bookmark,
diff --git a/sync/engine/get_commit_ids.cc b/sync/engine/get_commit_ids.cc
index c031e9f..e02cce5 100644
--- a/sync/engine/get_commit_ids.cc
+++ b/sync/engine/get_commit_ids.cc
@@ -349,7 +349,7 @@ void Traversal::AddItemThenPredecessors(
     return;  // Deleted items have no predecessors.
 
   syncable::Id prev_id = item.GetPredecessorId();
-  while (!prev_id.IsRoot()) {
+  while (!prev_id.IsNull()) {
     syncable::Entry prev(trans_, syncable::GET_BY_ID, prev_id);
     CHECK(prev.good()) << "Bad id when walking predecessors.";
     if (!prev.GetIsUnsynced()) {
diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc
index 965728b..17e5f09 100644
--- a/sync/engine/syncer_unittest.cc
+++ b/sync/engine/syncer_unittest.cc
@@ -4679,7 +4679,7 @@ TEST_F(SyncerBookmarksTest, LocalDeleteRemoteChangeConflict) {
 
   // Trigger a getupdates that modifies the bookmark. The update should  be
   // clobbered by the local delete.
-  mock_server_->AddUpdateBookmark(GetServerId(), Id(), "dummy", 10, 10,
+  mock_server_->AddUpdateBookmark(GetServerId(), Id::GetRoot(), "dummy", 10, 10,
                                   local_cache_guid(), local_id_.GetServerId());
 
   SyncShareNudge();
diff --git a/sync/engine/syncer_util.cc b/sync/engine/syncer_util.cc
index 3cb9bca..7f63b37 100644
--- a/sync/engine/syncer_util.cc
+++ b/sync/engine/syncer_util.cc
@@ -121,7 +121,7 @@ syncable::Id FindLocalIdToUpdate(
             // we don't server delete the item, because we don't allow it to
             // exist locally at all.  So the item will remain orphaned on
             // the server, and we won't pay attention to it.
-            return syncable::GetNullId();
+            return syncable::Id();
           }
         }
         // Target this change to the existing local entry; later,
diff --git a/sync/engine/syncer_util_unittest.cc b/sync/engine/syncer_util_unittest.cc
index 7580755..9fe3d4b 100644
--- a/sync/engine/syncer_util_unittest.cc
+++ b/sync/engine/syncer_util_unittest.cc
@@ -161,7 +161,7 @@ sync_pb::EntitySpecifics DefaultBookmarkSpecifics() {
 TEST_F(GetUpdatePositionTest, UpdateServerFieldsFromUpdateTest) {
   InitSuffixIngredients();  // Initialize update with valid data.
 
-  std::string root_server_id = syncable::GetNullId().GetServerId();
+  std::string root_server_id = syncable::Id::GetRoot().GetServerId();
   int64 handle = entry_factory()->CreateUnappliedNewBookmarkItemWithParent(
       "I", DefaultBookmarkSpecifics(), root_server_id);
 
@@ -184,7 +184,7 @@ TEST_F(GetUpdatePositionTest, UpdateServerFieldsFromUpdateTest) {
 TEST_F(GetUpdatePositionTest, UpdateServerFieldsFromInvalidUpdateTest) {
   // Do not initialize data in update, update is invalid.
 
-  std::string root_server_id = syncable::GetNullId().GetServerId();
+  std::string root_server_id = syncable::Id::GetRoot().GetServerId();
   int64 handle = entry_factory()->CreateUnappliedNewBookmarkItemWithParent(
       "I", DefaultBookmarkSpecifics(), root_server_id);
 
diff --git a/sync/internal_api/base_node.cc b/sync/internal_api/base_node.cc
index 57dbb22..b43c6ca 100644
--- a/sync/internal_api/base_node.cc
+++ b/sync/internal_api/base_node.cc
@@ -177,21 +177,21 @@ bool BaseNode::HasChildren() const {
 
 int64 BaseNode::GetPredecessorId() const {
   syncable::Id id_string = GetEntry()->GetPredecessorId();
-  if (id_string.IsRoot())
+  if (id_string.IsNull())
     return kInvalidId;
   return IdToMetahandle(GetTransaction()->GetWrappedTrans(), id_string);
 }
 
 int64 BaseNode::GetSuccessorId() const {
   syncable::Id id_string = GetEntry()->GetSuccessorId();
-  if (id_string.IsRoot())
+  if (id_string.IsNull())
     return kInvalidId;
   return IdToMetahandle(GetTransaction()->GetWrappedTrans(), id_string);
 }
 
 int64 BaseNode::GetFirstChildId() const {
   syncable::Id id_string = GetEntry()->GetFirstChildId();
-  if (id_string.IsRoot())
+  if (id_string.IsNull())
     return kInvalidId;
   return IdToMetahandle(GetTransaction()->GetWrappedTrans(), id_string);
 }
diff --git a/sync/internal_api/sync_manager_impl_unittest.cc b/sync/internal_api/sync_manager_impl_unittest.cc
index ac5b31a..e6a1fa9 100644
--- a/sync/internal_api/sync_manager_impl_unittest.cc
+++ b/sync/internal_api/sync_manager_impl_unittest.cc
@@ -155,7 +155,10 @@ int64 MakeServerNodeForType(UserShare* share,
   entry.PutBaseVersion(1);
   entry.PutServerVersion(1);
   entry.PutIsUnappliedUpdate(false);
-  entry.PutServerParentId(syncable::GetNullId());
+  // TODO(stanisc): setting parent ID might be unnecessary once
+  // empty parent ID is supported.
+  entry.PutParentId(syncable::Id::GetRoot());
+  entry.PutServerParentId(syncable::Id::GetRoot());
   entry.PutServerIsDir(true);
   entry.PutIsDir(true);
   entry.PutServerSpecifics(specifics);
diff --git a/sync/internal_api/sync_rollback_manager_base.cc b/sync/internal_api/sync_rollback_manager_base.cc
index baedec0..587ea56 100644
--- a/sync/internal_api/sync_rollback_manager_base.cc
+++ b/sync/internal_api/sync_rollback_manager_base.cc
@@ -270,7 +270,7 @@ bool SyncRollbackManagerBase::InitTypeRootNode(ModelType type) {
   if (!entry.good())
     return false;
 
-  entry.PutParentId(syncable::Id());
+  entry.PutParentId(syncable::Id::GetRoot());
   entry.PutBaseVersion(1);
   entry.PutUniqueServerTag(ModelTypeToRootTag(type));
   entry.PutNonUniqueName(ModelTypeToString(type));
diff --git a/sync/internal_api/test/test_entry_factory.cc b/sync/internal_api/test/test_entry_factory.cc
index d2e6b73..8941040 100644
--- a/sync/internal_api/test/test_entry_factory.cc
+++ b/sync/internal_api/test/test_entry_factory.cc
@@ -76,7 +76,7 @@ int64 TestEntryFactory::CreateUnappliedNewItem(
   entry.PutServerVersion(GetNextRevision());
   entry.PutIsUnappliedUpdate(true);
   entry.PutServerNonUniqueName(item_id);
-  entry.PutServerParentId(syncable::GetNullId());
+  entry.PutServerParentId(syncable::Id::GetRoot());
   entry.PutServerIsDir(is_unique);
   entry.PutServerSpecifics(specifics);
   if (is_unique) { // For top-level nodes.
diff --git a/sync/internal_api/write_node.cc b/sync/internal_api/write_node.cc
index b1f0872..4aab598 100644
--- a/sync/internal_api/write_node.cc
+++ b/sync/internal_api/write_node.cc
@@ -461,7 +461,7 @@ bool WriteNode::SetPosition(const BaseNode& new_parent,
   // Filter out redundant changes if both the parent and the predecessor match.
   if (new_parent_id == entry_->GetParentId()) {
     const syncable::Id& old = entry_->GetPredecessorId();
-    if ((!predecessor && old.IsRoot()) ||
+    if ((!predecessor && old.IsNull()) ||
         (predecessor && (old == predecessor->GetEntry()->GetId()))) {
       return true;
     }
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
 
diff --git a/sync/test/engine/test_id_factory.h b/sync/test/engine/test_id_factory.h
index 85781a7..3334729 100644
--- a/sync/test/engine/test_id_factory.h
+++ b/sync/test/engine/test_id_factory.h
@@ -20,9 +20,7 @@ class TestIdFactory {
   ~TestIdFactory() {}
 
   // Get the root ID.
-  static syncable::Id root() {
-    return syncable::Id();
-  }
+  static syncable::Id root() { return syncable::Id::GetRoot(); }
 
   // Make an ID from a number.  If the number is zero, return the root ID.
   // If the number is positive, create a server ID based on the value.  If