sync: Add getters and setters to Entry classes

Replace the Entry and MutableEntry classes' Get(X) and Put(X, V) members
with GetX() and PutX(V) style members.

There are many good reasons for performing this refactor. The main
reason to implement it now is that we'd like to have more fine-grained
control over the visibility of the getters and setters for certain
fields, and it's really hard to implement that control when several
different fields use the same accessor functions.

Once this refactor is complete, we can modify the inheritance hierarchy
for Entry and MutableEntry to introduce a new, semi-MutableEntry that
provides only the setters whose changes are not "visible" to the
underlying model.

We can also build on this work to simplify the implementation of
MutableEntry.

The interface changes are the most notable part of this CL, but there
are a few smaller changes here too, including:
- Some small formatting cleanups.
- In cases where Put() functions had signatures that returned bool but
never returned anything other than 'true', those return values have
been changed to void.
- Some dead code in syncer_proto_util.cc has been removed.

BUG=284672

Review URL: https://chromiumcodereview.appspot.com/23484035

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@223111 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/sync/glue/bookmark_change_processor.cc

@@ -303,15 +303,15 @@ void BookmarkChangeProcessor::BookmarkNodeChanged(BookmarkModel* model,
         error_handler()->OnSingleDatatypeUnrecoverableError(FROM_HERE,
             "Could not InitByIdLookup on BookmarkNodeChanged, good() failed");
         LOG(ERROR) << "Bad entry.";
-      } else if (sync_node.GetEntry()->Get(syncer::syncable::IS_DEL)) {
+      } else if (sync_node.GetEntry()->GetIsDel()) {
         error_handler()->OnSingleDatatypeUnrecoverableError(FROM_HERE,
             "Could not InitByIdLookup on BookmarkNodeChanged, is_del true");
         LOG(ERROR) << "Deleted entry.";
       } else {
         syncer::Cryptographer* crypto = trans.GetCryptographer();
         syncer::ModelTypeSet encrypted_types(trans.GetEncryptedTypes());
         const sync_pb::EntitySpecifics& specifics =
-            sync_node.GetEntry()->Get(syncer::syncable::SPECIFICS);
+            sync_node.GetEntry()->GetSpecifics();
         CHECK(specifics.has_encrypted());
         const bool can_decrypt = crypto->CanDecrypt(specifics.encrypted());
         const bool agreement = encrypted_types.Has(syncer::BOOKMARKS);

chrome/browser/sync/glue/generic_change_processor.cc

@@ -456,7 +456,7 @@ syncer::SyncError GenericChangeProcessor::ProcessSyncChanges(
           syncer::Cryptographer* crypto = trans.GetCryptographer();
           syncer::ModelTypeSet encrypted_types(trans.GetEncryptedTypes());
           const sync_pb::EntitySpecifics& specifics =
-              sync_node.GetEntry()->Get(syncer::syncable::SPECIFICS);
+              sync_node.GetEntry()->GetSpecifics();
           CHECK(specifics.has_encrypted());
           const bool can_decrypt = crypto->CanDecrypt(specifics.encrypted());
           const bool agreement = encrypted_types.Has(type);

chrome/browser/sync/glue/typed_url_change_processor.cc

@@ -133,7 +133,7 @@ bool TypedUrlChangeProcessor::CreateOrUpdateSyncNode(
     syncer::Cryptographer* crypto = trans->GetCryptographer();
     syncer::ModelTypeSet encrypted_types(trans->GetEncryptedTypes());
     const sync_pb::EntitySpecifics& specifics =
-        update_node.GetEntry()->Get(syncer::syncable::SPECIFICS);
+        update_node.GetEntry()->GetSpecifics();
     CHECK(specifics.has_encrypted());
     const bool can_decrypt = crypto->CanDecrypt(specifics.encrypted());
     const bool agreement = encrypted_types.Has(syncer::TYPED_URLS);

chrome/browser/sync/profile_sync_service_autofill_unittest.cc

@@ -867,15 +867,14 @@ class FakeServerUpdater : public base::RefCountedThreadSafe<FakeServerUpdater> {
       // Simulates effects of UpdateLocalDataFromServerData
       MutableEntry parent(&trans, GET_BY_SERVER_TAG,
                           syncer::ModelTypeToRootTag(syncer::AUTOFILL));
-      MutableEntry item(&trans, CREATE, syncer::AUTOFILL,
-                        parent.Get(syncer::syncable::ID), tag);
+      MutableEntry item(&trans, CREATE, syncer::AUTOFILL, parent.GetId(), tag);
       ASSERT_TRUE(item.good());
-      item.Put(SPECIFICS, entity_specifics);
-      item.Put(SERVER_SPECIFICS, entity_specifics);
-      item.Put(BASE_VERSION, 1);
+      item.PutSpecifics(entity_specifics);
+      item.PutServerSpecifics(entity_specifics);
+      item.PutBaseVersion(1);
       syncer::syncable::Id server_item_id =
           service_->id_factory()->NewServerId();
-      item.Put(syncer::syncable::ID, server_item_id);
+      item.PutId(server_item_id);
       syncer::syncable::Id new_predecessor;
       ASSERT_TRUE(item.PutPredecessor(new_predecessor));
     }

chrome/browser/sync/profile_sync_service_bookmark_unittest.cc

@@ -128,8 +128,7 @@ class FakeServerChange {
       EXPECT_EQ(BaseNode::INIT_OK, node.InitByIdLookup(id));
       if (node.GetIsFolder())
         EXPECT_FALSE(node.GetFirstChildId());
-      node.GetMutableEntryForTest()->Put(syncer::syncable::SERVER_IS_DEL,
-                                         true);
+      node.GetMutableEntryForTest()->PutServerIsDel(true);
       node.Tombstone();
     }
     {
@@ -418,8 +417,7 @@ class ProfileSyncServiceBookmarkTest : public testing::Test {
       if (!node.InitBookmarkByCreation(root, predecessor))
         return false;
       node.SetIsFolder(true);
-      node.GetMutableEntryForTest()->Put(
-          syncer::syncable::UNIQUE_SERVER_TAG, permanent_tags[i]);
+      node.GetMutableEntryForTest()->PutUniqueServerTag(permanent_tags[i]);
       node.SetTitle(UTF8ToWide(permanent_tags[i]));
       node.SetExternalId(0);
       last_child_id = node.GetId();
@@ -1867,8 +1865,7 @@ void ProfileSyncServiceBookmarkTestWithData::ExpectTransactionVersionMatch(
     syncer::ReadNode sync_node(&trans);
     ASSERT_TRUE(model_associator_->InitSyncNodeFromChromeId(it->first,
                                                             &sync_node));
-    EXPECT_EQ(sync_node.GetEntry()->Get(syncer::syncable::TRANSACTION_VERSION),
-              it->second);
+    EXPECT_EQ(sync_node.GetEntry()->GetTransactionVersion(), it->second);
     BookmarkNodeVersionMap::const_iterator expected_ver_it =
         version_expected.find(it->first);
     if (expected_ver_it != version_expected.end())

sync/engine/apply_control_data_updates.cc

@@ -45,7 +45,7 @@ void ApplyControlDataUpdates(sessions::SyncSession* session) {
                                  ModelTypeToRootTag(iter.Get()));
     if (!entry.good())
       continue;
-    if (!entry.Get(syncable::IS_UNAPPLIED_UPDATE))
+    if (!entry.GetIsUnappliedUpdate())
       continue;
 
     ModelType type = entry.GetServerModelType();
@@ -69,9 +69,9 @@ void ApplyControlDataUpdates(sessions::SyncSession* session) {
     CHECK(entry.good());
     ModelType type = entry.GetServerModelType();
     CHECK(ControlTypes().Has(type));
-    if (!entry.Get(syncable::UNIQUE_SERVER_TAG).empty()) {
+    if (!entry.GetUniqueServerTag().empty()) {
       // We should have already applied all top level control nodes.
-      DCHECK(!entry.Get(syncable::IS_UNAPPLIED_UPDATE));
+      DCHECK(!entry.GetIsUnappliedUpdate());
       continue;
     }
 
@@ -95,15 +95,15 @@ void ApplyControlDataUpdates(sessions::SyncSession* session) {
 void ApplyNigoriUpdate(syncable::WriteTransaction* const trans,
                        syncable::MutableEntry* const entry,
                        Cryptographer* cryptographer) {
-  DCHECK(entry->Get(IS_UNAPPLIED_UPDATE));
+  DCHECK(entry->GetIsUnappliedUpdate());
 
   // We apply the nigori update regardless of whether there's a conflict or
   // not in order to preserve any new encrypted types or encryption keys.
   // TODO(zea): consider having this return a bool reflecting whether it was a
   // valid update or not, and in the case of invalid updates not overwrite the
   // local data.
   const sync_pb::NigoriSpecifics& nigori =
-      entry->Get(SERVER_SPECIFICS).nigori();
+      entry->GetServerSpecifics().nigori();
   trans->directory()->GetNigoriHandler()->ApplyNigoriUpdate(nigori, trans);
 
   // Make sure any unsynced changes are properly encrypted as necessary.
@@ -125,19 +125,19 @@ void ApplyNigoriUpdate(syncable::WriteTransaction* const trans,
     syncable::ProcessUnsyncedChangesForEncryption(trans);
   }
 
-  if (!entry->Get(IS_UNSYNCED)) {  // Update only.
+  if (!entry->GetIsUnsynced()) {  // Update only.
     UpdateLocalDataFromServerData(trans, entry);
   } else {  // Conflict.
     const sync_pb::EntitySpecifics& server_specifics =
-        entry->Get(SERVER_SPECIFICS);
+        entry->GetServerSpecifics();
     const sync_pb::NigoriSpecifics& server_nigori = server_specifics.nigori();
     const sync_pb::EntitySpecifics& local_specifics =
-        entry->Get(SPECIFICS);
+        entry->GetSpecifics();
     const sync_pb::NigoriSpecifics& local_nigori = local_specifics.nigori();
 
     // We initialize the new nigori with the server state, and will override
     // it as necessary below.
-    sync_pb::EntitySpecifics new_specifics = entry->Get(SERVER_SPECIFICS);
+    sync_pb::EntitySpecifics new_specifics = entry->GetServerSpecifics();
     sync_pb::NigoriSpecifics* new_nigori = new_specifics.mutable_nigori();
 
     // If the cryptographer is not ready, another client set a new encryption
@@ -188,7 +188,7 @@ void ApplyNigoriUpdate(syncable::WriteTransaction* const trans,
         new_nigori,
         trans);
 
-    entry->Put(SPECIFICS, new_specifics);
+    entry->PutSpecifics(new_specifics);
     DVLOG(1) << "Resolving simple conflict, merging nigori nodes: "
              << entry;
 
@@ -204,8 +204,8 @@ void ApplyControlUpdate(syncable::WriteTransaction* const trans,
                         syncable::MutableEntry* const entry,
                         Cryptographer* cryptographer) {
   DCHECK_NE(entry->GetServerModelType(), NIGORI);
-  DCHECK(entry->Get(IS_UNAPPLIED_UPDATE));
-  if (entry->Get(IS_UNSYNCED)) {
+  DCHECK(entry->GetIsUnappliedUpdate());
+  if (entry->GetIsUnsynced()) {
       // We just let the server win all conflicts with control types.
     DVLOG(1) << "Ignoring local changes for control update.";
     conflict_util::IgnoreLocalChanges(entry);

sync/engine/apply_control_data_updates_unittest.cc

@@ -190,8 +190,8 @@ TEST_F(ApplyControlDataUpdatesTest, EncryptUnsyncedChanges) {
     MutableEntry entry(&trans, syncable::GET_BY_SERVER_TAG,
                        ModelTypeToRootTag(NIGORI));
     ASSERT_TRUE(entry.good());
-    entry.Put(syncable::SERVER_VERSION, entry_factory_->GetNextRevision());
-    entry.Put(syncable::IS_UNAPPLIED_UPDATE, true);
+    entry.PutServerVersion(entry_factory_->GetNextRevision());
+    entry.PutIsUnappliedUpdate(true);
   }
 
   ApplyControlDataUpdates(session());

sync/engine/apply_updates_and_resolve_conflicts_command_unittest.cc

@@ -146,8 +146,7 @@ TEST_F(ApplyUpdatesAndResolveConflictsCommandTest, HierarchyAndSimpleConflict) {
     MutableEntry entry(&trans, syncable::GET_BY_HANDLE, handle);
     ASSERT_TRUE(entry.good());
 
-    entry.Put(syncable::SERVER_PARENT_ID,
-              TestIdFactory::MakeServer("bogus_parent"));
+    entry.PutServerParentId(TestIdFactory::MakeServer("bogus_parent"));
   }
 
   ExpectGroupToChange(apply_updates_command_, GROUP_UI);
@@ -175,9 +174,9 @@ TEST_F(ApplyUpdatesAndResolveConflictsCommandTest,
     ASSERT_TRUE(entry.good());
 
     // Re-parent from root to "Y"
-    entry.Put(syncable::SERVER_VERSION, entry_factory_->GetNextRevision());
-    entry.Put(syncable::IS_UNAPPLIED_UPDATE, true);
-    entry.Put(syncable::SERVER_PARENT_ID, TestIdFactory::MakeServer("Y"));
+    entry.PutServerVersion(entry_factory_->GetNextRevision());
+    entry.PutIsUnappliedUpdate(true);
+    entry.PutServerParentId(TestIdFactory::MakeServer("Y"));
   }
 
   // Item 'Y' is child of 'X'.
@@ -213,7 +212,7 @@ TEST_F(ApplyUpdatesAndResolveConflictsCommandTest,
   {
     WriteTransaction trans(FROM_HERE, UNITTEST, directory());
     MutableEntry entry(&trans, syncable::GET_BY_HANDLE, parent_handle);
-    entry.Put(syncable::IS_DEL, true);
+    entry.PutIsDel(true);
   }
 
   // Create an incoming child from the server.
@@ -247,10 +246,10 @@ TEST_F(ApplyUpdatesAndResolveConflictsCommandTest,
     ASSERT_TRUE(entry.good());
 
     // Delete it on the server.
-    entry.Put(syncable::SERVER_VERSION, entry_factory_->GetNextRevision());
-    entry.Put(syncable::IS_UNAPPLIED_UPDATE, true);
-    entry.Put(syncable::SERVER_PARENT_ID, TestIdFactory::root());
-    entry.Put(syncable::SERVER_IS_DEL, true);
+    entry.PutServerVersion(entry_factory_->GetNextRevision());
+    entry.PutIsUnappliedUpdate(true);
+    entry.PutServerParentId(TestIdFactory::root());
+    entry.PutServerIsDel(true);
   }
 
   // Create a local child of the server-deleted directory.

sync/engine/build_commit_command.cc

@@ -101,8 +101,8 @@ namespace {
 void SetEntrySpecifics(const Entry& meta_entry,
                        sync_pb::SyncEntity* sync_entry) {
   // Add the new style extension and the folder bit.
-  sync_entry->mutable_specifics()->CopyFrom(meta_entry.Get(SPECIFICS));
-  sync_entry->set_folder(meta_entry.Get(syncable::IS_DIR));
+  sync_entry->mutable_specifics()->CopyFrom(meta_entry.GetSpecifics());
+  sync_entry->set_folder(meta_entry.GetIsDir());
 
   CHECK(!sync_entry->specifics().password().has_client_only_encrypted_data());
   DCHECK_EQ(meta_entry.GetModelType(), GetModelType(*sync_entry));
@@ -141,10 +141,10 @@ SyncerError BuildCommitCommand::ExecuteImpl(SyncSession* session) {
 void BuildCommitCommand::BuildCommitItem(
     const syncable::Entry& meta_entry,
     sync_pb::SyncEntity* sync_entry) {
-  syncable::Id id = meta_entry.Get(syncable::ID);
+  syncable::Id id = meta_entry.GetId();
   sync_entry->set_id_string(SyncableIdToProto(id));
 
-  string name = meta_entry.Get(syncable::NON_UNIQUE_NAME);
+  string name = meta_entry.GetNonUniqueName();
   CHECK(!name.empty());  // Make sure this isn't an update.
   // Note: Truncation is also performed in WriteNode::SetTitle(..). But this
   // call is still necessary to handle any title changes that might originate
@@ -158,19 +158,19 @@ void BuildCommitCommand::BuildCommitItem(
   // We send both because it may aid in logging.
   sync_entry->set_non_unique_name(name);
 
-  if (!meta_entry.Get(syncable::UNIQUE_CLIENT_TAG).empty()) {
+  if (!meta_entry.GetUniqueClientTag().empty()) {
     sync_entry->set_client_defined_unique_tag(
-        meta_entry.Get(syncable::UNIQUE_CLIENT_TAG));
+        meta_entry.GetUniqueClientTag());
   }
 
   // Deleted items with server-unknown parent ids can be a problem so we set
   // the parent to 0. (TODO(sync): Still true in protocol?).
   Id new_parent_id;
-  if (meta_entry.Get(syncable::IS_DEL) &&
-      !meta_entry.Get(syncable::PARENT_ID).ServerKnows()) {
+  if (meta_entry.GetIsDel() &&
+      !meta_entry.GetParentId().ServerKnows()) {
     new_parent_id = syncable::BaseTransaction::root_id();
   } else {
-    new_parent_id = meta_entry.Get(syncable::PARENT_ID);
+    new_parent_id = meta_entry.GetParentId();
   }
   sync_entry->set_parent_id_string(SyncableIdToProto(new_parent_id));
 
@@ -179,43 +179,43 @@ void BuildCommitCommand::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.Get(syncable::SERVER_PARENT_ID) &&
-      0 != meta_entry.Get(syncable::BASE_VERSION) &&
-      syncable::CHANGES_VERSION != meta_entry.Get(syncable::BASE_VERSION)) {
+  if (new_parent_id != meta_entry.GetServerParentId() &&
+      0 != meta_entry.GetBaseVersion() &&
+      syncable::CHANGES_VERSION != meta_entry.GetBaseVersion()) {
     sync_entry->set_old_parent_id(
-        SyncableIdToProto(meta_entry.Get(syncable::SERVER_PARENT_ID)));
+        SyncableIdToProto(meta_entry.GetServerParentId()));
   }
 
-  int64 version = meta_entry.Get(syncable::BASE_VERSION);
+  int64 version = meta_entry.GetBaseVersion();
   if (syncable::CHANGES_VERSION == version || 0 == version) {
     // Undeletions are only supported for items that have a client tag.
     DCHECK(!id.ServerKnows() ||
-           !meta_entry.Get(syncable::UNIQUE_CLIENT_TAG).empty())
+           !meta_entry.GetUniqueClientTag().empty())
         << meta_entry;
 
     // Version 0 means to create or undelete an object.
     sync_entry->set_version(0);
   } else {
     DCHECK(id.ServerKnows()) << meta_entry;
-    sync_entry->set_version(meta_entry.Get(syncable::BASE_VERSION));
+    sync_entry->set_version(meta_entry.GetBaseVersion());
   }
-  sync_entry->set_ctime(TimeToProtoTime(meta_entry.Get(syncable::CTIME)));
-  sync_entry->set_mtime(TimeToProtoTime(meta_entry.Get(syncable::MTIME)));
+  sync_entry->set_ctime(TimeToProtoTime(meta_entry.GetCtime()));
+  sync_entry->set_mtime(TimeToProtoTime(meta_entry.GetMtime()));
 
   // Deletion is final on the server, let's move things and then delete them.
-  if (meta_entry.Get(IS_DEL)) {
+  if (meta_entry.GetIsDel()) {
     sync_entry->set_deleted(true);
   } else {
-    if (meta_entry.Get(SPECIFICS).has_bookmark()) {
+    if (meta_entry.GetSpecifics().has_bookmark()) {
       // Both insert_after_item_id and position_in_parent fields are set only
       // 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();
       sync_entry->set_insert_after_item_id(prev_id_string);
       sync_entry->set_position_in_parent(
-          meta_entry.Get(UNIQUE_POSITION).ToInt64());
-      meta_entry.Get(UNIQUE_POSITION).ToProto(
+          meta_entry.GetUniquePosition().ToInt64());
+      meta_entry.GetUniquePosition().ToProto(
           sync_entry->mutable_unique_position());
     }
     SetEntrySpecifics(meta_entry, sync_entry);

sync/engine/commit.cc

@@ -32,7 +32,7 @@ void SetAllSyncingBitsToValue(WriteTransaction* trans,
        it != commit_handles.end(); ++it) {
     syncable::MutableEntry entry(trans, syncable::GET_BY_HANDLE, *it);
     if (entry.good()) {
-      entry.Put(syncable::SYNCING, value_to_set);
+      entry.PutSyncing(value_to_set);
     }
   }
 }