Do not update AttachmentIds after uploading attachments to sync server.

In a previous design iteration we planned to have the sync server assign
an attachments id after receiving an attachment upload. We have since
changed the design so the client is responsible for assigning the
attachment id before upload occurs.

Remove comments related to updating attachment id after upload.

Rename UpdateAttachmentIdWithServerInfo to MarkAttachmentAsOnServer.

Rename UpdateEntriesWithAttachmentId to
UpdateEntriesMarkAttachmentAsOnServer.

BUG=371522

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

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

9 files changed, 34 insertions, 45 deletions

diff --git a/sync/api/attachments/attachment_uploader.h b/sync/api/attachments/attachment_uploader.h
index 12fe1a3..4774451 100644
--- a/sync/api/attachments/attachment_uploader.h
+++ b/sync/api/attachments/attachment_uploader.h
@@ -34,8 +34,8 @@ class SYNC_EXPORT AttachmentUploader {
   // |callback| will be invoked when the operation has completed (successfully
   // or otherwise).
   //
-  // |callback| will receive an UploadResult code and an updated AttachmentId
-  // containing the server address of the newly uploaded attachment.
+  // |callback| will receive an UploadResult code and the AttachmentId of the
+  // newly uploaded attachment.
   virtual void UploadAttachment(const Attachment& attachment,
                                 const UploadCallback& callback) = 0;
 };
