Sync: Remove GetEntry() from BaseNode API.

1) Changed BaseNode::GetEntry() to be protected.
2) Introduced a couple of getters to expose some of the functionality
   that GetEntry() was used for.
3) Made all necessary changes to avoid calling GetEntry() outside of
   BaseNode, ReadNode, WriteNode, and a few specific unit tests.

BUG=123674

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

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

7 files changed, 44 insertions, 18 deletions

diff --git a/sync/internal_api/base_node.cc b/sync/internal_api/base_node.cc
index 9ecbf19..caaf4ee 100644
--- a/sync/internal_api/base_node.cc
+++ b/sync/internal_api/base_node.cc
@@ -48,7 +48,7 @@ BaseNode::BaseNode() : password_data_(new sync_pb::PasswordSpecificsData) {}
 BaseNode::~BaseNode() {}
 
 bool BaseNode::DecryptIfNecessary() {
-  if (!GetEntry()->GetUniqueServerTag().empty())
+  if (GetIsPermanentFolder())
       return true;  // Ignore unique folders.
   const sync_pb::EntitySpecifics& specifics =
       GetEntry()->GetSpecifics();
@@ -124,7 +124,7 @@ const sync_pb::EntitySpecifics& BaseNode::GetUnencryptedSpecifics(
           specifics.bookmark();
       if (bookmark_specifics.has_title() ||
           GetTitle().empty() ||  // For the empty node case
-          !GetEntry()->GetUniqueServerTag().empty()) {
+          GetIsPermanentFolder()) {
         // It's possible we previously had to convert and set
         // |unencrypted_data_| but then wrote our own data, so we allow
         // |unencrypted_data_| to be non-empty.
@@ -157,6 +157,15 @@ bool BaseNode::GetIsFolder() const {
   return GetEntry()->GetIsDir();
 }
 
+bool BaseNode::GetIsPermanentFolder() const {
+  bool is_permanent_folder = !GetEntry()->GetUniqueServerTag().empty();
+  if (is_permanent_folder) {
+    // If the node is a permanent folder it must also have IS_DIR bit set.
+    DCHECK(GetIsFolder());
+  }
+  return is_permanent_folder;
+}
+
 std::string BaseNode::GetTitle() const {
   std::string result;
   // TODO(zea): refactor bookmarks to not need this functionality.
@@ -218,6 +227,10 @@ int64 BaseNode::GetExternalId() const {
   return GetEntry()->GetLocalExternalId();
 }
 
+const syncable::Id& BaseNode::GetSyncId() const {
+  return GetEntry()->GetId();
+}
+
 const sync_pb::BookmarkSpecifics& BaseNode::GetBookmarkSpecifics() const {
   DCHECK_EQ(GetModelType(), BOOKMARKS);
   return GetEntitySpecifics().bookmark();
diff --git a/sync/internal_api/public/base_node.h b/sync/internal_api/public/base_node.h
index 9f29ffd..fe3ca1b 100644
--- a/sync/internal_api/public/base_node.h
+++ b/sync/internal_api/public/base_node.h
@@ -46,6 +46,7 @@ class BaseTransaction;
 namespace syncable {
 class BaseTransaction;
 class Entry;
+class Id;
 }
 
 // A valid BaseNode will never have an ID of zero.
@@ -102,6 +103,10 @@ class SYNC_EXPORT BaseNode {
   // of syncable::Entry.
   bool GetIsFolder() const;
 
+  // Specifies whether node is a permanent folder. This is true when
+  // UNIQUE_SERVER_TAG property of syncable::Entry is non-empty.
+  bool GetIsPermanentFolder() const;
+
   // Returns the title of the object.
   // Uniqueness of the title is not enforced on siblings -- it is not an error
   // for two children to share a title.
@@ -136,6 +141,9 @@ class SYNC_EXPORT BaseNode {
   // Returns the local external ID associated with the node.
   int64 GetExternalId() const;
 
+  // Returns the internal syncable ID associated with the node.
+  const syncable::Id& GetSyncId() const;
+
   // Returns true iff this node has children.
   bool HasChildren() const;
 
@@ -168,10 +176,6 @@ class SYNC_EXPORT BaseNode {
   // Returns this item's attachment ids.
   const syncer::AttachmentIdList GetAttachmentIds() const;
 
-  // These virtual accessors provide access to data members of derived classes.
-  virtual const syncable::Entry* GetEntry() const = 0;
-  virtual const BaseTransaction* GetTransaction() const = 0;
-
   // Returns a base::DictionaryValue serialization of this node.
   base::DictionaryValue* ToValue() const;
 
@@ -179,6 +183,10 @@ class SYNC_EXPORT BaseNode {
   BaseNode();
   virtual ~BaseNode();
 
+  // These virtual accessors provide access to data members of derived classes.
+  virtual const syncable::Entry* GetEntry() const = 0;
+  virtual const BaseTransaction* GetTransaction() const = 0;
+
   // Determines whether part of the entry is encrypted, and if so attempts to
   // decrypt it. Unless decryption is necessary and fails, this will always
   // return |true|. If the contents are encrypted, the decrypted data will be
@@ -200,6 +208,11 @@ class SYNC_EXPORT BaseNode {
   // protected/private BaseNode methods.
   friend class SyncManagerTest;
   FRIEND_TEST_ALL_PREFIXES(SyncApiTest, GenerateSyncableHash);
+  FRIEND_TEST_ALL_PREFIXES(SyncApiTest, WriteEmptyBookmarkTitle);
+  FRIEND_TEST_ALL_PREFIXES(SyncApiTest, WriteEncryptedTitle);
+  FRIEND_TEST_ALL_PREFIXES(SyncBackupManagerTest, NormalizeEntry);
+  FRIEND_TEST_ALL_PREFIXES(SyncBackupManagerTest,
+                           PersistWithSwitchToSyncShutdown);
   FRIEND_TEST_ALL_PREFIXES(SyncManagerTest, UpdateEntryWithEncryption);
   FRIEND_TEST_ALL_PREFIXES(SyncManagerTest,
                            UpdatePasswordSetEntitySpecificsNoChange);
diff --git a/sync/internal_api/sync_encryption_handler_impl.cc b/sync/internal_api/sync_encryption_handler_impl.cc
index b2c4b8b..3ea242c 100644
--- a/sync/internal_api/sync_encryption_handler_impl.cc
+++ b/sync/internal_api/sync_encryption_handler_impl.cc
@@ -848,7 +848,7 @@ void SyncEncryptionHandlerImpl::ReEncryptEverything(
       if (child.GetIsFolder()) {
         to_visit.push(child.GetFirstChildId());
       }
-      if (child.GetEntry()->GetUniqueServerTag().empty()) {
+      if (!child.GetIsPermanentFolder()) {
       // Rewrite the specifics of the node with encrypted data if necessary
       // (only rewrite the non-unique folders).
         child.ResetFromSpecifics();
diff --git a/sync/internal_api/sync_manager_impl_unittest.cc b/sync/internal_api/sync_manager_impl_unittest.cc
index 0d51577..67aba25 100644
--- a/sync/internal_api/sync_manager_impl_unittest.cc
+++ b/sync/internal_api/sync_manager_impl_unittest.cc
@@ -597,7 +597,7 @@ TEST_F(SyncApiTest, WriteEmptyBookmarkTitle) {
     ReadNode bookmark_node(&trans);
     ASSERT_EQ(BaseNode::INIT_OK, bookmark_node.InitByIdLookup(bookmark_id));
     EXPECT_EQ("", bookmark_node.GetTitle());
-    EXPECT_EQ(" ", bookmark_node.GetEntry()->GetSpecifics().bookmark().title());
+    EXPECT_EQ(" ", bookmark_node.GetEntitySpecifics().bookmark().title());
     EXPECT_EQ(" ", bookmark_node.GetEntry()->GetNonUniqueName());
   }
 }
diff --git a/sync/internal_api/sync_rollback_manager_unittest.cc b/sync/internal_api/sync_rollback_manager_unittest.cc
index 4a90a316..94ce3b1 100644
--- a/sync/internal_api/sync_rollback_manager_unittest.cc
+++ b/sync/internal_api/sync_rollback_manager_unittest.cc
@@ -97,7 +97,7 @@ class SyncRollbackManagerTest : public testing::Test,
     WriteNode node(&trans);
     EXPECT_EQ(WriteNode::INIT_SUCCESS,
               node.InitUniqueByCreation(type, client_tag));
-    return node.GetEntry()->GetMetahandle();
+    return node.GetId();
   }
 
   void InitManager(SyncManager* manager, ModelTypeSet types,
diff --git a/sync/internal_api/write_node.cc b/sync/internal_api/write_node.cc
index dd358c4..7a065a2 100644
--- a/sync/internal_api/write_node.cc
+++ b/sync/internal_api/write_node.cc
@@ -127,7 +127,7 @@ void WriteNode::SetPasswordSpecifics(
   // We have to do the idempotency check here (vs in UpdateEntryWithEncryption)
   // because Passwords have their encrypted data within the PasswordSpecifics,
   // vs within the EntitySpecifics like all the other types.
-  const sync_pb::EntitySpecifics& old_specifics = GetEntry()->GetSpecifics();
+  const sync_pb::EntitySpecifics& old_specifics = GetEntitySpecifics();
   sync_pb::EntitySpecifics entity_specifics;
   // Copy over the old specifics if they exist.
   if (GetModelTypeFromSpecifics(old_specifics) == PASSWORDS) {
@@ -268,7 +268,7 @@ bool WriteNode::InitBookmarkByCreation(const BaseNode& parent,
     return false;
   }
 
-  syncable::Id parent_id = parent.GetEntry()->GetId();
+  syncable::Id parent_id = parent.GetSyncId();
   DCHECK(!parent_id.IsNull());
 
   // Start out with a dummy name.  We expect
@@ -298,7 +298,7 @@ WriteNode::InitUniqueByCreationResult WriteNode::InitUniqueByCreation(
     ModelType model_type,
     const BaseNode& parent,
     const std::string& tag) {
-  return InitUniqueByCreationImpl(model_type, parent.GetEntry()->GetId(), tag);
+  return InitUniqueByCreationImpl(model_type, parent.GetSyncId(), tag);
 }
 
 WriteNode::InitUniqueByCreationResult WriteNode::InitUniqueByCreation(
@@ -406,14 +406,14 @@ bool WriteNode::SetPosition(const BaseNode& new_parent,
     return false;
   }
 
-  syncable::Id new_parent_id = new_parent.GetEntry()->GetId();
+  syncable::Id new_parent_id = new_parent.GetSyncId();
   DCHECK(!new_parent_id.IsNull());
 
   // 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.IsNull()) ||
-        (predecessor && (old == predecessor->GetEntry()->GetId()))) {
+        (predecessor && (old == predecessor->GetSyncId()))) {
       return true;
     }
   }
@@ -462,8 +462,8 @@ void WriteNode::Drop() {
 
 bool WriteNode::PutPredecessor(const BaseNode* predecessor) {
   DCHECK(!entry_->GetParentId().IsNull());
-  syncable::Id predecessor_id = predecessor ?
-      predecessor->GetEntry()->GetId() : syncable::Id();
+  syncable::Id predecessor_id =
+      predecessor ? predecessor->GetSyncId() : syncable::Id();
   return entry_->PutPredecessor(predecessor_id);
 }
 
diff --git a/sync/syncable/entry.h b/sync/syncable/entry.h
index 0c30ccd..47a7815 100644
--- a/sync/syncable/entry.h
+++ b/sync/syncable/entry.h
@@ -110,12 +110,12 @@ class SYNC_EXPORT Entry {
     return kernel_->ref(SERVER_CTIME);
   }
 
-  Id GetId() const {
+  const Id& GetId() const {
     DCHECK(kernel_);
     return kernel_->ref(ID);
   }
 
-  Id GetParentId() const {
+  const Id& GetParentId() const {
     DCHECK(kernel_);
     return kernel_->ref(PARENT_ID);
   }