[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

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 8c47cab..e381fa8 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<DownloadSaveInfo> 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 4326b24..e032fbf 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<DownloadSaveInfo> save_info);
   DownloadCreateInfo();
   ~DownloadCreateInfo();
 
diff --git a/content/browser/download/download_resource_handler.cc b/content/browser/download/download_resource_handler.cc
index 98a7f57..3e460b3 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<DownloadSaveInfo> 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<DownloadCreateInfo> info(new DownloadCreateInfo(
-      base::Time::Now(), content_length_,
-      request()->net_log(), request_info->HasUserGesture(),
-      request_info->GetPageTransition()));
+  scoped_ptr<DownloadCreateInfo> 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<ByteStreamReader> 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 e7114a1..36bb0d2 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 5b5f893..df3a059 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 9a9b1f2..b1307d3 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 8048663..8a6b285 100644
--- a/tools/metrics/histograms/histograms.xml
+++ b/tools/metrics/histograms/histograms.xml
@@ -22082,13 +22082,14 @@ other types of suffix sets.
   <int value="7" label="Cancelled"/>
   <int value="8" label="Started"/>
   <int value="9" label="Interrupted"/>
-  <int value="10" label="Calls to AppendDataToFile (Size) (Obsolete)"/>
-  <int value="11" label="Calls to AppendDataToFile (Count) (Obsolete)"/>
+  <int value="10" label="Calls to AppendDataToFile (Size) (Obsolete 8/2013)"/>
+  <int value="11" label="Calls to AppendDataToFile (Count) (Obsolete 8/2013)"/>
   <int value="12" label="Interrupted at End of Download"/>
   <int value="13" label="Attempt to Append to Detached File"/>
   <int value="14" label="File Missing After Successful Scan"/>
-  <int value="15" label="Supports ranges and strong validation"/>
+  <int value="15" label="Supports ranges and strong ETag (Obsolete 11/2013)"/>
   <int value="16" label="No WebContents at interruption"/>
+  <int value="17" label="Supports ranges and strong validation"/>
 </enum>
 
 <enum name="DownloadDatabaseRecordDroppedType" type="int">