Propagate server response error and interrupt the download.

content/browser/download/download_worker.cc

Index: content/browser/download/download_worker.cc
diff --git a/content/browser/download/download_worker.cc b/content/browser/download/download_worker.cc
index d537b1d9951e818909a3ebf3d8957f794142ba1e..5fb6c3e5858250958febe393466c3a624d85cfd7 100644
--- a/content/browser/download/download_worker.cc
+++ b/content/browser/download/download_worker.cc
@@ -32,7 +32,13 @@ CreateUrlDownloader(std::unique_ptr<DownloadUrlParameters> params,
 
 }  // namespace
 
-DownloadWorker::DownloadWorker() : weak_factory_(this) {}
+DownloadWorker::DownloadWorker(DownloadWorker::Delegate* delegate,
+                               int64_t offset,
+                               int64_t length)
+    : delegate_(delegate),
+      offset_(offset),
+      length_(length),
+      weak_factory_(this) {}
 
 DownloadWorker::~DownloadWorker() = default;
 
@@ -48,15 +54,18 @@ void DownloadWorker::SendRequest(
 }
 
 void DownloadWorker::Pause() {

[David Trainor- moved to gerrit, 2017/03/14 18:01:43]

Do we need to propagate any request state changes once we build the
request_handle_?

[xingliu, 2017/03/14 21:33:21]

Currently we don't, may added if we need more complex error handling.

-  request_handle_->PauseRequest();
+  if (request_handle_)
+    request_handle_->PauseRequest();
 }
 
 void DownloadWorker::Resume() {
-  request_handle_->ResumeRequest();
+  if (request_handle_)
+    request_handle_->ResumeRequest();
 }
 
 void DownloadWorker::Cancel() {
-  request_handle_->CancelRequest();
+  if (request_handle_)
+    request_handle_->CancelRequest();
 }
 
 void DownloadWorker::OnUrlDownloaderStarted(
@@ -66,13 +75,20 @@ void DownloadWorker::OnUrlDownloaderStarted(
   // |callback| is not used in subsequent requests.
   DCHECK(callback.is_null());
 
-  // TODO(xingliu): Pass the |stream_reader| to parallel job and handle failed
-  // request.
+  // If the server gave us a different part of the content, due to "If-Match" or
+  // "If-Unmodified-Since" headers, reports an error, which results in the
+  // download to be restarted.
+  if (create_info->save_info->offset != offset() ||

[David Trainor- moved to gerrit, 2017/03/14 18:01:43]

Just want to make sure, do we log this properly so we can track how often we're
hitting this with parallel downloads?

[asanka, 2017/03/14 19:52:10]

DownloadRequestCore::OnResponseStarted() already handles this case and
interrupts with DOWNLOAD_INTERRUPT_REASON_SERVER_BAD_CONTENT. Why set result
again?

[xingliu, 2017/03/14 21:33:21]

Make sense. Added log and TODO and remove the rewrite of result.

The original idea to rewrite the result here is to make it not a hard failure,
but it doesn't make sense since the etag has mismatched so the original request
should retry the whole file.

[David Trainor- moved to gerrit, 2017/03/17 07:29:46]

Sounds good!  Just checking we have something in place to catch it.

[David Trainor- moved to gerrit, 2017/03/17 07:29:46]

Yeah once the etag is mismatched I assume we have to toss out everything we
already downloaded.

+      create_info->save_info->length != length()) {
+    create_info->result = DOWNLOAD_INTERRUPT_REASON_SERVER_NO_RANGE;
+  }
+
   if (create_info->result !=
       DownloadInterruptReason::DOWNLOAD_INTERRUPT_REASON_NONE) {
-    VLOG(kVerboseLevel) << "Parallel download sub request failed. reason = "
+    VLOG(kVerboseLevel) << "Parallel download sub-request failed. reason = "
                         << create_info->result;
-    NOTIMPLEMENTED();
+    if (delegate_)
+      delegate_->OnServerResponseError(this, create_info->result);
     return;
   }