From 9e997d126ed5aea804efd253d5e424e8ddacb53d Mon Sep 17 00:00:00 2001 From: "asanka@chromium.org" Date: Fri, 22 Nov 2013 21:11:28 +0000 Subject: [PATCH] [Downloads] Cleanup DownloadResourceHandler * Avoid duplicate assignments. * Don't calculate whether strong validators are present in an HTTP response. A spec compliant implementation of this determination is already present elsewhere. BUG=7648 Review URL: https://codereview.chromium.org/66993008 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@236825 0039d316-1c4b-4281-b951-d872f2087c98 --- content/browser/download/download_create_info.cc | 17 ++--- content/browser/download/download_create_info.h | 3 +- .../browser/download/download_resource_handler.cc | 88 +++++++++------------- .../browser/download/download_resource_handler.h | 11 --- content/browser/download/download_stats.cc | 8 +- content/browser/download/download_stats.h | 21 ++++-- tools/metrics/histograms/histograms.xml | 7 +- 7 files changed, 66 insertions(+), 89 deletions(-) diff --git a/content/browser/download/download_create_info.cc b/content/browser/download/download_create_info.cc index 8c47cabd0380..e381fa854bd7 100644 --- a/content/browser/download/download_create_info.cc +++ b/content/browser/download/download_create_info.cc @@ -11,20 +11,19 @@ namespace content { -DownloadCreateInfo::DownloadCreateInfo( - const base::Time& start_time, - int64 total_bytes, - const net::BoundNetLog& bound_net_log, - bool has_user_gesture, - PageTransition transition_type) +DownloadCreateInfo::DownloadCreateInfo(const base::Time& start_time, + int64 total_bytes, + const net::BoundNetLog& bound_net_log, + bool has_user_gesture, + PageTransition transition_type, + scoped_ptr save_info) : start_time(start_time), total_bytes(total_bytes), download_id(DownloadItem::kInvalidId), has_user_gesture(has_user_gesture), transition_type(transition_type), - save_info(new DownloadSaveInfo()), - request_bound_net_log(bound_net_log) { -} + save_info(save_info.Pass()), + request_bound_net_log(bound_net_log) {} DownloadCreateInfo::DownloadCreateInfo() : total_bytes(0), diff --git a/content/browser/download/download_create_info.h b/content/browser/download/download_create_info.h index 4326b2428b0e..e032fbf293d8 100644 --- a/content/browser/download/download_create_info.h +++ b/content/browser/download/download_create_info.h @@ -28,7 +28,8 @@ struct CONTENT_EXPORT DownloadCreateInfo { int64 total_bytes, const net::BoundNetLog& bound_net_log, bool has_user_gesture, - PageTransition transition_type); + PageTransition transition_type, + scoped_ptr save_info); DownloadCreateInfo(); ~DownloadCreateInfo(); diff --git a/content/browser/download/download_resource_handler.cc b/content/browser/download/download_resource_handler.cc index 98a7f579a9df..3e460b3f0698 100644 --- a/content/browser/download/download_resource_handler.cc +++ b/content/browser/download/download_resource_handler.cc @@ -76,7 +76,6 @@ DownloadResourceHandler::DownloadResourceHandler( scoped_ptr save_info) : ResourceHandler(request), download_id_(id), - content_length_(0), started_cb_(started_cb), save_info_(save_info.Pass()), last_buffer_size_(0), @@ -125,19 +124,22 @@ bool DownloadResourceHandler::OnResponseStarted( // with main frames. request()->SetPriority(net::IDLE); - std::string content_disposition; - request()->GetResponseHeaderByName("content-disposition", - &content_disposition); - SetContentDisposition(content_disposition); - SetContentLength(response->head.content_length); + // If the content-length header is not present (or contains something other + // than numbers), the incoming content_length is -1 (unknown size). + // Set the content length to 0 to indicate unknown size to DownloadManager. + int64 content_length = + response->head.content_length > 0 ? response->head.content_length : 0; const ResourceRequestInfoImpl* request_info = GetRequestInfo(); // Deleted in DownloadManager. - scoped_ptr info(new DownloadCreateInfo( - base::Time::Now(), content_length_, - request()->net_log(), request_info->HasUserGesture(), - request_info->GetPageTransition())); + scoped_ptr info( + new DownloadCreateInfo(base::Time::Now(), + content_length, + request()->net_log(), + request_info->HasUserGesture(), + request_info->GetPageTransition(), + save_info_.Pass())); // Create the ByteStream for sending data to the download sink. scoped_ptr stream_reader; @@ -151,12 +153,10 @@ bool DownloadResourceHandler::OnResponseStarted( info->download_id = download_id_; info->url_chain = request()->url_chain(); info->referrer_url = GURL(request()->referrer()); - info->start_time = base::Time::Now(); - info->total_bytes = content_length_; - info->has_user_gesture = request_info->HasUserGesture(); - info->content_disposition = content_disposition_; info->mime_type = response->head.mime_type; info->remote_address = request()->GetSocketAddress().host(); + request()->GetResponseHeaderByName("content-disposition", + &info->content_disposition); RecordDownloadMimeType(info->mime_type); RecordDownloadContentDisposition(info->content_disposition); @@ -168,35 +168,25 @@ bool DownloadResourceHandler::OnResponseStarted( // Get the last modified time and etag. const net::HttpResponseHeaders* headers = request()->response_headers(); if (headers) { - std::string last_modified_hdr; - if (headers->EnumerateHeader(NULL, "Last-Modified", &last_modified_hdr)) - info->last_modified = last_modified_hdr; - if (headers->EnumerateHeader(NULL, "ETag", &etag_)) - info->etag = etag_; + // TODO(asanka): Only store these if headers->HasStrongValidators() is true. + // See RFC 2616 section 13.3.3. + if (!headers->EnumerateHeader(NULL, "Last-Modified", &info->last_modified)) + info->last_modified.clear(); + if (!headers->EnumerateHeader(NULL, "ETag", &info->etag)) + info->etag.clear(); int status = headers->response_code(); if (2 == status / 100 && status != net::HTTP_PARTIAL_CONTENT) { // Success & not range response; if we asked for a range, we didn't // get it--reset the file pointers to reflect that. - save_info_->offset = 0; - save_info_->hash_state = ""; + info->save_info->offset = 0; + info->save_info->hash_state = ""; } - } - std::string content_type_header; - if (!response->head.headers.get() || - !response->head.headers->GetMimeType(&content_type_header)) - content_type_header = ""; - info->original_mime_type = content_type_header; - - if (!response->head.headers.get() || - !response->head.headers->EnumerateHeader( - NULL, "Accept-Ranges", &accept_ranges_)) { - accept_ranges_ = ""; + if (!headers->GetMimeType(&info->original_mime_type)) + info->original_mime_type.clear(); } - info->save_info = save_info_.Pass(); - BrowserThread::PostTask( BrowserThread::UI, FROM_HERE, base::Bind(&StartOnUIThread, @@ -373,9 +363,17 @@ void DownloadResourceHandler::OnResponseCompleted( } } - RecordAcceptsRanges(accept_ranges_, bytes_read_, etag_); - RecordNetworkBlockage( - base::TimeTicks::Now() - download_start_time_, total_pause_time_); + std::string accept_ranges; + bool has_strong_validators = false; + if (request()->response_headers()) { + request()->response_headers()->EnumerateHeader( + NULL, "Accept-Ranges", &accept_ranges); + has_strong_validators = + request()->response_headers()->HasStrongValidators(); + } + RecordAcceptsRanges(accept_ranges, bytes_read_, has_strong_validators); + RecordNetworkBlockage(base::TimeTicks::Now() - download_start_time_, + total_pause_time_); CallStartedCB(NULL, error_code); @@ -402,22 +400,6 @@ void DownloadResourceHandler::OnDataDownloaded( NOTREACHED(); } -// If the content-length header is not present (or contains something other -// than numbers), the incoming content_length is -1 (unknown size). -// Set the content length to 0 to indicate unknown size to DownloadManager. -void DownloadResourceHandler::SetContentLength(const int64& content_length) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); - content_length_ = 0; - if (content_length > 0) - content_length_ = content_length; -} - -void DownloadResourceHandler::SetContentDisposition( - const std::string& content_disposition) { - DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); - content_disposition_ = content_disposition; -} - void DownloadResourceHandler::PauseRequest() { DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)); diff --git a/content/browser/download/download_resource_handler.h b/content/browser/download/download_resource_handler.h index e7114a1cea8b..36bb0d270053 100644 --- a/content/browser/download/download_resource_handler.h +++ b/content/browser/download/download_resource_handler.h @@ -96,16 +96,7 @@ class CONTENT_EXPORT DownloadResourceHandler // on the IO thread. void CallStartedCB(DownloadItem* item, net::Error error); - // If the content-length header is not present (or contains something other - // than numbers), the incoming content_length is -1 (unknown size). - // Set the content length to 0 to indicate unknown size to DownloadManager. - void SetContentLength(const int64& content_length); - - void SetContentDisposition(const std::string& content_disposition); - uint32 download_id_; - std::string content_disposition_; - int64 content_length_; // This is read only on the IO thread, but may only // be called on the UI thread. DownloadUrlParameters::OnStartedCallback started_cb_; @@ -122,8 +113,6 @@ class CONTENT_EXPORT DownloadResourceHandler base::TimeDelta total_pause_time_; size_t last_buffer_size_; int64 bytes_read_; - std::string accept_ranges_; - std::string etag_; int pause_count_; bool was_deferred_; diff --git a/content/browser/download/download_stats.cc b/content/browser/download/download_stats.cc index 5b5f893c2f47..df3a059f5cc8 100644 --- a/content/browser/download/download_stats.cc +++ b/content/browser/download/download_stats.cc @@ -332,7 +332,7 @@ void RecordDownloadWriteLoopCount(int count) { void RecordAcceptsRanges(const std::string& accepts_ranges, int64 download_len, - const std::string& etag) { + bool has_strong_validator) { int64 max = 1024 * 1024 * 1024; // One Terabyte. download_len /= 1024; // In Kilobytes static const int kBuckets = 50; @@ -349,10 +349,8 @@ void RecordAcceptsRanges(const std::string& accepts_ranges, 1, max, kBuckets); - // ETags that start with "W/" are considered weak ETags which don't imply - // byte-wise equality. - if (!StartsWithASCII(etag, "w/", false)) - RecordDownloadCount(STRONG_ETAG_AND_ACCEPTS_RANGES); + if (has_strong_validator) + RecordDownloadCount(STRONG_VALIDATOR_AND_ACCEPTS_RANGES); } else { UMA_HISTOGRAM_CUSTOM_COUNTS("Download.AcceptRangesMissingOrInvalid.KBytes", download_len, diff --git a/content/browser/download/download_stats.h b/content/browser/download/download_stats.h index 9a9b1f28b452..b1307d340095 100644 --- a/content/browser/download/download_stats.h +++ b/content/browser/download/download_stats.h @@ -72,14 +72,19 @@ enum DownloadCountTypes { // successful invocation of ScanAndSaveDownloadedFile(). FILE_MISSING_AFTER_SUCCESSFUL_SCAN_COUNT, - // Count of downloads that supplies a strong ETag and has a 'Accept-Ranges: - // bytes' header. These downloads are candidates for partial resumption. - STRONG_ETAG_AND_ACCEPTS_RANGES, + // (Deprecated) Count of downloads with a strong ETag and specified + // 'Accept-Ranges: bytes'. + DOWNLOAD_COUNT_UNUSED_15, // Count of downloads that didn't have a valid WebContents at the time it was // interrupted. INTERRUPTED_WITHOUT_WEBCONTENTS, + // Count of downloads that supplies a strong validator (implying byte-wise + // equivalence) and has a 'Accept-Ranges: bytes' header. These downloads are + // candidates for partial resumption. + STRONG_VALIDATOR_AND_ACCEPTS_RANGES, + DOWNLOAD_COUNT_TYPES_LAST_ENTRY }; @@ -165,10 +170,12 @@ void RecordBandwidth(double actual_bandwidth, double potential_bandwidth); void RecordOpen(const base::Time& end, bool first); // Record whether or not the server accepts ranges, and the download size. Also -// counts if a strong ETag is supplied. The combination of range request support -// and ETag indicates downloads that are candidates for partial resumption. -void RecordAcceptsRanges(const std::string& accepts_ranges, int64 download_len, - const std::string& etag); +// counts if a strong validator is supplied. The combination of range request +// support and ETag indicates downloads that are candidates for partial +// resumption. +void RecordAcceptsRanges(const std::string& accepts_ranges, + int64 download_len, + bool has_strong_validator); // Record the number of downloads removed by ClearAll. void RecordClearAllSize(int size); diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml index 8048663ae9e4..8a6b285a336a 100644 --- a/tools/metrics/histograms/histograms.xml +++ b/tools/metrics/histograms/histograms.xml @@ -22082,13 +22082,14 @@ other types of suffix sets. - - + + - + + -- 2.11.4.GIT