[Sync] Ensure we set the title of nodes in the generic change processor.

This is necessary because it affects conflict resolution, and helps with debugging.

BUG=86078
TEST=about:sync, sync node browser, preferences have names


Review URL: http://codereview.chromium.org/7144017

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

5 files changed, 39 insertions, 8 deletions

diff --git a/chrome/browser/sync/api/sync_change.cc b/chrome/browser/sync/api/sync_change.cc
index 320dd53..dcf5328 100644
--- a/chrome/browser/sync/api/sync_change.cc
+++ b/chrome/browser/sync/api/sync_change.cc
@@ -29,7 +29,8 @@ bool SyncChange::IsValid() const {
 
   // Adds and updates must have valid specifics (checked by GetDataType())
   if (change_type_ == ACTION_ADD || change_type_ == ACTION_UPDATE)
-    return (sync_data_.GetDataType() != syncable::UNSPECIFIED);
+    return (sync_data_.GetDataType() != syncable::UNSPECIFIED &&
+            !sync_data_.GetTitle().empty());
 
   return true;
 }
diff --git a/chrome/browser/sync/api/sync_change_unittest.cc b/chrome/browser/sync/api/sync_change_unittest.cc
index 0a9104d..e8cdd21e 100644
--- a/chrome/browser/sync/api/sync_change_unittest.cc
+++ b/chrome/browser/sync/api/sync_change_unittest.cc
@@ -37,10 +37,12 @@ TEST_F(SyncChangeTest, LocalUpdate) {
       specifics.MutableExtension(sync_pb::preference);
   pref_specifics->set_name("test");
   std::string tag = "client_tag";
+  std::string title = "client_title";
   SyncChange e(change_type,
-              SyncData::CreateLocalData(tag, specifics));
+              SyncData::CreateLocalData(tag, title, specifics));
   EXPECT_EQ(change_type, e.change_type());
   EXPECT_EQ(tag, e.sync_data().GetTag());
