Update Commit and GetUpdatesResponse messages to include attachment ids.

Private function HasAttachmentNotOnServer is now tested (albeit
indirectly) by directory_commit_contribution_unittest.cc via
DirectoryCommitContribution::Build.

When displayed in about:sync, SyncEntity instances will now show any
attachment ids they contain.

This CL depends on issue 395913003.

BUG=356266,394023

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

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

18 files changed, 533 insertions, 10 deletions

diff --git a/sync/engine/commit_util.cc b/sync/engine/commit_util.cc
index ad29e4f..c913edd 100644
--- a/sync/engine/commit_util.cc
+++ b/sync/engine/commit_util.cc
@@ -11,6 +11,7 @@
 
 #include "base/strings/string_util.h"
 #include "sync/engine/syncer_proto_util.h"
+#include "sync/internal_api/public/base/attachment_id_proto.h"
 #include "sync/internal_api/public/base/unique_position.h"
 #include "sync/protocol/bookmark_specifics.pb.h"
 #include "sync/protocol/sync.pb.h"
@@ -83,6 +84,7 @@ void AddClientConfigParamsToMessage(
 }
 
 namespace {
+
 void SetEntrySpecifics(const Entry& meta_entry,
                        sync_pb::SyncEntity* sync_entry) {
   // Add the new style extension and the folder bit.
@@ -92,6 +94,16 @@ void SetEntrySpecifics(const Entry& meta_entry,
   CHECK(!sync_entry->specifics().password().has_client_only_encrypted_data());
   DCHECK_EQ(meta_entry.GetModelType(), GetModelType(*sync_entry));
 }
+
+void SetAttachmentIds(const Entry& meta_entry,
+                      sync_pb::SyncEntity* sync_entry) {
+  const sync_pb::AttachmentMetadata& attachment_metadata =
+      meta_entry.GetAttachmentMetadata();
+  for (int i = 0; i < attachment_metadata.record_size(); ++i) {
+    *sync_entry->add_attachment_id() = attachment_metadata.record(i).id();
+  }
+}
+
 }  // namespace
 
 void BuildCommitItem(
@@ -161,6 +173,8 @@ void BuildCommitItem(
   sync_entry->set_ctime(TimeToProtoTime(meta_entry.GetCtime()));
   sync_entry->set_mtime(TimeToProtoTime(meta_entry.GetMtime()));
 
+  SetAttachmentIds(meta_entry, sync_entry);
+
   // Handle bookmarks separately.
   if (meta_entry.GetSpecifics().has_bookmark()) {
     if (meta_entry.GetIsDel()) {
@@ -305,6 +319,8 @@ void UpdateServerFieldsAfterCommit(
       (committed_entry.folder() ||
        committed_entry.bookmarkdata().bookmark_folder()));
   local_entry->PutServerSpecifics(committed_entry.specifics());
+  local_entry->PutServerAttachmentMetadata(
+      CreateAttachmentMetadata(committed_entry.attachment_id()));
   local_entry->PutServerMtime(ProtoTimeToTime(committed_entry.mtime()));
   local_entry->PutServerCtime(ProtoTimeToTime(committed_entry.ctime()));
   if (committed_entry.has_unique_position()) {
diff --git a/sync/engine/conflict_resolver.cc b/sync/engine/conflict_resolver.cc
index 1630620..f914403 100644
--- a/sync/engine/conflict_resolver.cc
+++ b/sync/engine/conflict_resolver.cc
@@ -30,6 +30,39 @@ using syncable::Id;
 using syncable::MutableEntry;
 using syncable::WriteTransaction;
 
+namespace {
+
+// Returns true iff the set of attachment ids contained in attachment_metadata
+// matches the set of ids contained in server_attachment_metadata.
+bool AttachmentMetadataMatches(const MutableEntry& entity) {
+  const sync_pb::AttachmentMetadata& local = entity.GetAttachmentMetadata();
+  const sync_pb::AttachmentMetadata& server =
+      entity.GetServerAttachmentMetadata();
+  if (local.record_size() != server.record_size()) {
+    return false;
+  }
+
+  // The order of records in local and server may be different so use a std::set
+  // to determine if they are equivalent.
+  std::set<std::string> local_ids;
+  for (int i = 0; i < local.record_size(); ++i) {
+    const sync_pb::AttachmentMetadataRecord& record = local.record(i);
+    DCHECK(record.is_on_server());
+    local_ids.insert(record.id().SerializeAsString());
+  }
+  for (int i = 0; i < server.record_size(); ++i) {
+    const sync_pb::AttachmentMetadataRecord& record = server.record(i);
+    DCHECK(record.is_on_server());
+    if (local_ids.find(record.id().SerializeAsString()) == local_ids.end()) {
+      return false;
+    }
+  }
+
+  return true;
+}
+
+}  // namespace
+
 ConflictResolver::ConflictResolver() {
 }
 
@@ -148,8 +181,9 @@ void ConflictResolver::ProcessSimpleConflict(WriteTransaction* trans,
           base_server_specifics_match = true;
     }
 
+    bool attachment_metadata_matches = AttachmentMetadataMatches(entry);
     if (!entry_deleted && name_matches && parent_matches && specifics_match &&
-        position_matches) {
+        position_matches && attachment_metadata_matches) {
       DVLOG(1) << "Resolving simple conflict, everything matches, ignoring "
                << "changes for: " << entry;
       conflict_util::IgnoreConflict(&entry);
diff --git a/sync/engine/directory_commit_contribution_unittest.cc b/sync/engine/directory_commit_contribution_unittest.cc
index 894db97..7c12f50 100644
--- a/sync/engine/directory_commit_contribution_unittest.cc
+++ b/sync/engine/directory_commit_contribution_unittest.cc
@@ -5,6 +5,7 @@
 #include "sync/engine/directory_commit_contribution.h"
 
 #include "base/message_loop/message_loop.h"
+#include "sync/internal_api/public/base/attachment_id_proto.h"
 #include "sync/sessions/status_controller.h"
 #include "sync/syncable/entry.h"
 #include "sync/syncable/mutable_entry.h"
@@ -25,6 +26,7 @@ class DirectoryCommitContributionTest : public ::testing::Test {
     syncable::WriteTransaction trans(FROM_HERE, syncable::UNITTEST, dir());
     CreateTypeRoot(&trans, dir(), PREFERENCES);
     CreateTypeRoot(&trans, dir(), EXTENSIONS);
+    CreateTypeRoot(&trans, dir(), ARTICLES);
     CreateTypeRoot(&trans, dir(), BOOKMARKS);
   }
 
@@ -33,9 +35,11 @@ class DirectoryCommitContributionTest : public ::testing::Test {
   }
 
  protected:
-  int64 CreateUnsyncedItem(syncable::WriteTransaction* trans,
-                           ModelType type,
-                           const std::string& tag) {
+  int64 CreateUnsyncedItemWithAttachments(
+      syncable::WriteTransaction* trans,
+      ModelType type,
+      const std::string& tag,
+      const sync_pb::AttachmentMetadata& attachment_metadata) {
     syncable::Entry parent_entry(trans, syncable::GET_TYPE_ROOT, type);
     syncable::MutableEntry entry(
         trans,
@@ -43,10 +47,20 @@ class DirectoryCommitContributionTest : public ::testing::Test {
         type,
         parent_entry.GetId(),
         tag);
+    if (attachment_metadata.record_size() > 0) {
+      entry.PutAttachmentMetadata(attachment_metadata);
+    }
     entry.PutIsUnsynced(true);
     return entry.GetMetahandle();
   }
 
+  int64 CreateUnsyncedItem(syncable::WriteTransaction* trans,
+                           ModelType type,
+                           const std::string& tag) {
+    return CreateUnsyncedItemWithAttachments(
+        trans, type, tag, sync_pb::AttachmentMetadata());
+  }
+
   int64 CreateSyncedItem(syncable::WriteTransaction* trans,
                          ModelType type,
                          const std::string& tag) {
@@ -336,6 +350,13 @@ TEST_F(DirectoryCommitContributionTest, HierarchySupport_Preferences) {
   EXPECT_TRUE(commit_message.entries(0).parent_id_string().empty());
 }
 
+void AddAttachment(sync_pb::AttachmentMetadata* metadata, bool is_on_server) {
+  sync_pb::AttachmentMetadataRecord record;
+  *record.mutable_id() = CreateAttachmentIdProto();
+  record.set_is_on_server(is_on_server);
+  *metadata->add_record() = record;
+}
+
 // Creates some unsynced items, pretends to commit them, and hands back a
 // specially crafted response to the syncer in order to test commit response
 // processing.  The response simulates a succesful commit scenario.
@@ -400,4 +421,84 @@ TEST_F(DirectoryCommitContributionTest, ProcessCommitResponse) {
   ext_cc->CleanUp();
 }
 
+// Creates some unsynced items with attachments and verifies that only items
+// where all attachments have been uploaded to the server are eligible to be
+// committed.
+TEST_F(DirectoryCommitContributionTest, ProcessCommitResponseWithAttachments) {
+  int64 art1_handle;
+  int64 art2_handle;
+  int64 art3_handle;
+  {
+    syncable::WriteTransaction trans(FROM_HERE, syncable::UNITTEST, dir());
+
+    // art1 has two attachments, both have been uploaded to the server.  art1 is
+    // eligible to be committed.
+    sync_pb::AttachmentMetadata art1_attachments;
+    AddAttachment(&art1_attachments, true /* is_on_server */);
+    AddAttachment(&art1_attachments, true /* is_on_server */);
+    art1_handle = CreateUnsyncedItemWithAttachments(
+        &trans, ARTICLES, "art1", art1_attachments);
+
+    // art2 has two attachments, one of which has been uploaded to the
+    // server. art2 is not eligible to be committed.
+    sync_pb::AttachmentMetadata art2_attachments;
+    AddAttachment(&art2_attachments, false /* is_on_server */);
+    AddAttachment(&art2_attachments, true /* is_on_server */);
+    art2_handle = CreateUnsyncedItemWithAttachments(
+        &trans, ARTICLES, "art2", art2_attachments);
+
+    // art3 has two attachments, neither of which have been uploaded to the
+    // server. art2 is not eligible to be committed.
+    sync_pb::AttachmentMetadata art3_attachments;
+    AddAttachment(&art3_attachments, false /* is_on_server */);
+    AddAttachment(&art3_attachments, false /* is_on_server */);
+    art3_handle = CreateUnsyncedItemWithAttachments(
+        &trans, ARTICLES, "art3", art3_attachments);
+  }
+
+  DirectoryTypeDebugInfoEmitter emitter(ARTICLES, &type_observers_);
+  scoped_ptr<DirectoryCommitContribution> art_cc(
+      DirectoryCommitContribution::Build(dir(), ARTICLES, 25, &emitter));
+
+  // Only art1 is ready.
+  EXPECT_EQ(1U, art_cc->GetNumEntries());
+
+  sync_pb::ClientToServerMessage message;
+  art_cc->AddToCommitMessage(&message);
+
+  const sync_pb::CommitMessage& commit_message = message.commit();
+  ASSERT_EQ(1, commit_message.entries_size());
+
+  sync_pb::ClientToServerResponse response;
+  for (int i = 0; i < commit_message.entries_size(); ++i) {
+    sync_pb::SyncEntity entity = commit_message.entries(i);
+    sync_pb::CommitResponse_EntryResponse* entry_response =
+        response.mutable_commit()->add_entryresponse();
+    CreateSuccessfulCommitResponse(entity, entry_response);
+  }
+
+  sessions::StatusController status;
+  art_cc->ProcessCommitResponse(response, &status);
+  {
+    syncable::ReadTransaction trans(FROM_HERE, dir());
+
+    syncable::Entry a1(&trans, syncable::GET_BY_HANDLE, art1_handle);
+    EXPECT_TRUE(a1.GetId().ServerKnows());
+    EXPECT_FALSE(a1.GetSyncing());
+    EXPECT_LT(0, a1.GetServerVersion());
+
+    syncable::Entry a2(&trans, syncable::GET_BY_HANDLE, art2_handle);
+    EXPECT_FALSE(a2.GetId().ServerKnows());
+    EXPECT_FALSE(a2.GetSyncing());
+    EXPECT_EQ(0, a2.GetServerVersion());
+
+    syncable::Entry a3(&trans, syncable::GET_BY_HANDLE, art3_handle);
+    EXPECT_FALSE(a3.GetId().ServerKnows());
+    EXPECT_FALSE(a3.GetSyncing());
+    EXPECT_EQ(0, a3.GetServerVersion());
+  }
+
+  art_cc->CleanUp();
+}
+
 }  // namespace syncer
diff --git a/sync/engine/directory_update_handler_unittest.cc b/sync/engine/directory_update_handler_unittest.cc
index 837e9f4..6052da19 100644
--- a/sync/engine/directory_update_handler_unittest.cc
+++ b/sync/engine/directory_update_handler_unittest.cc
@@ -8,6 +8,7 @@
 #include "base/message_loop/message_loop.h"
 #include "base/stl_util.h"
 #include "sync/engine/syncer_proto_util.h"
+#include "sync/internal_api/public/base/attachment_id_proto.h"
 #include "sync/internal_api/public/base/model_type.h"
 #include "sync/internal_api/public/test/test_entry_factory.h"
 #include "sync/protocol/sync.pb.h"
@@ -396,6 +397,69 @@ TEST_F(DirectoryUpdateHandlerProcessUpdateTest, ContextVersion) {
   }
 }
 
+// See that updates containing attachment metadata are applied
+// (i.e. server_attachment_metadata is copied to attachment_metadata).
+TEST_F(DirectoryUpdateHandlerProcessUpdateTest,
+       ProcessAndApplyUpdatesWithAttachments) {
+  DirectoryTypeDebugInfoEmitter emitter(ARTICLES, &type_observers_);
+  DirectoryUpdateHandler handler(dir(), ARTICLES, ui_worker(), &emitter);
+  sessions::StatusController status;
+
+  sync_pb::DataTypeProgressMarker progress;
+  progress.set_data_type_id(GetSpecificsFieldNumberFromModelType(ARTICLES));
+  progress.set_token("token");
+  progress.mutable_gc_directive()->set_version_watermark(kDefaultVersion + 10);
+
+  sync_pb::DataTypeContext context;
+  context.set_data_type_id(GetSpecificsFieldNumberFromModelType(ARTICLES));
+  context.set_context("context");
+  context.set_version(1);
+
+  scoped_ptr<sync_pb::SyncEntity> type_root =
+      CreateUpdate(SyncableIdToProto(syncable::Id::CreateFromServerId("root")),
+                   syncable::GetNullId().GetServerId(),
+                   ARTICLES);
+  type_root->set_server_defined_unique_tag(ModelTypeToRootTag(ARTICLES));
+  type_root->set_folder(true);
+
+  scoped_ptr<sync_pb::SyncEntity> e1 =
+      CreateUpdate(SyncableIdToProto(syncable::Id::CreateFromServerId("e1")),
+                   type_root->id_string(),
+                   ARTICLES);
+  sync_pb::AttachmentIdProto* attachment_id = e1->add_attachment_id();
+  *attachment_id = CreateAttachmentIdProto();
+
+  SyncEntityList updates;
+  updates.push_back(type_root.get());
+  updates.push_back(e1.get());
+
+  // Process and apply updates.
+  EXPECT_EQ(
+      SYNCER_OK,
+      handler.ProcessGetUpdatesResponse(progress, context, updates, &status));
+  handler.ApplyUpdates(&status);
+
+  ASSERT_TRUE(EntryExists(type_root->id_string()));
+  ASSERT_TRUE(EntryExists(e1->id_string()));
+  {
+    syncable::ReadTransaction trans(FROM_HERE, dir());
+    syncable::Entry e(&trans,
+                      syncable::GET_BY_ID,
+                      syncable::Id::CreateFromServerId(e1->id_string()));
+
+    // See that the attachment_metadata is correct.
+    sync_pb::AttachmentMetadata attachment_metadata = e.GetAttachmentMetadata();
+    ASSERT_EQ(1, attachment_metadata.record_size());
+    ASSERT_EQ(attachment_id->SerializeAsString(),
+              attachment_metadata.record(0).id().SerializeAsString());
+    ASSERT_TRUE(attachment_metadata.record(0).is_on_server());
+
+    // See that attachment_metadata and server_attachment_metadata are equal.
+    ASSERT_EQ(attachment_metadata.SerializeAsString(),
+              e.GetServerAttachmentMetadata().SerializeAsString());
+  }
+}
+
 // A test harness for tests that focus on applying updates.
 //
 // Update application is performed when we want to take updates that were
@@ -419,6 +483,7 @@ class DirectoryUpdateHandlerApplyUpdateTest : public ::testing::Test {
         passive_worker_(new FakeModelWorker(GROUP_PASSIVE)),
         bookmarks_emitter_(BOOKMARKS, &type_observers_),
         passwords_emitter_(PASSWORDS, &type_observers_),
+        articles_emitter_(ARTICLES, &type_observers_),
         update_handler_map_deleter_(&update_handler_map_) {}
 
   virtual void SetUp() OVERRIDE {
@@ -435,6 +500,10 @@ class DirectoryUpdateHandlerApplyUpdateTest : public ::testing::Test {
                                    PASSWORDS,
                                    password_worker_,
                                    &passwords_emitter_)));
+    update_handler_map_.insert(std::make_pair(
+        ARTICLES,
+        new DirectoryUpdateHandler(
+            directory(), ARTICLES, ui_worker_, &articles_emitter_)));
   }
 
   virtual void TearDown() OVERRIDE {
@@ -449,6 +518,10 @@ class DirectoryUpdateHandlerApplyUpdateTest : public ::testing::Test {
     return passwords_emitter_.GetUpdateCounters();
   }
 
+  const UpdateCounters& GetArticlesUpdateCounters() {
+    return articles_emitter_.GetUpdateCounters();
+  }
+
  protected:
   void ApplyBookmarkUpdates(sessions::StatusController* status) {
     update_handler_map_[BOOKMARKS]->ApplyUpdates(status);
@@ -458,6 +531,10 @@ class DirectoryUpdateHandlerApplyUpdateTest : public ::testing::Test {
     update_handler_map_[PASSWORDS]->ApplyUpdates(status);
   }
 
+  void ApplyArticlesUpdates(sessions::StatusController* status) {
+    update_handler_map_[ARTICLES]->ApplyUpdates(status);
+  }
+
   TestEntryFactory* entry_factory() {
     return entry_factory_.get();
   }
@@ -480,6 +557,7 @@ class DirectoryUpdateHandlerApplyUpdateTest : public ::testing::Test {
   ObserverList<TypeDebugInfoObserver> type_observers_;
   DirectoryTypeDebugInfoEmitter bookmarks_emitter_;
   DirectoryTypeDebugInfoEmitter passwords_emitter_;
+  DirectoryTypeDebugInfoEmitter articles_emitter_;
 
   UpdateHandlerMap update_handler_map_;
   STLValueDeleter<UpdateHandlerMap> update_handler_map_deleter_;
@@ -1024,4 +1102,91 @@ TEST_F(DirectoryUpdateHandlerApplyUpdateTest, SomeUndecryptablePassword) {
   }
 }
 
+TEST_F(DirectoryUpdateHandlerApplyUpdateTest,
+       SimpleConflictDifferentAttachmentMetadata) {
+  const bool is_folder = false;
+  sync_pb::EntitySpecifics specifics;
+  *specifics.mutable_article() = sync_pb::ArticleSpecifics();
+  int64 handle = entry_factory()->CreateSyncedItem("art1", ARTICLES, is_folder);
+
+  sync_pb::AttachmentIdProto local_attachment_id = CreateAttachmentIdProto();
+  sync_pb::AttachmentIdProto server_attachment_id = CreateAttachmentIdProto();
+
+  // Add an attachment to the local attachment metadata.
+  sync_pb::AttachmentMetadata local_metadata;
+  sync_pb::AttachmentMetadataRecord* local_record = local_metadata.add_record();
+  *local_record->mutable_id() = local_attachment_id;
+  local_record->set_is_on_server(true);
+  entry_factory()->SetLocalAttachmentMetadataForItem(handle, local_metadata);
+
+  // Add a different attachment to the server attachment metadata.
+  sync_pb::AttachmentMetadata server_metadata;
+  sync_pb::AttachmentMetadataRecord* server_record =
+      server_metadata.add_record();
+  *server_record->mutable_id() = server_attachment_id;
+  server_record->set_is_on_server(true);
+  entry_factory()->SetServerAttachmentMetadataForItem(handle, server_metadata);
+
+  // At this point we have a simple conflict.  The server says art1 should have
+  // server_attachment_id, but the local sync engine says it should have
+  // local_attachment_id.
+
+  sessions::StatusController status;
+  ApplyArticlesUpdates(&status);
+
+  // See that the server won.
+  const UpdateCounters& counters = GetArticlesUpdateCounters();
+  EXPECT_EQ(1, counters.num_updates_applied);
+  EXPECT_EQ(1, counters.num_local_overwrites);
+  EXPECT_EQ(0, counters.num_server_overwrites);
+  local_metadata = entry_factory()->GetLocalAttachmentMetadataForItem(handle);
+  EXPECT_EQ(server_metadata.SerializeAsString(),
+            local_metadata.SerializeAsString());
+}
+
+TEST_F(DirectoryUpdateHandlerApplyUpdateTest,
+       SimpleConflictSameAttachmentMetadataDifferentOrder) {
+  const bool is_folder = false;
+  sync_pb::EntitySpecifics specifics;
+  *specifics.mutable_article() = sync_pb::ArticleSpecifics();
+  int64 handle = entry_factory()->CreateSyncedItem("art1", ARTICLES, is_folder);
+
+  sync_pb::AttachmentIdProto id1 = CreateAttachmentIdProto();
+  sync_pb::AttachmentIdProto id2 = CreateAttachmentIdProto();
+
+  // Add id1, then id2 to the local attachment metadata.
+  sync_pb::AttachmentMetadata local_metadata;
+  sync_pb::AttachmentMetadataRecord* record = local_metadata.add_record();
+  *record->mutable_id() = id1;
+  record->set_is_on_server(true);
+  record = local_metadata.add_record();
+  *record->mutable_id() = id2;
+  record->set_is_on_server(true);
+  entry_factory()->SetLocalAttachmentMetadataForItem(handle, local_metadata);
+
+  // Add id1 and id2 to the server attachment metadata, but in reverse order.
+  sync_pb::AttachmentMetadata server_metadata;
+  record = server_metadata.add_record();
+  *record->mutable_id() = id2;
+  record->set_is_on_server(true);
+  record = local_metadata.add_record();
+  *record->mutable_id() = id1;
+  record->set_is_on_server(true);
+  entry_factory()->SetServerAttachmentMetadataForItem(handle, server_metadata);
+
+  // At this point we have a (false) conflict.
+
+  sessions::StatusController status;
+  ApplyArticlesUpdates(&status);
+
+  // See that the server won.
+  const UpdateCounters& counters = GetArticlesUpdateCounters();
+  EXPECT_EQ(1, counters.num_updates_applied);
+  EXPECT_EQ(1, counters.num_local_overwrites);
+  EXPECT_EQ(0, counters.num_server_overwrites);
+  local_metadata = entry_factory()->GetLocalAttachmentMetadataForItem(handle);
+  EXPECT_EQ(server_metadata.SerializeAsString(),
+            local_metadata.SerializeAsString());
+}
+
 }  // namespace syncer
diff --git a/sync/engine/get_commit_ids.cc b/sync/engine/get_commit_ids.cc
index 5f91915..5718cec 100644
--- a/sync/engine/get_commit_ids.cc
+++ b/sync/engine/get_commit_ids.cc
@@ -108,7 +108,6 @@ bool IsEntryInConflict(const syncable::Entry& entry) {
 // Return true if this entry has any attachments that haven't yet been uploaded
 // to the server.
 bool HasAttachmentNotOnServer(const syncable::Entry& entry) {
-  // TODO(maniscalco): Add test case (bug 356266).
   const sync_pb::AttachmentMetadata& metadata = entry.GetAttachmentMetadata();
   for (int i = 0; i < metadata.record_size(); ++i) {
     if (!metadata.record(i).is_on_server()) {
diff --git a/sync/engine/syncer_util.cc b/sync/engine/syncer_util.cc
index c7d93c1..3cb9bca 100644
--- a/sync/engine/syncer_util.cc
+++ b/sync/engine/syncer_util.cc
@@ -17,6 +17,7 @@
 #include "sync/engine/conflict_resolver.h"
 #include "sync/engine/syncer_proto_util.h"
 #include "sync/engine/syncer_types.h"
+#include "sync/internal_api/public/base/attachment_id_proto.h"
 #include "sync/internal_api/public/base/model_type.h"
 #include "sync/internal_api/public/base/unique_position.h"
 #include "sync/protocol/bookmark_specifics.pb.h"
@@ -407,6 +408,8 @@ void UpdateServerFieldsFromUpdate(
                             bookmark.bookmark_favicon(),
                             target);
   }
+  target->PutServerAttachmentMetadata(
+      CreateAttachmentMetadata(update.attachment_id()));
   if (SyncerProtoUtil::ShouldMaintainPosition(update)) {
     UpdateBookmarkPositioning(update, target);
   }
@@ -463,6 +466,7 @@ void UpdateLocalDataFromServerData(
   entry->PutBaseVersion(entry->GetServerVersion());
   entry->PutIsDel(entry->GetServerIsDel());
   entry->PutIsUnappliedUpdate(false);
+  entry->PutAttachmentMetadata(entry->GetServerAttachmentMetadata());
 }
 
 VerifyCommitResult ValidateCommitEntry(syncable::Entry* entry) {
diff --git a/sync/internal_api/public/base/attachment_id_proto.cc b/sync/internal_api/public/base/attachment_id_proto.cc
index 99480d7..eeaa427 100644
--- a/sync/internal_api/public/base/attachment_id_proto.cc
+++ b/sync/internal_api/public/base/attachment_id_proto.cc
@@ -20,4 +20,15 @@ sync_pb::AttachmentIdProto CreateAttachmentIdProto() {
   return proto;
 }
 
+sync_pb::AttachmentMetadata CreateAttachmentMetadata(
+    const google::protobuf::RepeatedPtrField<sync_pb::AttachmentIdProto>& ids) {
+  sync_pb::AttachmentMetadata result;
+  for (int i = 0; i < ids.size(); ++i) {
+    sync_pb::AttachmentMetadataRecord* record = result.add_record();
+    *record->mutable_id() = ids.Get(i);
+    record->set_is_on_server(true);
+  }
+  return result;
+}
+
 }  // namespace syncer
diff --git a/sync/internal_api/public/base/attachment_id_proto.h b/sync/internal_api/public/base/attachment_id_proto.h
index 3332d81..a47f46e 100644
--- a/sync/internal_api/public/base/attachment_id_proto.h
+++ b/sync/internal_api/public/base/attachment_id_proto.h
@@ -15,6 +15,14 @@ namespace syncer {
 // Creates a unique AttachmentIdProto.
 SYNC_EXPORT_PRIVATE sync_pb::AttachmentIdProto CreateAttachmentIdProto();
 
+// Creates an AttachmentMetadata object from a repeated field of
+// AttachmentIdProto objects.
+//
+// Note: each record in the AttachmentMetadata will be marked as "on the
+// server".
+SYNC_EXPORT_PRIVATE sync_pb::AttachmentMetadata CreateAttachmentMetadata(
+    const google::protobuf::RepeatedPtrField<sync_pb::AttachmentIdProto>& ids);
+
 }  // namespace syncer
 
 #endif  // SYNC_INTERNAL_API_PUBLIC_BASE_ATTACHMENT_ID_PROTO_H_
diff --git a/sync/internal_api/public/base/attachment_id_proto_unittest.cc b/sync/internal_api/public/base/attachment_id_proto_unittest.cc
index 8883a45..6f029ad 100644
--- a/sync/internal_api/public/base/attachment_id_proto_unittest.cc
+++ b/sync/internal_api/public/base/attachment_id_proto_unittest.cc
@@ -26,4 +26,24 @@ TEST(AttachmentIdProtoTest, UniqueIdFormat) {
   EXPECT_EQ(StringToLowerASCII(id_proto.unique_id()), id_proto.unique_id());
 }
 
+TEST(AttachmentIdProtoTest, CreateAttachmentMetadata_Empty) {
+  google::protobuf::RepeatedPtrField<sync_pb::AttachmentIdProto> ids;
+  sync_pb::AttachmentMetadata metadata = CreateAttachmentMetadata(ids);
+  EXPECT_EQ(0, metadata.record_size());
+}
+
+TEST(AttachmentIdProtoTest, CreateAttachmentMetadata_NonEmpty) {
+  google::protobuf::RepeatedPtrField<sync_pb::AttachmentIdProto> ids;
+  *ids.Add() = CreateAttachmentIdProto();
+  *ids.Add() = CreateAttachmentIdProto();
+  *ids.Add() = CreateAttachmentIdProto();
+  sync_pb::AttachmentMetadata metadata = CreateAttachmentMetadata(ids);
+  ASSERT_EQ(3, metadata.record_size());
+  for (int i = 0; i < metadata.record_size(); ++i) {
+    EXPECT_EQ(ids.Get(i).SerializeAsString(),
+              metadata.record(i).id().SerializeAsString());
+    EXPECT_TRUE(metadata.record(i).is_on_server());
+  }
+}
+
 }  // namespace syncer
diff --git a/sync/internal_api/public/test/test_entry_factory.h b/sync/internal_api/public/test/test_entry_factory.h
index 7ca79e6..97ff25e 100644
--- a/sync/internal_api/public/test/test_entry_factory.h
+++ b/sync/internal_api/public/test/test_entry_factory.h
@@ -61,7 +61,7 @@ class TestEntryFactory {
   int64 CreateSyncedItem(const std::string& name,
                          ModelType model_type, bool is_folder);
 
-  // Creates a root node that IS_UNAPPLIED. Smiilar to what one would find in
+  // Creates a root node that IS_UNAPPLIED. Similar to what one would find in
   // the database between the ProcessUpdates of an initial datatype configure
   // cycle and the ApplyUpdates step of the same sync cycle.
   int64 CreateUnappliedRootNode(ModelType model_type);
@@ -80,16 +80,41 @@ class TestEntryFactory {
   bool SetLocalSpecificsForItem(int64 meta_handle,
                                 const sync_pb::EntitySpecifics specifics);
 
-  // Looks up the item referenced by |meta_handle|. If successful, stores
-  // the server specifics into |specifics| and returns true. Else, return false.
+  // Looks up the item referenced by |meta_handle| and returns its server
+  // specifics.
   const sync_pb::EntitySpecifics& GetServerSpecificsForItem(
       int64 meta_handle) const;
 
-  // Looks up the item referenced by |meta_handle|. If successful, stores
-  // the local specifics into |specifics| and returns true. Else, return false.
+  // Looks up the item referenced by |meta_handle| and returns its specifics.
   const sync_pb::EntitySpecifics& GetLocalSpecificsForItem(
       int64 meta_handle) const;
 
+  // Looks up the item referenced by |meta_handle|. If successful, overwrites
+  // the server attachment metadata with |metadata|, sets
+  // IS_UNAPPLIED_UPDATES/IS_UNSYNCED appropriately, and returns true.
+  // Else, return false.
+  bool SetServerAttachmentMetadataForItem(
+      int64 meta_handle,
+      const sync_pb::AttachmentMetadata metadata);
+
+  // Looks up the item referenced by |meta_handle|. If successful, overwrites
+  // the local attachment metadata with |metadata|, sets
+  // IS_UNAPPLIED_UPDATES/IS_UNSYNCED appropriately, and returns true.
+  // Else, return false.
+  bool SetLocalAttachmentMetadataForItem(
+      int64 meta_handle,
+      const sync_pb::AttachmentMetadata metadata);
+
+  // Looks up the item referenced by |meta_handle| and returns its server
+  // attachment metadata.
+  const sync_pb::AttachmentMetadata& GetServerAttachmentMetadataForItem(
+      int64 meta_handle) const;
+
+  // Looks up the item referenced by |meta_handle| and returns its attachment
+  // metadata.
+  const sync_pb::AttachmentMetadata& GetLocalAttachmentMetadataForItem(
+      int64 meta_handle) const;
+
   // Getters for IS_UNSYNCED and IS_UNAPPLIED_UPDATE bit fields.
   bool GetIsUnsyncedForItem(int64 meta_handle) const;
   bool GetIsUnappliedForItem(int64 meta_handle) const;
diff --git a/sync/internal_api/sync_manager_impl.cc b/sync/internal_api/sync_manager_impl.cc
index 256a5c9..a64c941 100644
--- a/sync/internal_api/sync_manager_impl.cc
+++ b/sync/internal_api/sync_manager_impl.cc
@@ -226,6 +226,10 @@ bool SyncManagerImpl::VisiblePropertiesDiffer(
                          b.ref(syncable::SPECIFICS))) {
     return true;
   }
+  if (!AreAttachmentMetadataEqual(a.ref(syncable::ATTACHMENT_METADATA),
+                                  b.ref(syncable::ATTACHMENT_METADATA))) {
+    return true;
+  }
   // We only care if the name has changed if neither specifics is encrypted
   // (encrypted nodes blow away the NON_UNIQUE_NAME).
   if (!a_specifics.has_encrypted() && !b_specifics.has_encrypted() &&
diff --git a/sync/internal_api/sync_manager_impl_unittest.cc b/sync/internal_api/sync_manager_impl_unittest.cc
index d2b1dec..eeba3bd 100644
--- a/sync/internal_api/sync_manager_impl_unittest.cc
+++ b/sync/internal_api/sync_manager_impl_unittest.cc
@@ -24,6 +24,7 @@
 #include "base/values.h"
 #include "google_apis/gaia/gaia_constants.h"
 #include "sync/engine/sync_scheduler.h"
+#include "sync/internal_api/public/base/attachment_id_proto.h"
 #include "sync/internal_api/public/base/cancelation_signal.h"
 #include "sync/internal_api/public/base/model_type_test_util.h"
 #include "sync/internal_api/public/change_record.h"
@@ -866,6 +867,7 @@ class SyncManagerTest : public testing::Test,
     (*out)[PASSWORDS] = GROUP_PASSIVE;
     (*out)[PREFERENCES] = GROUP_PASSIVE;
     (*out)[PRIORITY_PREFERENCES] = GROUP_PASSIVE;
+    (*out)[ARTICLES] = GROUP_PASSIVE;
   }
 
   ModelTypeSet GetEnabledTypes() {
@@ -2872,6 +2874,8 @@ class SyncManagerChangeProcessingTest : public SyncManagerTest {
     return last_changes_.Get().size();
   }
 
+  void ClearChangeList() { last_changes_ = ImmutableChangeRecordList(); }
+
  protected:
   ImmutableChangeRecordList last_changes_;
   TestIdFactory id_factory_;
@@ -3107,6 +3111,66 @@ TEST_F(SyncManagerChangeProcessingTest, DeletionsAndChanges) {
   EXPECT_LT(folder_b_pos, folder_a_pos);
 }
 
+// See that attachment metadata changes are not filtered out by
+// SyncManagerImpl::VisiblePropertiesDiffer.
+TEST_F(SyncManagerChangeProcessingTest, AttachmentMetadataOnlyChanges) {
+  // Create an article with no attachments.  See that a change is generated.
+  int64 article_id = kInvalidId;
+  {
+    syncable::WriteTransaction trans(
+        FROM_HERE, syncable::SYNCER, share()->directory.get());
+    int64 type_root = GetIdForDataType(ARTICLES);
+    syncable::Entry root(&trans, syncable::GET_BY_HANDLE, type_root);
+    ASSERT_TRUE(root.good());
+    syncable::MutableEntry article(
+        &trans, syncable::CREATE, ARTICLES, root.GetId(), "article");
+    ASSERT_TRUE(article.good());
+    SetNodeProperties(&article);
+    article_id = article.GetMetahandle();
+  }
+  ASSERT_EQ(1UL, GetChangeListSize());
+  FindChangeInList(article_id, ChangeRecord::ACTION_ADD);
+  ClearChangeList();
+
+  // Modify the article by adding one attachment.  Don't touch anything else.
+  // See that a change is generated.
+  {
+    syncable::WriteTransaction trans(
+        FROM_HERE, syncable::SYNCER, share()->directory.get());
+    syncable::MutableEntry article(&trans, syncable::GET_BY_HANDLE, article_id);
+    sync_pb::AttachmentMetadata metadata;
+    *metadata.add_record()->mutable_id() = CreateAttachmentIdProto();
+    article.PutAttachmentMetadata(metadata);
+  }
+  ASSERT_EQ(1UL, GetChangeListSize());
+  FindChangeInList(article_id, ChangeRecord::ACTION_UPDATE);
+  ClearChangeList();
+
+  // Modify the article by replacing its attachment with a different one.  See
+  // that a change is generated.
+  {
+    syncable::WriteTransaction trans(
+        FROM_HERE, syncable::SYNCER, share()->directory.get());
+    syncable::MutableEntry article(&trans, syncable::GET_BY_HANDLE, article_id);
+    sync_pb::AttachmentMetadata metadata = article.GetAttachmentMetadata();
+    *metadata.add_record()->mutable_id() = CreateAttachmentIdProto();
+    article.PutAttachmentMetadata(metadata);
+  }
+  ASSERT_EQ(1UL, GetChangeListSize());
+  FindChangeInList(article_id, ChangeRecord::ACTION_UPDATE);
+  ClearChangeList();
+
+  // Modify the article by replacing its attachment metadata with the same
+  // attachment metadata.  No change should be generated.
+  {
+    syncable::WriteTransaction trans(
+        FROM_HERE, syncable::SYNCER, share()->directory.get());
+    syncable::MutableEntry article(&trans, syncable::GET_BY_HANDLE, article_id);
+    article.PutAttachmentMetadata(article.GetAttachmentMetadata());
+  }
+  ASSERT_EQ(0UL, GetChangeListSize());
+}
+
 // During initialization SyncManagerImpl loads sqlite database. If it fails to
 // do so it should fail initialization. This test verifies this behavior.
 // Test reuses SyncManagerImpl initialization from SyncManagerTest but overrides
diff --git a/sync/internal_api/syncapi_internal.cc b/sync/internal_api/syncapi_internal.cc
index d297fcb..91a6d79 100644
--- a/sync/internal_api/syncapi_internal.cc
+++ b/sync/internal_api/syncapi_internal.cc
@@ -5,6 +5,7 @@
 #include "sync/internal_api/syncapi_internal.h"
 
 #include "base/memory/scoped_ptr.h"
+#include "sync/protocol/attachments.pb.h"
 #include "sync/protocol/password_specifics.pb.h"
 #include "sync/protocol/sync.pb.h"
 #include "sync/util/cryptographer.h"
@@ -104,4 +105,12 @@ bool AreSpecificsEqual(const Cryptographer* cryptographer,
   return false;
 }
 
+bool AreAttachmentMetadataEqual(const sync_pb::AttachmentMetadata& left,
+                                const sync_pb::AttachmentMetadata& right) {
+  if (left.SerializeAsString() == right.SerializeAsString()) {
+    return true;
+  }
+  return false;
+}
+
 }  // namespace syncer
diff --git a/sync/internal_api/syncapi_internal.h b/sync/internal_api/syncapi_internal.h
index 2e7965d..acb1631 100644
--- a/sync/internal_api/syncapi_internal.h
+++ b/sync/internal_api/syncapi_internal.h
@@ -13,6 +13,7 @@
 #include "sync/base/sync_export.h"
 
 namespace sync_pb {
+class AttachmentMetadata;
 class EntitySpecifics;
 class PasswordSpecificsData;
 }
@@ -35,6 +36,11 @@ bool IsNameServerIllegalAfterTrimming(const std::string& name);
 bool AreSpecificsEqual(const Cryptographer* cryptographer,
                        const sync_pb::EntitySpecifics& left,
                        const sync_pb::EntitySpecifics& right);
+
+// Return true iff |left| and |right| are equal.
+bool AreAttachmentMetadataEqual(const sync_pb::AttachmentMetadata& left,
+                                const sync_pb::AttachmentMetadata& right);
+
 }  // namespace syncer
 
 #endif  // SYNC_INTERNAL_API_SYNCAPI_INTERNAL_H_
diff --git a/sync/internal_api/test/test_entry_factory.cc b/sync/internal_api/test/test_entry_factory.cc
index ad0fa64..d2e6b73 100644
--- a/sync/internal_api/test/test_entry_factory.cc
+++ b/sync/internal_api/test/test_entry_factory.cc
@@ -239,6 +239,49 @@ const sync_pb::EntitySpecifics& TestEntryFactory::GetLocalSpecificsForItem(
   return entry.GetSpecifics();
 }
 
+bool TestEntryFactory::SetServerAttachmentMetadataForItem(
+    int64 meta_handle,
+    const sync_pb::AttachmentMetadata metadata) {
+  WriteTransaction trans(FROM_HERE, UNITTEST, directory_);
+  MutableEntry entry(&trans, syncable::GET_BY_HANDLE, meta_handle);
+  if (!entry.good()) {
+    return false;
+  }
+  entry.PutServerAttachmentMetadata(metadata);
+  entry.PutIsUnappliedUpdate(true);
+  return true;
+
+}
+
+bool TestEntryFactory::SetLocalAttachmentMetadataForItem(
+    int64 meta_handle,
+    const sync_pb::AttachmentMetadata metadata) {
+  WriteTransaction trans(FROM_HERE, UNITTEST, directory_);
+  MutableEntry entry(&trans, syncable::GET_BY_HANDLE, meta_handle);
+  if (!entry.good()) {
+    return false;
+  }
+  entry.PutAttachmentMetadata(metadata);
+  entry.PutIsUnsynced(true);
+  return true;
+}
+
+const sync_pb::AttachmentMetadata&
+TestEntryFactory::GetServerAttachmentMetadataForItem(int64 meta_handle) const {
+  syncable::ReadTransaction trans(FROM_HERE, directory_);
+  syncable::Entry entry(&trans, syncable::GET_BY_HANDLE, meta_handle);
+  DCHECK(entry.good());
+  return entry.GetServerAttachmentMetadata();
+}
+
+const sync_pb::AttachmentMetadata&
+TestEntryFactory::GetLocalAttachmentMetadataForItem(int64 meta_handle) const {
+  syncable::ReadTransaction trans(FROM_HERE, directory_);
+  syncable::Entry entry(&trans, syncable::GET_BY_HANDLE, meta_handle);
+  DCHECK(entry.good());
+  return entry.GetAttachmentMetadata();
+}
+
 bool TestEntryFactory::GetIsUnsyncedForItem(int64 meta_handle) const {
   syncable::ReadTransaction trans(FROM_HERE, directory_);
   syncable::Entry entry(&trans, syncable::GET_BY_HANDLE, meta_handle);
diff --git a/sync/protocol/proto_value_conversions.cc b/sync/protocol/proto_value_conversions.cc
index de9aa45..740eae4 100644
--- a/sync/protocol/proto_value_conversions.cc
+++ b/sync/protocol/proto_value_conversions.cc
@@ -850,6 +850,7 @@ base::DictionaryValue* SyncEntityToValue(const sync_pb::SyncEntity& proto,
     SET(specifics, EntitySpecificsToValue);
   SET_BOOL(folder);
   SET_STR(client_defined_unique_tag);
+  SET_REP(attachment_id, AttachmentIdProtoToValue);
   return value;
 }
 
diff --git a/sync/syncable/model_neutral_mutable_entry.cc b/sync/syncable/model_neutral_mutable_entry.cc
index 2ae5450..c7f1e3d6 100644
--- a/sync/syncable/model_neutral_mutable_entry.cc
+++ b/sync/syncable/model_neutral_mutable_entry.cc
@@ -353,6 +353,18 @@ void ModelNeutralMutableEntry::PutServerUniquePosition(
   }
 }
 
+void ModelNeutralMutableEntry::PutServerAttachmentMetadata(
+    const sync_pb::AttachmentMetadata& value) {
+  DCHECK(kernel_);
+  base_write_transaction_->TrackChangesTo(kernel_);
+
+  if (kernel_->ref(SERVER_ATTACHMENT_METADATA).SerializeAsString() !=
+      value.SerializeAsString()) {
+    kernel_->put(SERVER_ATTACHMENT_METADATA, value);
+    kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
+  }
+}
+
 void ModelNeutralMutableEntry::PutSyncing(bool value) {
   kernel_->put(SYNCING, value);
 }
diff --git a/sync/syncable/model_neutral_mutable_entry.h b/sync/syncable/model_neutral_mutable_entry.h
index 4010dff..c1326b2 100644
--- a/sync/syncable/model_neutral_mutable_entry.h
+++ b/sync/syncable/model_neutral_mutable_entry.h
@@ -71,6 +71,7 @@ class SYNC_EXPORT_PRIVATE ModelNeutralMutableEntry : public Entry {
   void PutServerSpecifics(const sync_pb::EntitySpecifics& value);
   void PutBaseServerSpecifics(const sync_pb::EntitySpecifics& value);
   void PutServerUniquePosition(const UniquePosition& value);
+  void PutServerAttachmentMetadata(const sync_pb::AttachmentMetadata& value);
   void PutSyncing(bool value);
 
   // Do a simple property-only update of the PARENT_ID field.  Use with caution.