[Sync] Prevent redundant sync title changes

Title truncation is now performed in the WriteNode, before we do the
idempotency checks. By fixing this and an issue in the favicon cache
that was leading to automatic rewrites we should significantly reduce
the number of useless sync cycles.

BUG=229658

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

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

4 files changed, 59 insertions, 2 deletions

diff --git a/chrome/browser/sync/glue/favicon_cache.cc b/chrome/browser/sync/glue/favicon_cache.cc
index 8871fde..e14b763 100644
--- a/chrome/browser/sync/glue/favicon_cache.cc
+++ b/chrome/browser/sync/glue/favicon_cache.cc
@@ -88,12 +88,13 @@ IconSize GetIconSizeBinFromBitmapResult(const gfx::Size& pixel_size) {
   int max_size =
       (pixel_size.width() > pixel_size.height() ?
        pixel_size.width() : pixel_size.height());
+  // TODO(zea): re-enable 64p and 32p resolutions once we support them.
   if (max_size > 64)
     return SIZE_INVALID;
   else if (max_size > 32)
-    return SIZE_64;
+    return SIZE_INVALID;
   else if (max_size > 16)
-    return SIZE_32;
+    return SIZE_INVALID;
   else
     return SIZE_16;
 }
diff --git a/sync/engine/build_commit_command.cc b/sync/engine/build_commit_command.cc
index 96772ad..71a67f2 100644
--- a/sync/engine/build_commit_command.cc
+++ b/sync/engine/build_commit_command.cc
@@ -139,6 +139,9 @@ SyncerError BuildCommitCommand::ExecuteImpl(SyncSession* session) {
 
     string name = meta_entry.Get(syncable::NON_UNIQUE_NAME);
     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
+    // elsewhere, or already be persisted in the directory.
     TruncateUTF8ToByteSize(name, 255, &name);
     sync_entry->set_name(name);
 
diff --git a/sync/internal_api/sync_manager_impl_unittest.cc b/sync/internal_api/sync_manager_impl_unittest.cc
index c236e5b..1d38b531 100644
--- a/sync/internal_api/sync_manager_impl_unittest.cc
+++ b/sync/internal_api/sync_manager_impl_unittest.cc
@@ -2679,6 +2679,57 @@ TEST_F(SyncManagerTest, SetNonBookmarkTitleWithEncryption) {
   }
 }
 
+// Ensure that titles are truncated to 255 bytes, and attempting to reset
+// them to their longer version does not set IS_UNSYNCED.
+TEST_F(SyncManagerTest, SetLongTitle) {
+  const int kNumChars = 512;
+  const std::string kClientTag = "tag";
+  std::string title(kNumChars, '0');
+  sync_pb::EntitySpecifics entity_specifics;
+  entity_specifics.mutable_preference()->set_name("name");
+  entity_specifics.mutable_preference()->set_value("value");
+  MakeServerNode(sync_manager_.GetUserShare(),
+                 PREFERENCES,
+                 "short_title",
+                 syncable::GenerateSyncableHash(PREFERENCES,
+                                                kClientTag),
+                 entity_specifics);
+  // New node shouldn't start off unsynced.
+  EXPECT_FALSE(ResetUnsyncedEntry(PREFERENCES, kClientTag));
+
+  // Manually change to the long title. Should set is_unsynced.
+  {
+    WriteTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
+    WriteNode node(&trans);
+    EXPECT_EQ(BaseNode::INIT_OK,
+              node.InitByClientTagLookup(PREFERENCES, kClientTag));
+    node.SetTitle(UTF8ToWide(title));
+    EXPECT_EQ(node.GetTitle(), title.substr(0, 255));
+  }
+  EXPECT_TRUE(ResetUnsyncedEntry(PREFERENCES, kClientTag));
+
+  // Manually change to the same title. Should not set is_unsynced.
+  {
+    WriteTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
+    WriteNode node(&trans);
+    EXPECT_EQ(BaseNode::INIT_OK,
+              node.InitByClientTagLookup(PREFERENCES, kClientTag));
+    node.SetTitle(UTF8ToWide(title));
+    EXPECT_EQ(node.GetTitle(), title.substr(0, 255));
+  }
+  EXPECT_FALSE(ResetUnsyncedEntry(PREFERENCES, kClientTag));
+
+  // Manually change to new title. Should set is_unsynced.
+  {
+    WriteTransaction trans(FROM_HERE, sync_manager_.GetUserShare());
+    WriteNode node(&trans);
+    EXPECT_EQ(BaseNode::INIT_OK,
+              node.InitByClientTagLookup(PREFERENCES, kClientTag));
+    node.SetTitle(UTF8ToWide("title2"));
+  }
+  EXPECT_TRUE(ResetUnsyncedEntry(PREFERENCES, kClientTag));
+}
+
 // Create an encrypted entry when the cryptographer doesn't think the type is
 // marked for encryption. Ensure reads/writes don't break and don't unencrypt
 // the data.
diff --git a/sync/internal_api/write_node.cc b/sync/internal_api/write_node.cc
index 6ab752c..3a96764 100644
--- a/sync/internal_api/write_node.cc
+++ b/sync/internal_api/write_node.cc
@@ -4,6 +4,7 @@
 
 #include "sync/internal_api/public/write_node.h"
 
+#include "base/string_util.h"
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
 #include "sync/internal_api/public/base_transaction.h"
@@ -57,6 +58,7 @@ void WriteNode::SetTitle(const std::wstring& title) {
     new_legal_title = kEncryptedString;
   } else {
     SyncAPINameToServerName(WideToUTF8(title), &new_legal_title);
+    TruncateUTF8ToByteSize(new_legal_title, 255, &new_legal_title);
   }
 
   std::string current_legal_title;