Drive API: Simplify lifetime management of child requests in BatchUploadRequest.

google_apis/drive/drive_api_requests.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "google_apis/drive/drive_api_requests.h"
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/json/json_writer.h"
 #include "base/location.h"
@@ skipping 145 matching lines @@
 base::StringPiece TrimTransportPadding(const base::StringPiece& piece) {
   size_t trim_size = 0;
   while (trim_size < piece.size() &&
          (piece[piece.size() - 1 - trim_size] == ' ' ||
           piece[piece.size() - 1 - trim_size] == '\t')) {
     ++trim_size;
   }
   return piece.substr(0, piece.size() - trim_size);
 }
 
+void EmptyClosure(scoped_ptr<BatchableDelegate>) {
+}
+
 }  // namespace
 
 MultipartHttpResponse::MultipartHttpResponse() : code(HTTP_SUCCESS) {
 }
 
 MultipartHttpResponse::~MultipartHttpResponse() {
 }
 
 // The |response| must be multipart/mixed format that contains child HTTP
 // response of drive batch request.
@@ skipping 788 matching lines @@
 
 GetUploadStatusRequest::~GetUploadStatusRequest() {}
 
 void GetUploadStatusRequest::OnRangeRequestComplete(
     const UploadRangeResponse& response,
     scoped_ptr<base::Value> value) {
   DCHECK(CalledOnValidThread());
   ParseFileResourceWithUploadRangeAndRun(callback_, response, value.Pass());
 }
 
-//======================= MultipartUploadNewFileRequest =======================
+//======================= MultipartUploadNewFileDelegate =======================
 
