Range request support for parallel download in DownloadRequestCore.

content/browser/download/download_request_core.cc

Index: content/browser/download/download_request_core.cc
diff --git a/content/browser/download/download_request_core.cc b/content/browser/download/download_request_core.cc
index 0fec3f3b8e5730fd3f2b16884f2ca87a9d4e819c..dcfb06182c0eeda36f6cb87b334fb92eccd07811 100644
--- a/content/browser/download/download_request_core.cc
+++ b/content/browser/download/download_request_core.cc
@@ -158,14 +158,15 @@ std::unique_ptr<net::URLRequest> DownloadRequestCore::CreateRequestOnIOThread(
   bool has_last_modified = !params->last_modified().empty();
   bool has_etag = !params->etag().empty();
 
-  // If we've asked for a range, we want to make sure that we only get that
-  // range if our current copy of the information is good.  We shouldn't be
-  // asked to continue if we don't have a verifier.
-  DCHECK(params->offset() == 0 || has_etag || has_last_modified);
-
-  // If we're not at the beginning of the file, retrieve only the remaining
-  // portion.
-  if (params->offset() > 0 && (has_etag || has_last_modified)) {
+  // Strong validator is required for download resumption.
+  DCHECK(params->offset() == 0 || params->length() > 0 || has_etag ||

[qinmin, 2017/01/31 19:25:56]

I don't think this DCHECK is right.

The original DCHECK checks is purely for resumption: if the download is a
resumption, then has_etag || has_last_modified is true. Otherwise,
params->offset() == 0.

When dealing with parallel download, you should have a separate DCHECK:
DCHECK(param->length() == DownloadSaveInfo::kLengthUnknown || has_etag ||
has_last_modified);

[xingliu, 2017/01/31 21:32:25]

Done, this is something I didn't consider that strong validator ensures requests
are working on the same file.

Merged the two DCHECK into one.

+         has_last_modified);
+
+  // Add "Range" and "If-Range" request header fields for download resumption
+  // with strong validator.
+  if (params->offset() > 0 &&
+      params->length() == DownloadSaveInfo::kLengthUnknown &&
+      (has_etag || has_last_modified)) {
     request->SetExtraRequestHeaderByName(
         "Range", base::StringPrintf("bytes=%" PRId64 "-", params->offset()),
         true);
@@ -181,6 +182,15 @@ std::unique_ptr<net::URLRequest> DownloadRequestCore::CreateRequestOnIOThread(
         "If-Range", has_etag ? params->etag() : params->last_modified(), true);
   }
 
+  // Add "Range" request header for new download if length is specified.
+  if (params->length() > 0) {

[qinmin, 2017/01/31 19:25:56]

you need to add has_etag || has_last_modified condition here. If there is no
strong validator, we cannot ensure parallel download jobs are downloading the
same content

[xingliu, 2017/01/31 21:32:25]

Done.

+    request->SetExtraRequestHeaderByName(
+        "Range",
+        base::StringPrintf("bytes=%" PRId64 "-%" PRId64, params->offset(),
+                           params->offset() + params->length() - 1),
+        true);
+  }
+
   // Downloads are treated as top level navigations. Hence the first-party
   // origin for cookies is always based on the target URL and is updated on
   // redirects.
@@ -586,12 +596,12 @@ DownloadInterruptReason DownloadRequestCore::HandleSuccessfulServerResponse(
       return DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED;
   }
 
-  if (save_info && save_info->offset > 0) {
-    // The caller is expecting a partial response.
-
+  // The caller is expecting a partial response.
+  if (save_info && (save_info->offset > 0 || save_info->length > 0)) {
     if (http_headers.response_code() != net::HTTP_PARTIAL_CONTENT) {
       // Requested a partial range, but received the entire response.
       save_info->offset = 0;
+      save_info->length = DownloadSaveInfo::kLengthUnknown;
       save_info->hash_of_partial_file.clear();
       save_info->hash_state.reset();
       return DOWNLOAD_INTERRUPT_REASON_NONE;
@@ -604,7 +614,9 @@ DownloadInterruptReason DownloadRequestCore::HandleSuccessfulServerResponse(
       return DOWNLOAD_INTERRUPT_REASON_SERVER_BAD_CONTENT;
     DCHECK_GE(first_byte, 0);
 
-    if (first_byte != save_info->offset) {
+    if (first_byte != save_info->offset ||
+        (save_info->length > 0 &&
+         last_byte != save_info->offset + save_info->length - 1)) {
       // The server returned a different range than the one we requested. Assume
       // the response is bad.
       //