Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(309)

Unified Diff: content/browser/download/download_request_core.cc

Issue 2660783002: Range request support for parallel download in DownloadRequestCore. (Closed)
Patch Set: Work on feedbacks. Created 3 years, 10 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « no previous file | content/browser/download/download_request_core_unittest.cc » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
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..18f602215ff7d243229e9cd59ec0cf7187086f22 100644
--- a/content/browser/download/download_request_core.cc
+++ b/content/browser/download/download_request_core.cc
@@ -37,6 +37,7 @@
#include "net/base/load_flags.h"
#include "net/base/net_errors.h"
#include "net/base/upload_bytes_element_reader.h"
+#include "net/http/http_request_headers.h"
#include "net/http/http_response_headers.h"
#include "net/http/http_status_code.h"
#include "net/url_request/url_request_context.h"
@@ -158,19 +159,23 @@ 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(i.e. etag or last modified) is required in range requests
+ // for download resumption and parallel download.
+ DCHECK((params->offset() == 0 &&
+ params->length() == DownloadSaveInfo::kLengthFullContent) ||
+ has_etag || has_last_modified);
+
+ // Add "Range" and "If-Range" request header fields if the range request is to
+ // retrieve bytes from {offset} to the end of the file.
+ // E.g. "Range:bytes=50-".
+ if (params->offset() > 0 &&
+ params->length() == DownloadSaveInfo::kLengthFullContent &&
+ (has_etag || has_last_modified)) {
request->SetExtraRequestHeaderByName(
- "Range", base::StringPrintf("bytes=%" PRId64 "-", params->offset()),
- true);
+ net::HttpRequestHeaders::kRange,
+ base::StringPrintf("bytes=%" PRId64 "-", params->offset()), true);
- // In accordance with RFC 2616 Section 14.27, use If-Range to specify that
+ // In accordance with RFC 7233 Section 3.2, use If-Range to specify that
// the server return the entire entity if the validator doesn't match.
// Last-Modified can be used in the absence of ETag as a validator if the
// response headers satisfied the HttpUtil::HasStrongValidators() predicate.
@@ -178,7 +183,32 @@ std::unique_ptr<net::URLRequest> DownloadRequestCore::CreateRequestOnIOThread(
// This function assumes that HasStrongValidators() was true and that the
// ETag and Last-Modified header values supplied are valid.
request->SetExtraRequestHeaderByName(
- "If-Range", has_etag ? params->etag() : params->last_modified(), true);
+ net::HttpRequestHeaders::kIfRange,
+ has_etag ? params->etag() : params->last_modified(), true);
+ }
+
+ // Add "Range", "If-Match", and "If-Unmodified-Since" for range requests with
+ // last byte position specified. e.g. "Range:bytes=50-59". If the file is
+ // updated on the server, we should get http 412 response code.
+ if (params->length() != DownloadSaveInfo::kLengthFullContent &&
+ (has_etag || has_last_modified)) {
+ request->SetExtraRequestHeaderByName(
+ net::HttpRequestHeaders::kRange,
+ base::StringPrintf("bytes=%" PRId64 "-%" PRId64, params->offset(),
+ params->offset() + params->length() - 1),
+ true);
+ if (has_etag) {
+ request->SetExtraRequestHeaderByName(net::HttpRequestHeaders::kIfMatch,
+ params->etag(), true);
+ }
+ // According to RFC 7232 section 3.4, "If-Unmodified-Since" is mainly for
+ // old servers that didn't implement "If-Match" and must be ignored when
+ // "If-Match" presents.
+ if (has_last_modified) {
+ request->SetExtraRequestHeaderByName(
+ net::HttpRequestHeaders::kIfUnmodifiedSince, params->last_modified(),
+ true);
+ }
}
// Downloads are treated as top level navigations. Hence the first-party
@@ -304,7 +334,7 @@ bool DownloadRequestCore::OnResponseStarted(
const net::HttpResponseHeaders* headers = request()->response_headers();
if (headers) {
if (headers->HasStrongValidators()) {
- // If we don't have strong validators as per RFC 2616 section 13.3.3, then
+ // If we don't have strong validators as per RFC 7232 section 2, then
// we neither store nor use them for range requests.
if (!headers->EnumerateHeader(nullptr, "Last-Modified",
&create_info->last_modified))
@@ -546,7 +576,7 @@ DownloadInterruptReason DownloadRequestCore::HandleSuccessfulServerResponse(
case net::HTTP_CREATED:
case net::HTTP_ACCEPTED:
- // Per RFC 2616 the entity being transferred is metadata about the
+ // Per RFC 7231 the entity being transferred is metadata about the
// resource at the target URL and not the resource at that URL (or the
// resource that would be at the URL once processing is completed in the
// case of HTTP_ACCEPTED). However, we currently don't have special
@@ -556,7 +586,7 @@ DownloadInterruptReason DownloadRequestCore::HandleSuccessfulServerResponse(
case net::HTTP_NO_CONTENT:
case net::HTTP_RESET_CONTENT:
- // These two status codes don't have an entity (or rather RFC 2616
+ // These two status codes don't have an entity (or rather RFC 7231
// requires that there be no entity). They are treated the same as the
// resource not being found since there is no entity to download.
@@ -581,16 +611,25 @@ DownloadInterruptReason DownloadRequestCore::HandleSuccessfulServerResponse(
default: // All other errors.
// Redirection and informational codes should have been handled earlier
// in the stack.
+ // TODO(xingliu): Handle HTTP_PRECONDITION_FAILED and resurrect
+ // DOWNLOAD_INTERRUPT_REASON_SERVER_PRECONDITION for range requests.
+ // This will change extensions::api::download::InterruptReason.
DCHECK_NE(3, http_headers.response_code() / 100);
DCHECK_NE(1, http_headers.response_code() / 100);
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.
+ // Server should send partial content when "If-Match" or
+ // "If-Unmodified-Since" check passes, and the range request header has
+ // last byte position. e.g. "Range:bytes=50-99".
+ if (save_info->length != DownloadSaveInfo::kLengthFullContent)
+ return DOWNLOAD_INTERRUPT_REASON_SERVER_BAD_CONTENT;
+
+ // Requested a partial range, but received the entire response, when
+ // the range request header is "Range:bytes={offset}-".
save_info->offset = 0;
save_info->hash_of_partial_file.clear();
save_info->hash_state.reset();
@@ -604,7 +643,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.
//
« no previous file with comments | « no previous file | content/browser/download/download_request_core_unittest.cc » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698