Initial CL for Downloads resumption.

content/browser/download/download_resource_handler.cc

Index: content/browser/download/download_resource_handler.cc
diff --git a/content/browser/download/download_resource_handler.cc b/content/browser/download/download_resource_handler.cc
index d7e7a68ce5d062b6c96ca4966d45704ec13fa667..69bb3f270878b1b8f998e78f6a76b12fd85150d0 100644
--- a/content/browser/download/download_resource_handler.cc
+++ b/content/browser/download/download_resource_handler.cc
@@ -73,10 +73,12 @@ static void StartOnUIThread(
 }  // 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),
@@ -149,6 +151,7 @@ bool DownloadResourceHandler::OnResponseStarted(
   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();
@@ -173,6 +176,14 @@ bool DownloadResourceHandler::OnResponseStarted(
       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;
@@ -323,19 +334,31 @@ bool DownloadResourceHandler::OnResponseCompleted(
     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;
       }