Cleanup: have common HttpResponseHeaders routine to update with range

Cleanup: have common HttpResponseHeaders routine to update with range

Currently we do this header-fixup in two places: PartialData and AppCache,
and I'm planning to do the very same header-fixup in yet another module,
ServiceWorker.  I want to have a common utility in net/http/http_util
to do this so that all 3 modules can share it.

BUG=349319
TEST=HttpResponseHeadersTest.UpdateWithNewRange

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

[kinuko, 2014-03-05 10:05:11]

Hi, I do like to make a small refactoring to have common routine in
net/http/http_util. Could you take a look?

[rvargas (doing something else), 2014-03-05 19:47:33]

https://codereview.chromium.org/187583002/diff/20001/net/http/http_util.cc
File net/http/http_util.cc (right):

https://codereview.chromium.org/187583002/diff/20001/net/http/http_util.cc#ne...
net/http/http_util.cc:300: if (!byte_range.IsValid())
We should probably dcheck this and return void... the callers are not checking
the return value.

https://codereview.chromium.org/187583002/diff/20001/net/http/http_util.h
File net/http/http_util.h (right):

https://codereview.chromium.org/187583002/diff/20001/net/http/http_util.h#new...
net/http/http_util.h:71: // Updates Content-Length and Content-Range headers in
the |headers|
tiny nit: go as close to 80 columns as possible.

