Use If-Range instead of If-Match/If-Unmodified-Since for partial requests.

Use If-Range instead of If-Match/If-Unmodified-Since for partial requests.

If-Range can shortcut partial resumption requests by having the server
respond with the entire entity if there's a validator mismatch. This
cuts down resumptions by one roundtrip.

The If-Range has been in use by HttpCacheTransaction successfully and it
is not believed that availability of support is a concern.

R=rdsmith@chromium.org,svaldez@chromium.org,mkearney@chromium.org,davidben@chromium.org,isherman@chromium.org
BUG=7648
Committed: https://crrev.com/eaa4e86fe7d9154bde31181108180480e1d603c1
Cr-Commit-Position: refs/heads/master@{#362771}

[asanka, 2015-11-16 19:32:13]

asanka@chromium.org changed reviewers:
+ rdsmith@chromium.org, svaldez@chromium.org

[asanka, 2015-11-16 19:33:18]

rdsmith: I'm breaking out the change to use If-Range instead of
If-Match/If-Unmodified-Since.

[asanka, 2015-11-16 19:33:29]

On 2015/11/16 at 19:33:18, asanka wrote:
> rdsmith: I'm breaking out the change to use If-Range instead of
If-Match/If-Unmodified-Since.

svaldez: FYI

[Randy Smith (Not in Mondays), 2015-11-16 20:59:57]

Should we make a corresponding change on the receiving side?  I looked over the
code, and I think the corresponding change would be to DCHECK or default on
HTTP_PRECONDITION_FAILED, but I couldn't find the code that handled the range
header and automatically truncated the download file if we were starting from
the beginning.  Can you give me a pointer to that so I can verify that logic as
well?

[asanka, 2015-11-16 21:21:04]

On 2015/11/16 at 20:59:57, rdsmith wrote:
> Should we make a corresponding change on the receiving side?  I looked over
the code, and I think the corresponding change would be to DCHECK or default on
HTTP_PRECONDITION_FAILED, but I couldn't find the code that handled the range
header and automatically truncated the download file if we were starting from
the beginning.  Can you give me a pointer to that so I can verify that logic as
well?

The server behavior for If-Range is:

   IF [validators match the entity at the target URL]:
     IF [request's "Range" header specifies a valid range]:
       Send status "HTTP/1.1 206 Partial Response" along with
Content-Range/Content-Length and content body according to the request's "Range"
header.
     ELSE
       Send status "HTTP/1.1 416 Range not satisfiable"
   ELSE:
     Send "HTTP/1.1 200 Success" along with the entire body.


The DownloadResourceHandler code currently handles the 206 and 200 statuses in
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/do...:

    if (2 == status / 100  && status != net::HTTP_PARTIAL_CONTENT) {
      // Success & not range response; if we asked for a range, we didn't
      // get it--reset the file pointers to reflect that.
      info->save_info->offset = 0;
      info->save_info->hash_state = "";
    }

Fortunately, the behavior of a validator mismatch is identical to the server not
respecting the If-Range/Ranges headers at all. Both outcomes result in DRH
treating the response as the entirety of the entity to be downloaded.

A 206 continues to result in being treated as a partial response.

[Randy Smith (Not in Mondays), 2015-11-17 22:04:39]

On 2015/11/16 21:21:04, asanka wrote:
> On 2015/11/16 at 20:59:57, rdsmith wrote:
> > Should we make a corresponding change on the receiving side?  I looked over
> the code, and I think the corresponding change would be to DCHECK or default
on
> HTTP_PRECONDITION_FAILED, but I couldn't find the code that handled the range
> header and automatically truncated the download file if we were starting from
> the beginning.  Can you give me a pointer to that so I can verify that logic
as
> well?
> 
> The server behavior for If-Range is:
> 
>    IF [validators match the entity at the target URL]:
>      IF [request's "Range" header specifies a valid range]:
>        Send status "HTTP/1.1 206 Partial Response" along with
> Content-Range/Content-Length and content body according to the request's
"Range"
> header.
>      ELSE
>        Send status "HTTP/1.1 416 Range not satisfiable"
>    ELSE:
>      Send "HTTP/1.1 200 Success" along with the entire body.
> 
> 
> The DownloadResourceHandler code currently handles the 206 and 200 statuses in
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/do...:
> 
>     if (2 == status / 100  && status != net::HTTP_PARTIAL_CONTENT) {
>       // Success & not range response; if we asked for a range, we didn't
>       // get it--reset the file pointers to reflect that.
>       info->save_info->offset = 0;
>       info->save_info->hash_state = "";
>     }
> 
> Fortunately, the behavior of a validator mismatch is identical to the server
not
> respecting the If-Range/Ranges headers at all. Both outcomes result in DRH
> treating the response as the entirety of the entity to be downloaded.
> 
> A 206 continues to result in being treated as a partial response.

That all makes sense.  What do you think of DCHECKing on
HTTP_PRECONDITION_FAILED?  It sounds like we're no longer sending requests for
which that's ever a valid response.

[asanka, 2015-11-21 01:27:38]

On 2015/11/17 at 22:04:39, rdsmith wrote:
> On 2015/11/16 21:21:04, asanka wrote:
> > On 2015/11/16 at 20:59:57, rdsmith wrote:
> > > Should we make a corresponding change on the receiving side?  I looked
over
> > the code, and I think the corresponding change would be to DCHECK or default
on
> > HTTP_PRECONDITION_FAILED, but I couldn't find the code that handled the
range
> > header and automatically truncated the download file if we were starting
from
> > the beginning.  Can you give me a pointer to that so I can verify that logic
as
> > well?
> > 
> > The server behavior for If-Range is:
> > 
> >    IF [validators match the entity at the target URL]:
> >      IF [request's "Range" header specifies a valid range]:
> >        Send status "HTTP/1.1 206 Partial Response" along with
> > Content-Range/Content-Length and content body according to the request's
"Range"
> > header.
> >      ELSE
> >        Send status "HTTP/1.1 416 Range not satisfiable"
> >    ELSE:
> >      Send "HTTP/1.1 200 Success" along with the entire body.
> > 
> > 
> > The DownloadResourceHandler code currently handles the 206 and 200 statuses
in
> >
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/do...:
> > 
> >     if (2 == status / 100  && status != net::HTTP_PARTIAL_CONTENT) {
> >       // Success & not range response; if we asked for a range, we didn't
> >       // get it--reset the file pointers to reflect that.
> >       info->save_info->offset = 0;
> >       info->save_info->hash_state = "";
> >     }
> > 
> > Fortunately, the behavior of a validator mismatch is identical to the server
not
> > respecting the If-Range/Ranges headers at all. Both outcomes result in DRH
> > treating the response as the entirety of the entity to be downloaded.
> > 
> > A 206 continues to result in being treated as a partial response.
> 
> That all makes sense.  What do you think of DCHECKing on
HTTP_PRECONDITION_FAILED?  It sounds like we're no longer sending requests for
which that's ever a valid response.

[asanka, 2015-11-21 01:28:41]

On 2015/11/21 at 01:27:38, asanka wrote:
> On 2015/11/17 at 22:04:39, rdsmith wrote:
> > On 2015/11/16 21:21:04, asanka wrote:
> > > On 2015/11/16 at 20:59:57, rdsmith wrote:
> > > > Should we make a corresponding change on the receiving side?  I looked
over
> > > the code, and I think the corresponding change would be to DCHECK or
default on
> > > HTTP_PRECONDITION_FAILED, but I couldn't find the code that handled the
range
> > > header and automatically truncated the download file if we were starting
from
> > > the beginning.  Can you give me a pointer to that so I can verify that
logic as
> > > well?
> > > 
> > > The server behavior for If-Range is:
> > > 
> > >    IF [validators match the entity at the target URL]:
> > >      IF [request's "Range" header specifies a valid range]:
> > >        Send status "HTTP/1.1 206 Partial Response" along with
> > > Content-Range/Content-Length and content body according to the request's
"Range"
> > > header.
> > >      ELSE
> > >        Send status "HTTP/1.1 416 Range not satisfiable"
> > >    ELSE:
> > >      Send "HTTP/1.1 200 Success" along with the entire body.
> > > 
> > > 
> > > The DownloadResourceHandler code currently handles the 206 and 200
statuses in
> > >
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/do...:
> > > 
> > >     if (2 == status / 100  && status != net::HTTP_PARTIAL_CONTENT) {
> > >       // Success & not range response; if we asked for a range, we didn't
> > >       // get it--reset the file pointers to reflect that.
> > >       info->save_info->offset = 0;
> > >       info->save_info->hash_state = "";
> > >     }
> > > 
> > > Fortunately, the behavior of a validator mismatch is identical to the
server not
> > > respecting the If-Range/Ranges headers at all. Both outcomes result in DRH
> > > treating the response as the entirety of the entity to be downloaded.
> > > 
> > > A 206 continues to result in being treated as a partial response.
> > 
> > That all makes sense.  What do you think of DCHECKing on
HTTP_PRECONDITION_FAILED?  It sounds like we're no longer sending requests for
which that's ever a valid response.

I chose the retire the PRECONDITION_FAILED condition. I'm reluctant to DCHECK on
HTTP_PRECONDITION_FAILED since that can be sent by any server and hence seeing
it doesn't necessarily imply a bug in Chrome.

[Randy Smith (Not in Mondays), 2015-11-21 15:40:03]

LGTM with nit.

https://codereview.chromium.org/1444253003/diff/40001/content/public/browser/...
File content/public/browser/download_interrupt_reason_values.h (left):

https://codereview.chromium.org/1444253003/diff/40001/content/public/browser/...
content/public/browser/download_interrupt_reason_values.h:92: 
I'd leave a comment here just so it's clear the hole is intentional.

[asanka, 2015-11-23 16:25:51]

asanka@chromium.org changed reviewers:
+ davidben@chromium.org, isherman@chromium.org, mkearney@chromium.org

[asanka, 2015-11-23 16:26:41]

Description was changed from

==========
Use If-Range instead of If-Match/If-Unmodified-Since for partial requests.

If-Range can shortcut partial resumption requests by having the server
respond with the entire entity if there's a validator mismatch. This
cuts down resumptions by one roundtrip.

The If-Range has been in use by HttpCacheTransaction successfully and it
is not believed that availability of support is a concern.

R=rdsmith@chromium.org,svaldez@chromium.org
BUG=7648
==========

to

==========
Use If-Range instead of If-Match/If-Unmodified-Since for partial requests.

If-Range can shortcut partial resumption requests by having the server
respond with the entire entity if there's a validator mismatch. This
cuts down resumptions by one roundtrip.

The If-Range has been in use by HttpCacheTransaction successfully and it
is not believed that availability of support is a concern.

R=rdsmith@chromium.org,svaldez@chromium.org,mkearney@chromium.org,davidben@ch...
BUG=7648
==========

[asanka, 2015-11-23 16:42:48]

rdsmith: Thanks!

mkearney: downloads.idl
  I'm removing SERVER_PRECONDITION from InterruptReasons enum.

  Note that this change causes "SERVER_PRECONDITION" to no longer be recognized
as an InterruptReason. This affects search() and erase() calls which use a
DownloadQuery that specifies an 'error' field of 'SERVER_PRECONDITION'. This
shouldn't happen because the SERVER_PRECONDITION interrupt reason is an
ephemeral state during download resumption and was not expected to be set on a
download in a way that search() and erase() could've observed it. 

  I'm removing it because we will now never set this interrupt reason on a
download. In the longer term it would be great if we could remove the specific
interrupt reasons from the public API (probably infeasible) because they are
exposing implementation details in a way that is already causing problems.

asvitkine: histograms.xml
  I'm marking one error code as being deprecated.

davidben: content/public/browser/download_interrupt_reason_values.h
  Removing one error code and replacing it with a comment.

https://codereview.chromium.org/1444253003/diff/40001/content/public/browser/...
File content/public/browser/download_interrupt_reason_values.h (left):

https://codereview.chromium.org/1444253003/diff/40001/content/public/browser/...
content/public/browser/download_interrupt_reason_values.h:92: 
On 2015/11/21 at 15:40:03, rdsmith wrote:
> I'd leave a comment here just so it's clear the hole is intentional.

Done.

[Randy Smith (Not in Mondays), 2015-11-23 17:13:45]

On 2015/11/23 16:42:48, asanka wrote:
> rdsmith: Thanks!
> 
> mkearney: downloads.idl
>   I'm removing SERVER_PRECONDITION from InterruptReasons enum.
> 
>   Note that this change causes "SERVER_PRECONDITION" to no longer be
recognized
> as an InterruptReason. This affects search() and erase() calls which use a
> DownloadQuery that specifies an 'error' field of 'SERVER_PRECONDITION'. This
> shouldn't happen because the SERVER_PRECONDITION interrupt reason is an
> ephemeral state during download resumption and was not expected to be set on a
> download in a way that search() and erase() could've observed it. 
> 
>   I'm removing it because we will now never set this interrupt reason on a
> download. In the longer term it would be great if we could remove the specific
> interrupt reasons from the public API (probably infeasible) because they are
> exposing implementation details in a way that is already causing problems.
> 
> asvitkine: histograms.xml
>   I'm marking one error code as being deprecated.
> 
> davidben: content/public/browser/download_interrupt_reason_values.h
>   Removing one error code and replacing it with a comment.

Separate from this CL, I'll note that I think it would be reasonable/a good idea
to put a per-file line pointer at Asanak and I in content/public/browser/OWNERS
for all the downloads files, given my sense as to what's required to approve
changes to those files and the current burden on content OWNERS.

> 
>
https://codereview.chromium.org/1444253003/diff/40001/content/public/browser/...
> File content/public/browser/download_interrupt_reason_values.h (left):
> 
>
https://codereview.chromium.org/1444253003/diff/40001/content/public/browser/...
> content/public/browser/download_interrupt_reason_values.h:92: 
> On 2015/11/21 at 15:40:03, rdsmith wrote:
> > I'd leave a comment here just so it's clear the hole is intentional.
> 
> Done.

[davidben, 2015-11-23 17:35:37]

content/public/browser/download_interrupt_reason_values.h lgtm with nit. Didn't
look at the rest of the CL.

https://codereview.chromium.org/1444253003/diff/60001/content/public/browser/...
File content/public/browser/download_interrupt_reason_values.h (right):

https://codereview.chromium.org/1444253003/diff/60001/content/public/browser/...
content/public/browser/download_interrupt_reason_values.h:93: //
INTERRUPT_REASON(SERVER_PRECONDITION, 32)
Nit: Maybe do

// Obsolete: INTERRUPT_REASON(...)

or

// Obsolete: Precondition failed. [...]

So it's clearer this value is supposed to be gone. Otherwise you have to read
the comment carefully to realize what's going on.

[Ilya Sherman, 2015-11-23 19:21:09]

histograms.xml lgtm

[asanka, 2015-11-23 22:01:11]

https://codereview.chromium.org/1444253003/diff/60001/content/public/browser/...
File content/public/browser/download_interrupt_reason_values.h (right):

https://codereview.chromium.org/1444253003/diff/60001/content/public/browser/...
content/public/browser/download_interrupt_reason_values.h:93: //
INTERRUPT_REASON(SERVER_PRECONDITION, 32)
On 2015/11/23 at 17:35:37, davidben (slow) wrote:
> Nit: Maybe do
> 
> // Obsolete: INTERRUPT_REASON(...)
> 
> or
> 
> // Obsolete: Precondition failed. [...]
> 
> So it's clearer this value is supposed to be gone. Otherwise you have to read
the comment carefully to realize what's going on.

Done. Went with the former.

[asanka, 2015-11-24 17:34:34]

mkearney: ping? Could you take a look at the downloads.idl changes? #12 mentions
some caveats.

[asanka, 2015-11-30 19:14:39]

On 2015/11/24 at 17:34:34, asanka wrote:
> mkearney: ping? Could you take a look at the downloads.idl changes? #12
mentions some caveats.

Ping^2

[mkearney1, 2015-12-02 16:49:50]

lgtm

[asanka, 2015-12-02 16:51:25]

asanka@chromium.org changed reviewers:
+ devlinn.cronin@chromium.org

[asanka, 2015-12-02 16:56:36]

asanka@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org
- devlinn.cronin@chromium.org

[asanka, 2015-12-02 16:59:18]

On 2015/12/02 at 16:49:50, mkearney wrote:
> lgtm

Thanks!

Devlin: Could you also take a look at downloads.idl? mkearney recommended that I
run this by you as well.

We are removing a enum that's no longer used.

[Devlin, 2015-12-02 17:18:27]

lgtm

https://codereview.chromium.org/1444253003/diff/80001/chrome/common/extension...
File chrome/common/extensions/api/downloads.idl (left):

https://codereview.chromium.org/1444253003/diff/80001/chrome/common/extension...
chrome/common/extensions/api/downloads.idl:58: SERVER_PRECONDITION,
Unfortunately, we expose these enums on the api object, so this would have the
potential to break code if anyone used
chrome.downloads.InterruptReason.SERVER_PRECONDITION, this would break.

Luckily, it looks as though no one does, so this should be safe.  But just as
FYI that, in general, we shouldn't be doing this.

[asanka, 2015-12-02 17:23:52]

Thanks!

https://codereview.chromium.org/1444253003/diff/80001/chrome/common/extension...
File chrome/common/extensions/api/downloads.idl (left):

https://codereview.chromium.org/1444253003/diff/80001/chrome/common/extension...
chrome/common/extensions/api/downloads.idl:58: SERVER_PRECONDITION,
On 2015/12/02 at 17:18:27, Devlin wrote:
> Unfortunately, we expose these enums on the api object, so this would have the
potential to break code if anyone used
chrome.downloads.InterruptReason.SERVER_PRECONDITION, this would break.
> 
> Luckily, it looks as though no one does, so this should be safe.  But just as
FYI that, in general, we shouldn't be doing this.

Yup. See comment #12 (relevant portion pasted below):

---
  Note that this change causes "SERVER_PRECONDITION" to no longer be recognized
as an InterruptReason. This affects search() and erase() calls which use a
DownloadQuery that specifies an 'error' field of 'SERVER_PRECONDITION'. This
shouldn't happen because the SERVER_PRECONDITION interrupt reason is an
ephemeral state during download resumption and was not expected to be set on a
download in a way that search() and erase() could've observed it. 

  I'm removing it because we will now never set this interrupt reason on a
download. In the longer term it would be great if we could remove the specific
interrupt reasons from the public API (probably infeasible) because they are
exposing implementation details in a way that is already causing problems.
---

[asanka, 2015-12-02 17:24:31]

The CQ bit was checked by asanka@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-02 17:24:47]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1444253003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1444253003/80001

[Devlin, 2015-12-02 17:26:48]

On 2015/12/02 17:23:52, asanka wrote:
> ---
>   Note that this change causes "SERVER_PRECONDITION" to no longer be
recognized
> as an InterruptReason. This affects search() and erase() calls which use a
> DownloadQuery that specifies an 'error' field of 'SERVER_PRECONDITION'. This
> shouldn't happen because the SERVER_PRECONDITION interrupt reason is an
> ephemeral state during download resumption and was not expected to be set on a
> download in a way that search() and erase() could've observed it. 
> 
>   I'm removing it because we will now never set this interrupt reason on a
> download. In the longer term it would be great if we could remove the specific
> interrupt reasons from the public API (probably infeasible) because they are
> exposing implementation details in a way that is already causing problems.
> ---

This *will* remove SERVER_PRECONDITION from the public (extension) API.

[asanka, 2015-12-02 17:48:17]

On 2015/12/02 at 17:26:48, rdevlin.cronin wrote:
> On 2015/12/02 17:23:52, asanka wrote:
> > ---
> >   Note that this change causes "SERVER_PRECONDITION" to no longer be
recognized
> > as an InterruptReason. This affects search() and erase() calls which use a
> > DownloadQuery that specifies an 'error' field of 'SERVER_PRECONDITION'. This
> > shouldn't happen because the SERVER_PRECONDITION interrupt reason is an
> > ephemeral state during download resumption and was not expected to be set on
a
> > download in a way that search() and erase() could've observed it. 
> > 
> >   I'm removing it because we will now never set this interrupt reason on a
> > download. In the longer term it would be great if we could remove the
specific
> > interrupt reasons from the public API (probably infeasible) because they are
> > exposing implementation details in a way that is already causing problems.
> > ---
> 
> This *will* remove SERVER_PRECONDITION from the public (extension) API.

I worded things poorly. What I meant with "remove the specific interrupt
reasons" is "remove the aspect of exposing interrupt reasons as specific enums."
Interrupt reasons and their meanings are things that the core downloads
currently wants to consider as fluid. I.e. we need to add / remove / change
their meaning as the implementation evolves. This is even more true now that we
are implementing resumption for interrupted downloads. All this makes the list
of interrupt reasons be a bad candidate for exposing as a part of any stable
extensions API.

Then again, I don't know how we are going to undo the fact that interrupt
reasons are already a part of the public API.

[commit-bot: I haz the power, 2015-12-02 18:58:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-02 18:58:03]

Dry run: This issue passed the CQ dry run.

[asanka, 2015-12-02 19:14:48]

The CQ bit was checked by asanka@chromium.org

[asanka, 2015-12-02 19:14:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdsmith@chromium.org, isherman@chromium.org, davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1444253003/#ps80001
(title: "Make it obvious that the interrupt reason is obsolete.")

[commit-bot: I haz the power, 2015-12-02 19:15:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1444253003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1444253003/80001

[commit-bot: I haz the power, 2015-12-02 19:32:13]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-02 19:33:09]

Description was changed from

==========
Use If-Range instead of If-Match/If-Unmodified-Since for partial requests.

If-Range can shortcut partial resumption requests by having the server
respond with the entire entity if there's a validator mismatch. This
cuts down resumptions by one roundtrip.

The If-Range has been in use by HttpCacheTransaction successfully and it
is not believed that availability of support is a concern.

R=rdsmith@chromium.org,svaldez@chromium.org,mkearney@chromium.org,davidben@ch...
BUG=7648
==========

to

==========
Use If-Range instead of If-Match/If-Unmodified-Since for partial requests.

If-Range can shortcut partial resumption requests by having the server
respond with the entire entity if there's a validator mismatch. This
cuts down resumptions by one roundtrip.

The If-Range has been in use by HttpCacheTransaction successfully and it
is not believed that availability of support is a concern.

R=rdsmith@chromium.org,svaldez@chromium.org,mkearney@chromium.org,davidben@ch...
BUG=7648

Committed: https://crrev.com/eaa4e86fe7d9154bde31181108180480e1d603c1
Cr-Commit-Position: refs/heads/master@{#362771}
==========

[commit-bot: I haz the power, 2015-12-02 19:33:11]

Patchset 5 (id:??) landed as
https://crrev.com/eaa4e86fe7d9154bde31181108180480e1d603c1
Cr-Commit-Position: refs/heads/master@{#362771}