[Sync] WriteNode::SetTitle doesn't update bookmark title when it is empty string.

sync/internal_api/write_node.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "sync/internal_api/public/write_node.h"
 
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "sync/internal_api/public/base_transaction.h"
@@ skipping 59 matching lines @@
   } else {
     // Non-bookmarks and legacy bookmarks (those with no title in their
     // specifics) store their title in NON_UNIQUE_NAME. Non-legacy bookmarks
     // store their title in specifics as well as NON_UNIQUE_NAME.
     current_legal_title = entry_->GetNonUniqueName();
   }
 
   bool title_matches = (current_legal_title == new_legal_title);
   bool encrypted_without_overwriting_name = (needs_encryption &&
       entry_->GetNonUniqueName() != kEncryptedString);
+  bool bookmark_title_updated = false;

[Nicolas Zea, 2015/05/15 20:15:17]

Does it make sense to, instead of having this new boolean, to just update
title_matches above to be false if specifics.bookmark().title() !=
new_legal_title?

[pavely, 2015/05/15 21:45:28]

I used title_matches first, but decided to use separate variable because
title_matches reflects comparison of NonUniqueName(), not bookmark's title. I
think updating title_matches is fine. I'll change code.

+
+  // For bookmarks, we also set the title field in the specifics.
+  // TODO(zea): refactor bookmarks to not need this functionality.
+  sync_pb::EntitySpecifics specifics = GetEntitySpecifics();
+  if (GetModelType() == BOOKMARKS &&
+      specifics.bookmark().title() != new_legal_title) {
+    specifics.mutable_bookmark()->set_title(new_legal_title);
+    SetEntitySpecifics(specifics);  // Does it's own encryption checking.
+    bookmark_title_updated = true;
+  }
 
   // If the title matches and the NON_UNIQUE_NAME is properly overwritten as
   // necessary, nothing needs to change.
-  if (title_matches && !encrypted_without_overwriting_name) {
+  if (title_matches && !encrypted_without_overwriting_name &&
+      !bookmark_title_updated) {
     DVLOG(2) << "Title matches, dropping change.";
     return;
   }
 
-  // For bookmarks, we also set the title field in the specifics.
-  // TODO(zea): refactor bookmarks to not need this functionality.
-  if (GetModelType() == BOOKMARKS) {
-    sync_pb::EntitySpecifics specifics = GetEntitySpecifics();
-    specifics.mutable_bookmark()->set_title(new_legal_title);
-    SetEntitySpecifics(specifics);  // Does it's own encryption checking.
-  }
-
   // For bookmarks, this has to happen after we set the title in the specifics,
   // because the presence of a title in the NON_UNIQUE_NAME is what controls
   // the logic deciding whether this is an empty node or a legacy bookmark.
   // See BaseNode::GetUnencryptedSpecific(..).
   if (needs_encryption)
     entry_->PutNonUniqueName(kEncryptedString);
   else
     entry_->PutNonUniqueName(new_legal_title);
 
   DVLOG(1) << "Overwriting title of type "
@@ skipping 429 matching lines @@
   syncable::Id predecessor_id = predecessor ?
       predecessor->GetEntry()->GetId() : syncable::Id();
   return entry_->PutPredecessor(predecessor_id);
 }
 
 void WriteNode::MarkForSyncing() {
   syncable::MarkForSyncing(entry_);
 }
 
 }  // namespace syncer