[Downloads] Cleanup DownloadResourceHandler

content/browser/download/download_resource_handler.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/download/download_resource_handler.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/logging.h"
@@ skipping 58 matching lines @@
 
 const int DownloadResourceHandler::kDownloadByteStreamSize = 100 * 1024;
 
 DownloadResourceHandler::DownloadResourceHandler(
     uint32 id,
     net::URLRequest* request,
     const DownloadUrlParameters::OnStartedCallback& started_cb,
     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),
       bytes_read_(0),
       pause_count_(0),
       was_deferred_(false),
       on_response_started_called_(false) {
   RecordDownloadCount(UNTHROTTLED_COUNT);
 }
 
@@ skipping 28 matching lines @@
            << " request_id = " << request_id;
   download_start_time_ = base::TimeTicks::Now();
 
   // If it's a download, we don't want to poison the cache with it.
   request()->StopCaching();
 
   // Lower priority as well, so downloads don't contend for resources
   // 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;
   CreateByteStream(
       base::MessageLoopProxy::current(),
       BrowserThread::GetMessageLoopProxyForThread(BrowserThread::FILE),
       kDownloadByteStreamSize, &stream_writer_, &stream_reader);
   stream_writer_->RegisterCallback(
       base::Bind(&DownloadResourceHandler::ResumeRequest, AsWeakPtr()));
 
   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);
 
   info->request_handle =
       DownloadRequestHandle(AsWeakPtr(), request_info->GetChildID(),
                             request_info->GetRouteID(),
                             request_info->GetRequestID());
 
   // 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.
+    headers->EnumerateHeader(NULL, "Last-Modified", &info->last_modified);
+    headers->EnumerateHeader(NULL, "ETag", &info->etag);

[Randy Smith (Not in Mondays), 2013/11/20 21:35:56]

I'm feeling like if I objected to ignoring the return value from GetMimeType() I
should also object to ignoring the return value from EnumerateHeader().  But
here we're implicitly assuming that the value will be left alone if there's no
header of that type (which assumption is actually wrong--it's cleared, unlike in
GetMImeType, where it's left alone.  Which actually looks like a bug in
GetMimeType, because the return value is based on whether it's empty or not. 
Yuck.)  So I sorta feel like it's ok to ignore the return type here, which makes
me wonder if I was being too nit-picky on GetMImeType() :-} :-J.  

So I think I'll request: Do the same thing here as for GetMimeType(), and I'll
be ok with whichever way you got.  (Though I'll call out that if I'm reading the
code correctly, we're relying on info->original_mime_type being null going into
GetMimeType()).

[asanka, 2013/11/21 17:30:21]

Done. I chose to check the return value here as well since the behavior when
there's no header isn't documented.

 
     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 = "";
     }
+
+    if (!headers->GetMimeType(&info->original_mime_type))
+      info->original_mime_type.clear();
   }
 
-  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_ = "";
-  }
-
-  info->save_info = save_info_.Pass();
-
   BrowserThread::PostTask(
       BrowserThread::UI, FROM_HERE,
       base::Bind(&StartOnUIThread,
                  base::Passed(&info),
                  base::Passed(&stream_reader),
                  // Pass to StartOnUIThread so that variable
                  // access is always on IO thread but function
                  // is called on UI thread.
                  started_cb_));
   // Guaranteed to be called in StartOnUIThread
@@ skipping 156 matching lines @@
       default:    // All other errors.
         // Redirection and informational codes should have been handled earlier
         // in the stack.
         DCHECK_NE(3, response_code / 100);
         DCHECK_NE(1, response_code / 100);
         reason = DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED;
         break;
     }
   }
 
-  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);
 
   // Send the info down the stream.  Conditional is in case we get
   // OnResponseCompleted without OnResponseStarted.
   if (stream_writer_)
     stream_writer_->Close(reason);
 
   // If the error mapped to something unknown, record it so that
   // we can drill down.
   if (reason == DOWNLOAD_INTERRUPT_REASON_NETWORK_FAILED) {
     UMA_HISTOGRAM_CUSTOM_ENUMERATION("Download.MapErrorNetworkFailed",
                                      std::abs(status.error()),
                                      net::GetAllErrorCodesForUma());
   }
 
   stream_writer_.reset();  // We no longer need the stream.
   read_buffer_ = NULL;
 }
 
 void DownloadResourceHandler::OnDataDownloaded(
     int request_id,
     int bytes_downloaded) {
   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));
 
   ++pause_count_;
 }
 
 void DownloadResourceHandler::ResumeRequest() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   DCHECK_LT(0, pause_count_);
 
@@ skipping 51 matching lines @@
 
   // Remove output stream callback if a stream exists.
   if (stream_writer_)
     stream_writer_->RegisterCallback(base::Closure());
 
   UMA_HISTOGRAM_TIMES("SB2.DownloadDuration",
                       base::TimeTicks::Now() - download_start_time_);
 }
 
 }  // namespace content