Fix a bug that would cause spurious commits from the bookmark model

under the following circumstances:

  (a) There were two or more bookmarks with the
      same title in a folder.

  (b) A change notification event occured for
      the bookmark (in my case it was a favicon
      loaded event; these would happen every
      startup, and maybe on every page visit too),
      but the bookmark properties didn't
      actually change.

What was happening is that we were filtering out redundant
property changes for the title, but only after we'd uniquified
the name.  This wasn't necessary, and for cases where
there was a sibling with an identical name, the newly
generated uniquified name would result in a false positive.

The fix is to do the filtering on the non-unique name only.
This is the only property visible to syncapi clients.

BUG=24770
TEST=See bug.

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

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

1 files changed, 9 insertions, 4 deletions

diff --git a/chrome/browser/sync/engine/syncapi.cc b/chrome/browser/sync/engine/syncapi.cc
index 2206024..b0eaa5b 100644
--- a/chrome/browser/sync/engine/syncapi.cc
+++ b/chrome/browser/sync/engine/syncapi.cc
@@ -336,6 +336,14 @@ void WriteNode::SetIsFolder(bool folder) {
 void WriteNode::SetTitle(const sync_char16* title) {
   PathString server_legal_name;
   SyncAPINameToServerName(title, &server_legal_name);
+
+  syncable::Name old_name = entry_->GetName();
+
+  if (server_legal_name == old_name.non_unique_value())
+    return;  // Skip redundant changes.
+
+  // Otherwise, derive a new unique name so we have a valid value
+  // to use as the DBName.
   syncable::SyncName sync_name(server_legal_name);
   syncable::DBName db_name(sync_name.value());
   db_name.MakeOSLegal();
@@ -343,10 +351,7 @@ void WriteNode::SetTitle(const sync_char16* title) {
                                    entry_->Get(syncable::PARENT_ID), entry_);
 
   syncable::Name new_name = syncable::Name::FromDBNameAndSyncName(db_name,
-                                                              sync_name);
-  if (new_name == entry_->GetName())
-    return;  // Skip redundant changes.
-
+                                                                  sync_name);
   entry_->PutName(new_name);
   MarkForSyncing();
 }