diff --git a/sync/internal_api/attachments/attachment_uploader_impl.cc b/sync/internal_api/attachments/attachment_uploader_impl.cc
index 7e12350..c2bda36f 100644
--- a/sync/internal_api/attachments/attachment_uploader_impl.cc
+++ b/sync/internal_api/attachments/attachment_uploader_impl.cc
@@ -137,8 +137,6 @@ void AttachmentUploaderImpl::UploadState::OnURLFetchComplete(
   AttachmentId attachment_id = attachment_.GetId();
   if (source->GetResponseCode() == net::HTTP_OK) {
     result = UPLOAD_SUCCESS;
-    // TODO(maniscalco): Update the attachment id with server address
-    // information before passing it to the callback (bug 371522).
   } else if (source->GetResponseCode() == net::HTTP_UNAUTHORIZED) {
     // TODO(maniscalco): One possibility is that we received a 401 because our
     // access token has expired.  We should probably fetch a new access token
diff --git a/sync/internal_api/attachments/attachment_uploader_impl_unittest.cc b/sync/internal_api/attachments/attachment_uploader_impl_unittest.cc
index 90cb07d..14fcfc7 100644
--- a/sync/internal_api/attachments/attachment_uploader_impl_unittest.cc
+++ b/sync/internal_api/attachments/attachment_uploader_impl_unittest.cc
@@ -187,7 +187,7 @@ class AttachmentUploaderImplTest : public testing::Test,
   const AttachmentUploader::UploadCallback& upload_callback() const;
   std::vector<HttpRequest>& http_requests_received();
   std::vector<AttachmentUploader::UploadResult>& upload_results();
-  std::vector<AttachmentId>& updated_attachment_ids();
+  std::vector<AttachmentId>& attachment_ids();
   MockOAuth2TokenService& token_service();
   base::MessageLoopForIO& message_loop();
   RequestHandler& request_handler();
@@ -195,7 +195,7 @@ class AttachmentUploaderImplTest : public testing::Test,
  private:
   // An UploadCallback invoked by AttachmentUploaderImpl.
   void UploadDone(const AttachmentUploader::UploadResult& result,
-                  const AttachmentId& updated_attachment_id);
+                  const AttachmentId& attachment_id);
 
   base::MessageLoopForIO message_loop_;
   scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
@@ -207,7 +207,7 @@ class AttachmentUploaderImplTest : public testing::Test,
   base::Closure signal_upload_done_;
   std::vector<HttpRequest> http_requests_received_;
   std::vector<AttachmentUploader::UploadResult> upload_results_;
-  std::vector<AttachmentId> updated_attachment_ids_;
+  std::vector<AttachmentId> attachment_ids_;
   scoped_ptr<MockOAuth2TokenService> token_service_;
 
   // Must be last data member.
@@ -315,8 +315,8 @@ AttachmentUploaderImplTest::upload_results() {
 }
 
 std::vector<AttachmentId>&
-AttachmentUploaderImplTest::updated_attachment_ids() {
-  return updated_attachment_ids_;
+AttachmentUploaderImplTest::attachment_ids() {
+  return attachment_ids_;
 }
 
 MockOAuth2TokenService& AttachmentUploaderImplTest::token_service() {
@@ -333,10 +333,10 @@ RequestHandler& AttachmentUploaderImplTest::request_handler() {
 
 void AttachmentUploaderImplTest::UploadDone(
     const AttachmentUploader::UploadResult& result,
-    const AttachmentId& updated_attachment_id) {
+    const AttachmentId& attachment_id) {
   DCHECK(CalledOnValidThread());
   upload_results_.push_back(result);
-  updated_attachment_ids_.push_back(updated_attachment_id);
+  attachment_ids_.push_back(attachment_id);
   DCHECK(!signal_upload_done_.is_null());
   signal_upload_done_.Run();
 }
@@ -432,8 +432,8 @@ TEST_F(AttachmentUploaderImplTest, UploadAttachment_HappyCase) {
   // See that the done callback was invoked with the right arguments.
   ASSERT_EQ(1U, upload_results().size());
   EXPECT_EQ(AttachmentUploader::UPLOAD_SUCCESS, upload_results()[0]);
-  ASSERT_EQ(1U, updated_attachment_ids().size());
-  EXPECT_EQ(attachment.GetId(), updated_attachment_ids()[0]);
+  ASSERT_EQ(1U, attachment_ids().size());
+  EXPECT_EQ(attachment.GetId(), attachment_ids()[0]);
 
   // See that the HTTP server received one request.
   ASSERT_EQ(1U, http_requests_received().size());
@@ -448,10 +448,6 @@ TEST_F(AttachmentUploaderImplTest, UploadAttachment_HappyCase) {
   const std::string header_value(std::string("Bearer ") + kAccessToken);
   EXPECT_THAT(http_request.headers,
               testing::Contains(testing::Pair(header_name, header_value)));
-
-  // TODO(maniscalco): Once AttachmentUploaderImpl is capable of updating the
-  // AttachmentId with server address information about the attachment, add some
-  // checks here to verify it works properly (bug 371522).
 }
 
 // Verify two overlapping calls to upload the same attachment result in only one
@@ -513,8 +509,8 @@ TEST_F(AttachmentUploaderImplTest, UploadAttachment_FailToGetToken) {
   // See that the done callback was invoked.
   ASSERT_EQ(1U, upload_results().size());
   EXPECT_EQ(AttachmentUploader::UPLOAD_UNSPECIFIED_ERROR, upload_results()[0]);
-  ASSERT_EQ(1U, updated_attachment_ids().size());
-  EXPECT_EQ(attachment.GetId(), updated_attachment_ids()[0]);
+  ASSERT_EQ(1U, attachment_ids().size());
+  EXPECT_EQ(attachment.GetId(), attachment_ids()[0]);
 
   // See that no HTTP request was received.
   ASSERT_EQ(0U, http_requests_received().size());
@@ -535,8 +531,8 @@ TEST_F(AttachmentUploaderImplTest, UploadAttachment_ServiceUnavilable) {
   // See that the done callback was invoked.
   ASSERT_EQ(1U, upload_results().size());
   EXPECT_EQ(AttachmentUploader::UPLOAD_UNSPECIFIED_ERROR, upload_results()[0]);
-  ASSERT_EQ(1U, updated_attachment_ids().size());
-  EXPECT_EQ(attachment.GetId(), updated_attachment_ids()[0]);
+  ASSERT_EQ(1U, attachment_ids().size());
+  EXPECT_EQ(attachment.GetId(), attachment_ids()[0]);
 
   // See that the HTTP server received one request.
   ASSERT_EQ(1U, http_requests_received().size());
@@ -573,8 +569,8 @@ TEST_F(AttachmentUploaderImplTest, UploadAttachment_BadToken) {
   // See that the done callback was invoked.
   ASSERT_EQ(1U, upload_results().size());
   EXPECT_EQ(AttachmentUploader::UPLOAD_UNSPECIFIED_ERROR, upload_results()[0]);
-  ASSERT_EQ(1U, updated_attachment_ids().size());
-  EXPECT_EQ(attachment.GetId(), updated_attachment_ids()[0]);
+  ASSERT_EQ(1U, attachment_ids().size());
+  EXPECT_EQ(attachment.GetId(), attachment_ids()[0]);
 
   // See that the HTTP server received one request.
   ASSERT_EQ(1U, http_requests_received().size());
diff --git a/sync/internal_api/attachments/fake_attachment_uploader.cc b/sync/internal_api/attachments/fake_attachment_uploader.cc
index 743b534..1ffbe5b 100644
--- a/sync/internal_api/attachments/fake_attachment_uploader.cc
+++ b/sync/internal_api/attachments/fake_attachment_uploader.cc
@@ -25,11 +25,9 @@ void FakeAttachmentUploader::UploadAttachment(const Attachment& attachment,
   DCHECK(!attachment.GetId().GetProto().unique_id().empty());
 
   UploadResult result = UPLOAD_SUCCESS;
-  AttachmentId updated_id = attachment.GetId();
-  // TODO(maniscalco): Update the attachment id with server address information
-  // before passing it to the callback.
-  base::MessageLoop::current()->PostTask(
-      FROM_HERE, base::Bind(callback, result, updated_id));
+  AttachmentId id = attachment.GetId();
+  base::MessageLoop::current()->PostTask(FROM_HERE,
+                                         base::Bind(callback, result, id));
 }
 
 }  // namespace syncer
diff --git a/sync/internal_api/public/write_transaction.h b/sync/internal_api/public/write_transaction.h
index 06d1354..2dbcdaa 100644
--- a/sync/internal_api/public/write_transaction.h
+++ b/sync/internal_api/public/write_transaction.h
@@ -52,9 +52,9 @@ class SYNC_EXPORT WriteTransaction : public BaseTransaction {
       syncer::SyncChangeProcessor::ContextRefreshStatus refresh_status,
       const std::string& context);
 
-  // Attachment id was just updated. Propagate this change to all entries that
-  // refer this attachment id and set is_on_server for corresponding records.
-  void UpdateEntriesWithAttachmentId(const AttachmentId& attachment_id);
+  // Update all entries that refer to |attachment_id| indicating that
+  // |attachment_id| has been uploaded to the sync server.
+  void UpdateEntriesMarkAttachmentAsOnServer(const AttachmentId& attachment_id);
 
  protected:
   WriteTransaction() {}
diff --git a/sync/internal_api/write_transaction.cc b/sync/internal_api/write_transaction.cc
index 70e9029..5415564 100644
--- a/sync/internal_api/write_transaction.cc
+++ b/sync/internal_api/write_transaction.cc
@@ -81,7 +81,7 @@ void WriteTransaction::SetDataTypeContext(
   // See crbug.com/360280
 }
 
-void WriteTransaction::UpdateEntriesWithAttachmentId(
+void WriteTransaction::UpdateEntriesMarkAttachmentAsOnServer(
     const AttachmentId& attachment_id) {
   syncable::Directory::Metahandles handles;
   GetDirectory()->GetMetahandlesByAttachmentId(
@@ -90,7 +90,7 @@ void WriteTransaction::UpdateEntriesWithAttachmentId(
        iter != handles.end();
        ++iter) {
     syncable::MutableEntry entry(transaction_, syncable::GET_BY_HANDLE, *iter);
-    entry.UpdateAttachmentIdWithServerInfo(attachment_id.GetProto());
+    entry.MarkAttachmentAsOnServer(attachment_id.GetProto());
   }
 }
 
diff --git a/sync/syncable/directory_unittest.cc b/sync/syncable/directory_unittest.cc
index adc4004..efc97e6 100644
--- a/sync/syncable/directory_unittest.cc
+++ b/sync/syncable/directory_unittest.cc
@@ -1649,8 +1649,7 @@ TEST_F(SyncableDirectoryTest, MutableEntry_UpdateAttachmentId) {
   ASSERT_FALSE(entry_metadata.record(1).is_on_server());
   ASSERT_FALSE(entry.GetIsUnsynced());
 
-  // TODO(pavely): When we add server info to proto, add test for it here.
-  entry.UpdateAttachmentIdWithServerInfo(attachment_id_proto);
+  entry.MarkAttachmentAsOnServer(attachment_id_proto);
 
   ASSERT_TRUE(entry_metadata.record(0).is_on_server());
   ASSERT_FALSE(entry_metadata.record(1).is_on_server());
diff --git a/sync/syncable/mutable_entry.cc b/sync/syncable/mutable_entry.cc
index a506e4f..b1c1ff4 100644
--- a/sync/syncable/mutable_entry.cc
+++ b/sync/syncable/mutable_entry.cc
@@ -246,19 +246,18 @@ void MutableEntry::PutAttachmentMetadata(
   }
 }
 
-void MutableEntry::UpdateAttachmentIdWithServerInfo(
-    const sync_pb::AttachmentIdProto& updated_attachment_id) {
+void MutableEntry::MarkAttachmentAsOnServer(
+    const sync_pb::AttachmentIdProto& attachment_id) {
   DCHECK(kernel_);
-  DCHECK(!updated_attachment_id.unique_id().empty());
+  DCHECK(!attachment_id.unique_id().empty());
   write_transaction()->TrackChangesTo(kernel_);
   sync_pb::AttachmentMetadata& attachment_metadata =
       kernel_->mutable_ref(ATTACHMENT_METADATA);
   for (int i = 0; i < attachment_metadata.record_size(); ++i) {
     sync_pb::AttachmentMetadataRecord* record =
         attachment_metadata.mutable_record(i);
-    if (record->id().unique_id() != updated_attachment_id.unique_id())
+    if (record->id().unique_id() != attachment_id.unique_id())
       continue;
-    *record->mutable_id() = updated_attachment_id;
     record->set_is_on_server(true);
   }
   kernel_->mark_dirty(&dir()->kernel_->dirty_metahandles);
diff --git a/sync/syncable/mutable_entry.h b/sync/syncable/mutable_entry.h
index 1709475..615fe80 100644
--- a/sync/syncable/mutable_entry.h
+++ b/sync/syncable/mutable_entry.h
@@ -62,11 +62,10 @@ class SYNC_EXPORT_PRIVATE MutableEntry : public ModelNeutralMutableEntry {
   void PutAttachmentMetadata(
       const sync_pb::AttachmentMetadata& attachment_metadata);
 
-  // Update attachment metadata, replace all records matching attachment id's
-  // unique id with updated attachment id that contains server info.
-  // Set is_in_server for corresponding records.
-  void UpdateAttachmentIdWithServerInfo(
-      const sync_pb::AttachmentIdProto& updated_attachment_id);
+  // Update attachment metadata for |attachment_id| to indicate that this
+  // attachment has been uploaded to the sync server.
+  void MarkAttachmentAsOnServer(
+      const sync_pb::AttachmentIdProto& attachment_id);
 
  private:
   // Kind of redundant. We should reduce the number of pointers