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

components/sync_driver/generic_change_processor.cc

@@ -214,7 +214,7 @@ syncer::SyncError GenericChangeProcessor::UpdateDataTypeContext(
 void GenericChangeProcessor::OnAttachmentUploaded(
     const syncer::AttachmentId& attachment_id) {
   syncer::WriteTransaction trans(FROM_HERE, share_handle());
-  trans.UpdateEntriesWithAttachmentId(attachment_id);
+  trans.UpdateEntriesMarkAttachmentAsOnServer(attachment_id);
 }
 
 syncer::SyncError GenericChangeProcessor::GetAllSyncDataReturnError(

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;
 };

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

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());

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

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() {}

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());
   }
 }
 

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());

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);

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