[Downloads] Update origin info after each response.

content/browser/download/download_item_impl.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.
 
 // File method ordering: Methods in this file are in the same order as
 // in download_item_impl.h, with the following exception: The public
 // interface Start is placed in chronological order with the other
 // (private) routines that together define a DownloadItem's state
 // transitions as the download progresses.  See "Download progression
 // cascade" later in this file.
@@ skipping 878 matching lines @@
     case DOWNLOAD_INTERRUPT_REASON_USER_CANCELED:
     case DOWNLOAD_INTERRUPT_REASON_FILE_BLOCKED:
     case DOWNLOAD_INTERRUPT_REASON_FILE_SECURITY_CHECK_FAILED:
       mode = RESUME_MODE_INVALID;
       break;
   }
 
   return mode;
 }
 
+void DownloadItemImpl::MergeOriginInfoOnResume(
+    const DownloadCreateInfo& new_create_info) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK_EQ(RESUMING_INTERNAL, state_);
+  DCHECK(!new_create_info.url_chain.empty());
+
+  // We are going to tack on any new redirects to our list of redirects.

[Randy Smith (Not in Mondays), 2013/11/18 22:15:56]

So I can see two cases here: a) The validators match, and we're doing a
range-request for the data we haven't gotten yet, or b) the validators didn't
match, and we're starting from the beginning.  If the latter, I feel like we
should simply overwrite the redirects.  If the former, shouldn't we block
accepting the range info and restart from the beginning?  The redirects changing
while the validators stay the same seems fishy to me.  (Oh, ok, that's your TODO
below.  Is there a reason to measure the changes rather than just being
draconian?  I suppose we could check how often the validators stay strong when
the redirect chain changes, but I doubt it's going to happen very often.)

[asanka, 2013/11/19 23:54:19]

For b) case, I chose not to overwrite the redirects because we use the redirect
chain for the SafeBrowsing check at download completion.

I chose to be a bit conservative for a) here since I couldn't convince myself
that the spec disallowed a redirect for a range request. In fact searching
around the web seems to indicate that this happens.

[Randy Smith (Not in Mondays), 2013/11/20 22:32:18]

When we resume, are we resuming from the URL at the end of the redirect chain,
or the start?  If the first, this makes sense and I was confused.  If the
second, we'll end up with a redirect chain that doesn't accurately reflect the
actual redirections to get the bits that we're serving.  (Of course, in a
continuation situation we're not going to be doing an accurate reflection
anyway, as bits 0-n will be from a prefix of the redirect chain, and bits
n+1-<end> will be from the full redirect chain.  MIght be worthwhile pinging the
SB folks to see if that behavior is likely to cause them any grief).

Willing to put in a clarifying comment (if you agree, including the caveat I
have above about possible not-quite-accurate-reflection-of-reality redirect
chain)?
Huh.  Okay.

[asanka, 2013/11/21 18:11:08]

We start from the end of the chain since that's the server which sent us the
headers we are using for the download. ETag/Last-Modified is only likely to be
valid if we talk directly to the server that sent them to us.
Done.

+  std::vector<GURL>::const_iterator chain_iter =
+      new_create_info.url_chain.begin();
+  if (*chain_iter == url_chain_.back())
+    ++chain_iter;
+  url_chain_.insert(
+      url_chain_.end(), chain_iter, new_create_info.url_chain.end());
+  // TODO(asanka): Measure how URL chains change with resumption. Resumption
+  // requests are issued against the actual download URL and we don't expect any
+  // redirects.
+
+  // If the server precondition failed, then the download will be resumed
+  // automatically without validators.
+  etag_ = new_create_info.etag;
+  last_modified_time_ = new_create_info.last_modified;
+  content_disposition_ = new_create_info.content_disposition;
+  // TODO(asanka): Measure how validators change with resumption. I.e. An ETag
+  // should only change if we used it and the server responded with
+  // HTTP_PRECONDITION_FAILED.
+
+  // Don't update observers. This method is expected to be called just before a
+  // DownloadFile is created and Start() is called. The observers will be
+  // notified when the download transitions to the IN_PROGRESS state.
+}
+
 void DownloadItemImpl::NotifyRemoved() {
   FOR_EACH_OBSERVER(Observer, observers_, OnDownloadRemoved(this));
 }
 
 void DownloadItemImpl::OnDownloadedFileRemoved() {
   file_externally_removed_ = true;
   VLOG(20) << __FUNCTION__ << " download=" << DebugString(true);
   UpdateObservers();
 }
 
@@ skipping 795 matching lines @@
     case RESUME_MODE_USER_CONTINUE:
       return "USER_CONTINUE";
     case RESUME_MODE_USER_RESTART:
       return "USER_RESTART";
   }
   NOTREACHED() << "Unknown resume mode " << mode;
   return "unknown";
 }
 
 }  // namespace content