[Downloads] Update origin info after each response.

content/browser/download/download_item_impl.cc

Index: content/browser/download/download_item_impl.cc
diff --git a/content/browser/download/download_item_impl.cc b/content/browser/download/download_item_impl.cc
index 507dfe1692cbc5dd0da9e58ad25fe46649b23bfe..d9a7fda8cee85b0e8bfdc6f8c009da775d86cd4e 100644
--- a/content/browser/download/download_item_impl.cc
+++ b/content/browser/download/download_item_impl.cc
@@ -896,6 +896,37 @@ DownloadItemImpl::ResumeMode DownloadItemImpl::GetResumeMode() const {
   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));
 }