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

6 files changed, 324 insertions, 5 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) {