-MultipartUploadNewFileRequest::MultipartUploadNewFileRequest(
+MultipartUploadNewFileDelegate::MultipartUploadNewFileDelegate(
     RequestSender* sender,
     const std::string& title,
     const std::string& parent_resource_id,
     const std::string& content_type,
     int64 content_length,
     const base::Time& modified_date,
     const base::Time& last_viewed_by_me_date,
     const base::FilePath& local_file_path,
     const Properties& properties,
     const DriveApiUrlGenerator& url_generator,
     const FileResourceCallback& callback,
     const ProgressCallback& progress_callback)
     : MultipartUploadRequestBase(
-          sender,
+          sender->blocking_task_runner(),
           CreateMultipartUploadMetadataJson(title,
                                             parent_resource_id,
                                             modified_date,
                                             last_viewed_by_me_date,
                                             properties),
           content_type,
           content_length,
           local_file_path,
           callback,
           progress_callback),
       has_modified_date_(!modified_date.is_null()),
       url_generator_(url_generator) {
 }
 
-MultipartUploadNewFileRequest::~MultipartUploadNewFileRequest() {
+MultipartUploadNewFileDelegate::~MultipartUploadNewFileDelegate() {
 }
 
-GURL MultipartUploadNewFileRequest::GetURL() const {
+GURL MultipartUploadNewFileDelegate::GetURL() const {
   return url_generator_.GetMultipartUploadNewFileUrl(has_modified_date_);
 }
 
-net::URLFetcher::RequestType MultipartUploadNewFileRequest::GetRequestType()
+net::URLFetcher::RequestType MultipartUploadNewFileDelegate::GetRequestType()
     const {
   return net::URLFetcher::POST;
 }
 
-//======================= MultipartUploadExistingFileRequest ===================
+//====================== MultipartUploadExistingFileDelegate ===================
 
-MultipartUploadExistingFileRequest::MultipartUploadExistingFileRequest(
+MultipartUploadExistingFileDelegate::MultipartUploadExistingFileDelegate(
     RequestSender* sender,
     const std::string& title,
     const std::string& resource_id,
     const std::string& parent_resource_id,
     const std::string& content_type,
     int64 content_length,
     const base::Time& modified_date,
     const base::Time& last_viewed_by_me_date,
     const base::FilePath& local_file_path,
     const std::string& etag,
     const Properties& properties,
     const DriveApiUrlGenerator& url_generator,
     const FileResourceCallback& callback,
     const ProgressCallback& progress_callback)
     : MultipartUploadRequestBase(
-          sender,
+          sender->blocking_task_runner(),
           CreateMultipartUploadMetadataJson(title,
                                             parent_resource_id,
                                             modified_date,
                                             last_viewed_by_me_date,
                                             properties),
           content_type,
           content_length,
           local_file_path,
           callback,
           progress_callback),
       resource_id_(resource_id),
       etag_(etag),
       has_modified_date_(!modified_date.is_null()),
       url_generator_(url_generator) {
 }
 
-MultipartUploadExistingFileRequest::~MultipartUploadExistingFileRequest() {
+MultipartUploadExistingFileDelegate::~MultipartUploadExistingFileDelegate() {
 }
 
 std::vector<std::string>
-MultipartUploadExistingFileRequest::GetExtraRequestHeaders() const {
+MultipartUploadExistingFileDelegate::GetExtraRequestHeaders() const {
   std::vector<std::string> headers(
       MultipartUploadRequestBase::GetExtraRequestHeaders());
   headers.push_back(util::GenerateIfMatchHeader(etag_));
   return headers;
 }
 
-GURL MultipartUploadExistingFileRequest::GetURL() const {
+GURL MultipartUploadExistingFileDelegate::GetURL() const {
   return url_generator_.GetMultipartUploadExistingFileUrl(resource_id_,
                                                           has_modified_date_);
 }
 
 net::URLFetcher::RequestType
-MultipartUploadExistingFileRequest::GetRequestType() const {
+MultipartUploadExistingFileDelegate::GetRequestType() const {
   return net::URLFetcher::PUT;
 }
 
 //========================== DownloadFileRequest ==========================
 
 DownloadFileRequest::DownloadFileRequest(
     RequestSender* sender,
     const DriveApiUrlGenerator& url_generator,
     const std::string& resource_id,
     const base::FilePath& output_file_path,
@@ skipping 72 matching lines @@
         list->AppendString("commenter");
         root.Set("additionalRoles", list);
       }
       break;
   }
   root.SetString("value", value_);
   base::JSONWriter::Write(&root, upload_content);
   return true;
 }
 
+//======================= SingleBatchableDelegateRequest =======================
+
+SingleBatchableDelegateRequest::SingleBatchableDelegateRequest(
+    RequestSender* sender,
+    BatchableDelegate* delegate)
+    : UrlFetchRequestBase(sender),
+      delegate_(delegate),
+      weak_ptr_factory_(this) {
+}
+
+SingleBatchableDelegateRequest::~SingleBatchableDelegateRequest() {
+}
+
+GURL SingleBatchableDelegateRequest::GetURL() const {
+  return delegate_->GetURL();
+}
+
+net::URLFetcher::RequestType SingleBatchableDelegateRequest::GetRequestType()
+    const {
+  return delegate_->GetRequestType();
+}
+
+std::vector<std::string>
+SingleBatchableDelegateRequest::GetExtraRequestHeaders() const {
+  return delegate_->GetExtraRequestHeaders();
+}
+
+void SingleBatchableDelegateRequest::Prepare(const PrepareCallback& callback) {
+  delegate_->Prepare(callback);
+}
+
+bool SingleBatchableDelegateRequest::GetContentData(
+    std::string* upload_content_type,
+    std::string* upload_content) {
+  return delegate_->GetContentData(upload_content_type, upload_content);
+}
+
+void SingleBatchableDelegateRequest::ProcessURLFetchResults(
+    const net::URLFetcher* source) {
+  delegate_->NotifyResult(
+      GetErrorCode(), response_writer()->data(),
+      base::Bind(
+          &SingleBatchableDelegateRequest::OnProcessURLFetchResultsComplete,
+          weak_ptr_factory_.GetWeakPtr()));
+}
+
+void SingleBatchableDelegateRequest::RunCallbackOnPrematureFailure(
+    DriveApiErrorCode code) {
+  delegate_->NotifyError(code);
+}
+
+void SingleBatchableDelegateRequest::OnURLFetchUploadProgress(
+    const net::URLFetcher* source,
+    int64 current,
+    int64 total) {
+  delegate_->NotifyUploadProgress(source, current, total);
+}
+
 //========================== BatchUploadRequest ==========================
 
+BatchUploadChildEntry::BatchUploadChildEntry(BatchableDelegate* request)
+    : request(request), prepared(false), data_offset(0), data_size(0) {
+}
+
+BatchUploadChildEntry::~BatchUploadChildEntry() {
+}
+
 BatchUploadRequest::BatchUploadRequest(
     RequestSender* sender,
     const DriveApiUrlGenerator& url_generator)
     : UrlFetchRequestBase(sender),
       sender_(sender),
       url_generator_(url_generator),
       committed_(false),
       last_progress_value_(0),
       weak_ptr_factory_(this) {
 }
 
 BatchUploadRequest::~BatchUploadRequest() {
-  for (const auto& child : child_requests_) {
-    // Request will be deleted in |RequestFinished| method.
-    sender_->RequestFinished(child.request);
-  }
 }
 
 void BatchUploadRequest::SetBoundaryForTesting(const std::string& boundary) {
   boundary_ = boundary;
 }
 
-void BatchUploadRequest::AddRequest(BatchableRequestBase* request) {
+void BatchUploadRequest::AddRequest(BatchableDelegate* request) {
   DCHECK(CalledOnValidThread());
   DCHECK(request);
   DCHECK(GetChildEntry(request) == child_requests_.end());
   DCHECK(!committed_);
-  child_requests_.push_back(BatchUploadChildEntry(request));
+  child_requests_.push_back(new BatchUploadChildEntry(request));
   request->Prepare(base::Bind(&BatchUploadRequest::OnChildRequestPrepared,
                               weak_ptr_factory_.GetWeakPtr(), request));
 }
 
 void BatchUploadRequest::OnChildRequestPrepared(RequestID request_id,
                                                 DriveApiErrorCode result) {
   DCHECK(CalledOnValidThread());
   auto const child = GetChildEntry(request_id);
   DCHECK(child != child_requests_.end());
   if (IsSuccessfulDriveApiErrorCode(result)) {
-    child->prepared = true;
+    (*child)->prepared = true;
   } else {
-    child->request->RunCallbackOnPrematureFailure(result);
-    sender_->RequestFinished(child->request);
+    (*child)->request->NotifyError(result);
     child_requests_.erase(child);
   }
   MayCompletePrepare();
 }
 
 void BatchUploadRequest::Commit() {
   DCHECK(CalledOnValidThread());
   DCHECK(!committed_);
   if (child_requests_.empty()) {
     Cancel();
   } else {
     committed_ = true;
     MayCompletePrepare();
   }
 }
 
 void BatchUploadRequest::Prepare(const PrepareCallback& callback) {
   DCHECK(CalledOnValidThread());
   DCHECK(!callback.is_null());
   prepare_callback_ = callback;
   MayCompletePrepare();
 }
 
 void BatchUploadRequest::Cancel() {
-  for (auto& child : child_requests_) {
-    // Request cancel should delete the request instance.
-    child.request->Cancel();
-  }
   child_requests_.clear();
   UrlFetchRequestBase::Cancel();
 }
 
 // Obtains corresponding child entry of |request_id|. Returns NULL if the
 // entry is not found.
-std::vector<BatchUploadChildEntry>::iterator BatchUploadRequest::GetChildEntry(
+ScopedVector<BatchUploadChildEntry>::iterator BatchUploadRequest::GetChildEntry(
     RequestID request_id) {
   for (auto it = child_requests_.begin(); it != child_requests_.end(); ++it) {
-    if (it->request == request_id)
+    if ((*it)->request.get() == request_id)
       return it;
   }
   return child_requests_.end();
 }
 
 void BatchUploadRequest::MayCompletePrepare() {
   if (!committed_ || prepare_callback_.is_null())
     return;
   for (const auto& child : child_requests_) {
-    if (!child.prepared)
+    if (!child->prepared)
       return;
   }
 
   // Build multipart body here.
   std::vector<ContentTypeAndData> parts;
   for (auto& child : child_requests_) {
     std::string type;
     std::string data;
-    const bool result = child.request->GetContentData(&type, &data);
+    const bool result = child->request->GetContentData(&type, &data);
     // Upload request must have content data.
     DCHECK(result);
 
-    const GURL url = child.request->GetURL();
+    const GURL url = child->request->GetURL();
     std::string method;
-    switch (child.request->GetRequestType()) {
+    switch (child->request->GetRequestType()) {
       case net::URLFetcher::POST:
         method = "POST";
         break;
       case net::URLFetcher::PUT:
         method = "PUT";
         break;
       default:
         NOTREACHED();
         break;
     }
     const std::string header = base::StringPrintf(
         kBatchUploadRequestFormat, method.c_str(), url.path().c_str(),
         url_generator_.GetBatchUploadUrl().host().c_str(), type.c_str());
 
-    child.data_offset = header.size();
-    child.data_size = data.size();
+    child->data_offset = header.size();
+    child->data_size = data.size();
 
     parts.push_back(ContentTypeAndData());
     parts.back().type = kHttpContentType;
     parts.back().data = header;
     parts.back().data.append(data);
   }
 
   std::vector<uint64> part_data_offset;
   GenerateMultipartBody(MULTIPART_MIXED, boundary_, parts, &upload_content_,
                         &part_data_offset);
   DCHECK(part_data_offset.size() == child_requests_.size());
   for (size_t i = 0; i < child_requests_.size(); ++i) {
-    child_requests_[i].data_offset += part_data_offset[i];
+    child_requests_[i]->data_offset += part_data_offset[i];
   }
   prepare_callback_.Run(HTTP_SUCCESS);
 }
 
 bool BatchUploadRequest::GetContentData(std::string* upload_content_type,
                                         std::string* upload_content_data) {
   upload_content_type->assign(upload_content_.type);
   upload_content_data->assign(upload_content_.data);
   return true;
 }
@@ skipping 31 matching lines @@
   std::vector<MultipartHttpResponse> parts;
   if (!ParseMultipartResponse(content_type, response_writer()->data(),
                               &parts) ||
       child_requests_.size() != parts.size()) {
     RunCallbackOnPrematureFailure(DRIVE_PARSE_ERROR);
     sender_->RequestFinished(this);
     return;
   }
 
   for (size_t i = 0; i < parts.size(); ++i) {
-    child_requests_[i].request->ProcessURLFetchResults(parts[i].code,
-                                                       parts[i].body);
+    BatchableDelegate* const delegate = child_requests_[i]->request.get();
+    // Pass onwership of |delegate| so that it is deleted after notifying.

[kinaba, 2015/05/14 04:16:40]

I think there's a room for improvement on this comment. At the first time I read
"so that it is deleted after notifying" as saying "so that it is *deleted*", but
only after careful inspection of the CL I realized that your intention is rather
"so that it is *after*".

How about:

// Pass onwership of |delegate| so that child_requests_.clear() won't
// kill the delegate. It has to be deleted after the notification.

[hirono, 2015/05/14 04:43:12]

Thank you for the suggestion. I just copied the comment.

+    delegate->NotifyResult(
+        parts[i].code, parts[i].body,
+        base::Bind(&EmptyClosure, base::Passed(&child_requests_[i]->request)));
   }
+  child_requests_.clear();
 
-  child_requests_.clear();
   sender_->RequestFinished(this);
 }
 
 void BatchUploadRequest::RunCallbackOnPrematureFailure(DriveApiErrorCode code) {
   for (auto child : child_requests_) {
-    child.request->RunCallbackOnPrematureFailure(code);
-    sender_->RequestFinished(child.request);
+    child->request->NotifyError(code);
   }
   child_requests_.clear();
 }
 
 void BatchUploadRequest::OnURLFetchUploadProgress(const net::URLFetcher* source,
                                                   int64 current,
                                                   int64 total) {
   for (auto child : child_requests_) {
-    if (child.data_offset <= current &&
-        current <= child.data_offset + child.data_size) {
-      child.request->OnURLFetchUploadProgress(
-          source, current - child.data_offset, child.data_size);
-    } else if (last_progress_value_ < child.data_offset + child.data_size &&
-               child.data_offset + child.data_size < current) {
-      child.request->OnURLFetchUploadProgress(source, child.data_size,
-                                              child.data_size);
+    if (child->data_offset <= current &&
+        current <= child->data_offset + child->data_size) {
+      child->request->NotifyUploadProgress(source, current - child->data_offset,
+                                           child->data_size);
+    } else if (last_progress_value_ < child->data_offset + child->data_size &&
+               child->data_offset + child->data_size < current) {
+      child->request->NotifyUploadProgress(source, child->data_size,
+                                           child->data_size);
     }
   }
   last_progress_value_ = current;
 }
 }  // namespace drive
 }  // namespace google_apis