+  EXPECT_EQ(title, e.sync_data().GetTitle());
   EXPECT_EQ(syncable::PREFERENCES, e.sync_data().GetDataType());
   scoped_ptr<DictionaryValue> ref_spec(EntitySpecificsToValue(specifics));
   scoped_ptr<DictionaryValue> e_spec(EntitySpecificsToValue(
@@ -55,10 +57,12 @@ TEST_F(SyncChangeTest, LocalAdd) {
       specifics.MutableExtension(sync_pb::preference);
   pref_specifics->set_name("test");
   std::string tag = "client_tag";
+  std::string title = "client_title";
   SyncChange e(change_type,
-              SyncData::CreateLocalData(tag, specifics));
+              SyncData::CreateLocalData(tag, title, specifics));
   EXPECT_EQ(change_type, e.change_type());
   EXPECT_EQ(tag, e.sync_data().GetTag());
+  EXPECT_EQ(title, e.sync_data().GetTitle());
   EXPECT_EQ(syncable::PREFERENCES, e.sync_data().GetDataType());
   scoped_ptr<DictionaryValue> ref_spec(EntitySpecificsToValue(specifics));
   scoped_ptr<DictionaryValue> e_spec(EntitySpecificsToValue(
diff --git a/chrome/browser/sync/api/sync_data.cc b/chrome/browser/sync/api/sync_data.cc
index 3cdf340..d1cc86c 100644
--- a/chrome/browser/sync/api/sync_data.cc
+++ b/chrome/browser/sync/api/sync_data.cc
@@ -39,9 +39,11 @@ SyncData SyncData::CreateLocalData(const std::string& sync_tag) {
 // Static.
 SyncData SyncData::CreateLocalData(
     const std::string& sync_tag,
+    const std::string& non_unique_title,
     const sync_pb::EntitySpecifics& specifics) {
   sync_pb::SyncEntity entity;
   entity.set_client_defined_unique_tag(sync_tag);
+  entity.set_non_unique_name(non_unique_title);
   entity.mutable_specifics()->CopyFrom(specifics);
   SyncData a;
   a.shared_entity_ = new SharedSyncEntity(&entity);
@@ -85,6 +87,12 @@ const std::string& SyncData::GetTag() const {
   return shared_entity_->sync_entity().client_defined_unique_tag();
 }
 
+const std::string& SyncData::GetTitle() const {
+  // TODO(zea): set this for data coming from the syncer too.
+  DCHECK(shared_entity_->sync_entity().has_non_unique_name());
+  return shared_entity_->sync_entity().non_unique_name();
+}
+
 bool SyncData::IsLocal() const {
   return is_local_;
 }
diff --git a/chrome/browser/sync/api/sync_data.h b/chrome/browser/sync/api/sync_data.h
index 1f238cb..7656de8 100644
--- a/chrome/browser/sync/api/sync_data.h
+++ b/chrome/browser/sync/api/sync_data.h
@@ -31,34 +31,50 @@ class SyncData {
    // Default copy and assign welcome.
 
   // Helper methods for creating SyncData objects for local data.
-  // Local sync data must always at least set the sync tag.
+  // Local sync data must always at least set the sync tag (which must be a
+  // string unique to this datatype), for use as a node identifier server-side.
+  // For adds/updates, the actual sync data that is changing (in the form of
+  // sync_pb::EntitySpecifics) and a human-readable/non-unique title (can be the
+  // same as sync tag) must also specfied.
+  // Note: the non_unique_title is primarily for debug purposes, and will be
+  // overwritten if the datatype is encrypted.
+  // For deletes:
   static SyncData CreateLocalData(const std::string& sync_tag);
+  // For adds/updates:
   static SyncData CreateLocalData(
       const std::string& sync_tag,
+      const std::string& non_unique_title,
       const sync_pb::EntitySpecifics& specifics);
 
-  // Helper method for creating SyncData objects originating from the syncer.
+  // Helper methods for creating SyncData objects originating from the syncer.
   static SyncData CreateRemoteData(
       const sync_pb::SyncEntity& entity);
   static SyncData CreateRemoteData(
-    const sync_pb::EntitySpecifics& specifics);
+      const sync_pb::EntitySpecifics& specifics);
 
   // Whether this SyncData holds valid data. The only way to have a SyncData
   // without valid data is to use the default constructor.
   bool IsValid() const;
+
   // Return the datatype we're holding information about. Derived from the sync
   // datatype specifics.
   SyncDataType GetDataType() const;
+
   // Return the current sync datatype specifics.
   const sync_pb::EntitySpecifics& GetSpecifics() const;
+
   // Returns the value of the unique client tag. This is only set for data going
   // TO the syncer, not coming from.
   const std::string& GetTag() const;
+
+  // Returns the non unique title (for debugging). Currently only set for data
+  // going TO the syncer, not from.
+  const std::string& GetTitle() const;
+
   // Whether this sync data is for local data or data coming from the syncer.
   bool IsLocal() const;
 
-  // TODO(zea): Query methods for other sync properties: title, parent,
-  // successor, etc. Possibly support for hashed tag and sync id?
+  // TODO(zea): Query methods for other sync properties: parent, successor, etc.
 
  private:
   // A reference counted immutable SyncEntity.
diff --git a/chrome/browser/sync/glue/generic_change_processor.cc b/chrome/browser/sync/glue/generic_change_processor.cc
index d2056d2..83c1736 100644
--- a/chrome/browser/sync/glue/generic_change_processor.cc
+++ b/chrome/browser/sync/glue/generic_change_processor.cc
@@ -138,6 +138,7 @@ void GenericChangeProcessor::ProcessSyncChanges(
             "Failed to create " + type_str + " node.");
         return;
       }
+      sync_node.SetTitle(UTF8ToWide(change.sync_data().GetTitle()));
       sync_node.SetEntitySpecifics(change.sync_data().GetSpecifics());
     } else if (change.change_type() == SyncChange::ACTION_UPDATE) {
       if (change.sync_data().GetTag() == "" ||
@@ -148,6 +149,7 @@ void GenericChangeProcessor::ProcessSyncChanges(
             "Failed to update " + type_str + " node");
         return;
       }
+      sync_node.SetTitle(UTF8ToWide(change.sync_data().GetTitle()));
       sync_node.SetEntitySpecifics(change.sync_data().GetSpecifics());
       // TODO(sync): Support updating other parts of the sync node (title,
       // successor, parent, etc.).