https://codereview.chromium.org/187583002/diff/20001/net/http/http_util.h#new...
net/http/http_util.h:76: static bool UpdateResponseHeadersWithRange(
Did you consider making this a member of ResponseHeaders instead?. This code is
not as general as the usual ResponseHeaders API, but given that it operates
directly on a response header it probably should be there.

https://codereview.chromium.org/187583002/diff/20001/net/http/partial_data.cc
File net/http/partial_data.cc (right):

https://codereview.chromium.org/187583002/diff/20001/net/http/partial_data.cc...
net/http/partial_data.cc:394: // success==false cases.
nit: remove comment

[kinuko, 2014-03-06 04:11:29]

(Changed issue subject)

Thanks, addressed comments.

https://codereview.chromium.org/187583002/diff/20001/net/http/http_util.cc
File net/http/http_util.cc (right):

https://codereview.chromium.org/187583002/diff/20001/net/http/http_util.cc#ne...
net/http/http_util.cc:300: if (!byte_range.IsValid())
On 2014/03/05 19:47:33, rvargas wrote:
> We should probably dcheck this and return void... the callers are not checking
> the return value. 

Done.

https://codereview.chromium.org/187583002/diff/20001/net/http/http_util.h
File net/http/http_util.h (right):

https://codereview.chromium.org/187583002/diff/20001/net/http/http_util.h#new...
net/http/http_util.h:71: // Updates Content-Length and Content-Range headers in
the |headers|
On 2014/03/05 19:47:33, rvargas wrote:
> tiny nit: go as close to 80 columns as possible.

Done.

https://codereview.chromium.org/187583002/diff/20001/net/http/http_util.h#new...
net/http/http_util.h:76: static bool UpdateResponseHeadersWithRange(
Makes sense, I was worried that no other http util directly works on
ResponseHeaders. Done.

https://codereview.chromium.org/187583002/diff/20001/net/http/partial_data.cc
File net/http/partial_data.cc (right):

https://codereview.chromium.org/187583002/diff/20001/net/http/partial_data.cc...
net/http/partial_data.cc:394: // success==false cases.
On 2014/03/05 19:47:33, rvargas wrote:
> nit: remove comment

Done.

[kinuko, 2014-03-06 04:14:42]

(+michaeln for appcache/ owner review)

[mmenke, 2014-03-06 18:18:47]

I'm going to defer to rvargas on this one - I really don't know byte ranges. 
Two suggestions, though.

https://codereview.chromium.org/187583002/diff/40001/net/http/http_response_h...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/187583002/diff/40001/net/http/http_response_h...
net/http/http_response_headers.cc:383: DCHECK(byte_range.IsValid());
Maybe add:

DCHECK(!byte_range.suffix_length());
DCHECK_NE(-1, byte_range.last_byte_position());

https://codereview.chromium.org/187583002/diff/40001/net/http/http_response_h...
File net/http/http_response_headers.h (right):

https://codereview.chromium.org/187583002/diff/40001/net/http/http_response_h...
net/http/http_response_headers.h:91: // to give invalid |byte_range|.
This also doesn't work for suffix byte ranges, or those unbounded on the right.

[michaeln, 2014-03-06 18:58:52]

appcache lgtm

[rvargas (doing something else), 2014-03-06 19:24:00]

https://codereview.chromium.org/187583002/diff/40001/net/http/http_response_h...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/187583002/diff/40001/net/http/http_response_h...
net/http/http_response_headers.cc:383: DCHECK(byte_range.IsValid());
On 2014/03/06 18:18:48, mmenke wrote:
> Maybe add:
> 
> DCHECK(!byte_range.suffix_length());
> DCHECK_NE(-1, byte_range.last_byte_position());

Excellent point. Now that this lives here we cannot assume where the byte range
comes from.

I think the checks that we need are actually
HasFirstBytePosition/HasLastBytePosition. That implies a valid, already bounded
range regardless of how it was originally created.

IsValid by itself accepts unbounded data, and having a suffix is ok as long as
the struct goes at some point through ComputeBounds, so using those checks here
is not appropriate.

https://codereview.chromium.org/187583002/diff/40001/net/http/http_response_h...
File net/http/http_response_headers.h (right):

https://codereview.chromium.org/187583002/diff/40001/net/http/http_response_h...
net/http/http_response_headers.h:91: // to give invalid |byte_range|.
Maybe the right message is that byte_range should come from a valid response, or
that should be usable for a response (as opposed to for a request)... or maybe
that is implied by the fact that this attempts to use that range for a response
:)

[kinuko, 2014-03-07 05:05:13]

Updated, thanks.

https://codereview.chromium.org/187583002/diff/40001/net/http/http_response_h...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/187583002/diff/40001/net/http/http_response_h...
net/http/http_response_headers.cc:383: DCHECK(byte_range.IsValid());
On 2014/03/06 19:24:00, rvargas wrote:
> On 2014/03/06 18:18:48, mmenke wrote:
> > Maybe add:
> > 
> > DCHECK(!byte_range.suffix_length());
> > DCHECK_NE(-1, byte_range.last_byte_position());
> 
> Excellent point. Now that this lives here we cannot assume where the byte
range
> comes from.
> 
> I think the checks that we need are actually
> HasFirstBytePosition/HasLastBytePosition. That implies a valid, already
bounded
> range regardless of how it was originally created.
> 
> IsValid by itself accepts unbounded data, and having a suffix is ok as long as
> the struct goes at some point through ComputeBounds, so using those checks
here
> is not appropriate.

Added DCHECKs for HasFirstBytePosition/HasLastBytePosition. (I wasn't aware that
IsValid accepts unbounded range)

https://codereview.chromium.org/187583002/diff/40001/net/http/http_response_h...
File net/http/http_response_headers.h (right):

https://codereview.chromium.org/187583002/diff/40001/net/http/http_response_h...
net/http/http_response_headers.h:91: // to give invalid |byte_range|.
I tried to improve the comment... I'm still learning how byte range works and I
hope the new comment makes sense.

[rvargas (doing something else), 2014-03-07 19:07:53]

LGTM

[kinuko, 2014-03-10 02:18:00]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2014-03-10 02:18:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/187583002/60001

[commit-bot: I haz the power, 2014-03-10 02:18:32]

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

[commit-bot: I haz the power, 2014-03-10 02:18:33]

Try jobs failed on following builders: mac_chromium_rel

[kinuko, 2014-03-10 04:28:40]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2014-03-10 04:28:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kinuko@chromium.org/187583002/60001

[commit-bot: I haz the power, 2014-03-10 07:29:01]

Change committed as 255905