Initial CL for Downloads resumption.

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 55 matching lines @@
   DownloadItem* item = download_manager->StartDownload(
       info.Pass(), stream.Pass());
 
   if (!started_cb.is_null())
     started_cb.Run(item, net::OK);
 }
 
 }  // namespace
 
 DownloadResourceHandler::DownloadResourceHandler(
+    DownloadId id,
     net::URLRequest* request,
     const DownloadResourceHandler::OnStartedCallback& started_cb,
     scoped_ptr<DownloadSaveInfo> save_info)
-    : render_view_id_(0),               // Actually initialized below.
+    : download_id_(id),
+      render_view_id_(0),               // Actually initialized below.
       content_length_(0),
       request_(request),
       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) {
   ResourceRequestInfoImpl* info(ResourceRequestInfoImpl::ForRequest(request));
@@ skipping 52 matching lines @@
 
   // 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();
   RecordDownloadMimeType(info->mime_type);
   RecordDownloadContentDisposition(info->content_disposition);
 
   info->request_handle =
       DownloadRequestHandle(AsWeakPtr(), global_id_.child_id,
                             render_view_id_, global_id_.request_id);
 
   // Get the last modified time and etag.
   const net::HttpResponseHeaders* headers = request_->response_headers();
   if (headers) {
     std::string last_modified_hdr;
     std::string etag;
     if (headers->EnumerateHeader(NULL, "Last-Modified", &last_modified_hdr))
       info->last_modified = last_modified_hdr;
     if (headers->EnumerateHeader(NULL, "ETag", &etag))
       info->etag = etag;
+
+    int status = headers->response_code();
+    if (status == 200) {  // Continue with download:

[benjhayden, 2013/01/06 15:46:04]

This is a browser. If there isn't a constant somewhere like HTTP_OK, then I
quit.
Also, what about 201 "Created", 202 "Accepted", 203 "Non-Authoritative
Information", and especially 206 "Partial Content"?

[Randy Smith (Not in Mondays), 2013/01/07 20:54:10]

Depressingly enough, when I use code search to look at all calls to
HttpResponseHeaders::response_code(), I see a whole lot of numeric constants
used for comparison.  Maybe because it's a browser, those numbers are supposed
to be written on the livers of everyone working on it?  :-}  Though I'm afraid
my liver is sadly lacking in that department.

(Keeping my fingers cross that you won't quit ... )
So 206 is the default here--it's what we're assuming if save_info_ is non-null. 
So we don't need to check for it.  The others strike me as very unlikely in any
request that might be reasonable for a download, and specifically, if we're
sending a range request (which is the only time this is relevant) it seems like
we should only get 200, 206, or an error.  Having said all that, I think
checking for 2xx && !206 is a better test here.  I'll change it to that.  

(I think we'd be safe to just make it !206 because I think the save_info_
structure is going to be thrown away if this is an error return.  But I dislike
throwing away information in case someone in the future finds a use for it, and
I *think* success && !206 is the situation under which we need to reset the
values.  But since I can't imagine a success && !206 use case that doesn't ==
200, I'm not sure :-}.)

[benjhayden, 2013/01/07 21:14:34]

Please use net/http/http_status_code.h

[Randy Smith (Not in Mondays), 2013/01/08 16:07:03]

But all the *other* lemmings are going off the cliff!

(Done.  And thank you for finding that.)

+      // Downloading the full file.  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 = "";
+    }
   }
 
   std::string content_type_header;
   if (!response->head.headers ||
       !response->head.headers->GetMimeType(&content_type_header))
     content_type_header = "";
   info->original_mime_type = content_type_header;
 
   if (!response->head.headers ||
       !response->head.headers->EnumerateHeader(
@@ skipping 130 matching lines @@
     // stack cancelled the request.  There aren't that many things that
     // could do this to a download request (whose lifetime is separated from
     // the tab from which it came).  We map this to USER_CANCELLED as the
     // case we know about (system suspend because of laptop close) corresponds
     // to a user action.
     // TODO(ahendrickson) -- Find a better set of codes to use here, as
     // CANCELED/ERR_ABORTED can occur for reasons other than user cancel.
     reason = DOWNLOAD_INTERRUPT_REASON_USER_CANCELED;
   }
 
-  if (status.is_success()) {
-    if (response_code >= 400) {
+  if (status.is_success() &&
+      (reason == DOWNLOAD_INTERRUPT_REASON_NONE) &&
+      request_->response_headers()) {
+    // Handle server's response codes.
+    if ((response_code >= 0) && ((response_code % 100) != 2)) {

[benjhayden, 2013/01/06 15:46:04]

There really ought to be a function somewhere like IsHttpResponseSuccessful().

[benjhayden, 2013/01/06 15:46:04]

Looks like the >= should be >

[Randy Smith (Not in Mondays), 2013/01/07 20:54:10]

I'm not sure where to look.  I'd expect to see it somewhere around
HttpResponseHeaders::response_code(), and that cupboard was bare.

[Randy Smith (Not in Mondays), 2013/01/07 20:54:10]

Happy to change it, but what's the use case?  I'm not sure that response_code
can ever be zero or negative.  

[benjhayden, 2013/01/07 21:14:34]

There isn't a use-case, it just takes the sphinx-like riddle out.
(x >= 0) = ((x > 0) || (x == 0))
(x >= 0) && ((x % 100) != 2) = (((x > 0) || (x == 0)) && ((x % 100) != 2))

Sphinx: When is x == 0 && (x % 100) != 2?
You: ...
*The sphinx eats you*
*Game over*

Oh, and that should probably be (x / 100) == 2.
*That* is why I thought there should be a utility function.

[Randy Smith (Not in Mondays), 2013/01/08 16:07:03]

Ok, ok, you win.  (Aka you're completely right and I'm just refusing to admit it
:-}.)  Let me know what you think of the rewrite.  It doesn't use utility
functions (I couldn't find them, and I did scan for them--you might have better
luck, though), but I think it's clearer and commented.  

       switch(response_code) {
-        case 404:  // File Not Found.
+        // Continue with download:
+        case 200:   // Downloading the full file, even if we asked for a

[benjhayden, 2013/01/06 15:46:04]

Are there constants that we can use instead of these magic numbers?

[Randy Smith (Not in Mondays), 2013/01/07 20:54:10]

As mentioned above, didn't find any in the way I thought to scan for them.

+                    // range.
+        case 206:   // Partial content.  Leave alone.
+          break;
+        case 204:   // No content.  File not present.
+        case 404:   // File Not Found.
           reason = DOWNLOAD_INTERRUPT_REASON_SERVER_BAD_CONTENT;
           break;
+        case 412:   // Precondition failed.  Fails 'If-Unmodified-Since' or
+                    // 'If-Match'.
+          reason = DOWNLOAD_INTERRUPT_REASON_SERVER_PRECONDITION;
+          break;
         case 416:  // Range Not Satisfiable.
+          // Retry by downloading from the start automatically:
+          // If we haven't received data when we get this error, we won't.
           reason = DOWNLOAD_INTERRUPT_REASON_SERVER_NO_RANGE;
           break;
-        case 412:  // Precondition Failed.
-          reason = DOWNLOAD_INTERRUPT_REASON_SERVER_PRECONDITION;
-          break;
-        default:
+        default:    // All other errors.
           reason = DOWNLOAD_INTERRUPT_REASON_SERVER_FAILED;
           break;
       }
     }
   }
 
   RecordAcceptsRanges(accept_ranges_, bytes_read_);
   RecordNetworkBlockage(
       base::TimeTicks::Now() - download_start_time_, total_pause_time_);
 
@@ skipping 102 matching lines @@
 
   // Remove output stream callback if a stream exists.
   if (stream_writer_.get())
     stream_writer_->RegisterCallback(base::Closure());
 
   UMA_HISTOGRAM_TIMES("SB2.DownloadDuration",
                       base::TimeTicks::Now() - download_start_time_);
 }
 
 }  // namespace content