[Downloads] Update origin info after each response.

[Downloads] Update origin info after each response.

When a download is resumed, a new URL request is sent out. The response
received for this request may contain new ETag and Last-Modified
information which should be used for subsequent resumption attempts.
Otherwise if a resource changes (along with the corresponding ETag)
subsequent partial resumption attempts will all fail even if it should
have succeeded.

BUG=7648
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240773

[asanka, 2013-11-18 18:37:32]

Depends on https://codereview.chromium.org/67883007/

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

https://codereview.chromium.org/74523002/diff/1/content/browser/download/down...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/74523002/diff/1/content/browser/download/down...
content/browser/download/download_item_impl.cc:905: // We are going to tack on
any new redirects to our list of redirects.
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.)

https://codereview.chromium.org/74523002/diff/1/content/browser/download/down...
File content/browser/download/download_item_impl.h (right):

https://codereview.chromium.org/74523002/diff/1/content/browser/download/down...
content/browser/download/download_item_impl.h:170: 
Long-term (:-J) I'd like to separate out the origin info, and just assign to it.

[Randy Smith (Not in Mondays), 2013-11-18 22:16:28]

Whoops, sorry, one other thing: Could you fix the ""after on" each response?  I
think you want one of those two, not both.

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

I removed the TODO and added UMA.

https://codereview.chromium.org/74523002/diff/1/content/browser/download/down...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/74523002/diff/1/content/browser/download/down...
content/browser/download/download_item_impl.cc:905: // We are going to tack on
any new redirects to our list of redirects.
On 2013/11/18 22:15:56, rdsmith wrote:
> 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.)

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.

https://codereview.chromium.org/74523002/diff/1/content/browser/download/down...
File content/browser/download/download_item_impl.h (right):

https://codereview.chromium.org/74523002/diff/1/content/browser/download/down...
content/browser/download/download_item_impl.h:170: 
On 2013/11/18 22:15:56, rdsmith wrote:
> Long-term (:-J) I'd like to separate out the origin info, and just assign to
it.
>  

Agreed.

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

https://codereview.chromium.org/74523002/diff/1/content/browser/download/down...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/74523002/diff/1/content/browser/download/down...
content/browser/download/download_item_impl.cc:905: // We are going to tack on
any new redirects to our list of redirects.
On 2013/11/19 23:54:19, asanka wrote:
> On 2013/11/18 22:15:56, rdsmith wrote:
> > 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.)
> 
> For b) case, I chose not to overwrite the redirects because we use the
redirect
> chain for the SafeBrowsing check at download completion.

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)?

> 
> 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.

Huh.  Okay.

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

https://codereview.chromium.org/74523002/diff/1/content/browser/download/down...
File content/browser/download/download_item_impl.cc (right):

https://codereview.chromium.org/74523002/diff/1/content/browser/download/down...
content/browser/download/download_item_impl.cc:905: // We are going to tack on
any new redirects to our list of redirects.
On 2013/11/20 22:32:18, rdsmith wrote:
> On 2013/11/19 23:54:19, asanka wrote:
> > On 2013/11/18 22:15:56, rdsmith wrote:
> > > 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.)
> > 
> > For b) case, I chose not to overwrite the redirects because we use the
> redirect
> > chain for the SafeBrowsing check at download completion.
> 
> 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).

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.

> 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)?

Done.

> > 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.
> 
> Huh.  Okay.

[Randy Smith (Not in Mondays), 2013-11-27 20:08:30]

lgtm

[asanka, 2013-11-27 20:54:42]

On 2013/11/27 20:08:30, rdsmith wrote:
> lgtm

Thanks!

[asanka, 2013-12-11 20:24:36]

+asvitkine for histograms.xml

[Alexei Svitkine (slow), 2013-12-11 20:38:21]

histograms lgtm

[commit-bot: I haz the power, 2013-12-12 16:00:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asanka@chromium.org/74523002/370001

[commit-bot: I haz the power, 2013-12-13 19:21:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asanka@chromium.org/74523002/370001

[commit-bot: I haz the power, 2013-12-13 22:20:29]

Change committed as 240773