Cleaned up the API of HttpUtil::ParseContentRangeHeader().

Cleaned up the API of HttpUtil::ParseContentRangeHeader().

HttpUtil::ParseContentRangeHeader() is used to validate and extract
values from a Content-Range header for a 206 response.

Before this CL, ParseContentRangeHeader() also had partial support for
extracting values from a Content-Range header for a 416 response,
without validating the returned values.

This behavior didn't appear to be used by any production code, so this
CL removes that support and makes ParseContentRangeHeader return true
only if all three of |first_byte_position|, |last_byte_position|, and
|instance_length| are specified and valid for a 206 response; otherwise,
all outputs are set to -1 and returns false.

BUG=670913
Committed: https://crrev.com/d43b2fbe1d074927d76c6fa6fb7e4cd816b8527a
Cr-Commit-Position: refs/heads/master@{#439305}

[sclittle, 2016-12-06 00:50:22]

Patchset #1 (id:1) has been deleted

[sclittle, 2016-12-06 00:50:28]

Patchset #1 (id:20001) has been deleted

[sclittle, 2016-12-06 00:51:39]

sclittle@chromium.org changed reviewers:
+ mmenke@chromium.org

[mmenke, 2016-12-06 19:18:06]

Looks good, just think we should rename the method, to make sure it's clear that
it doesn't handle all valid byte ranges

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

https://codereview.chromium.org/2549143003/diff/40001/net/http/http_response_...
net/http/http_response_headers.h:302: bool GetContentRange(int64_t*
first_byte_position,
Maybe rename to GetContentRangeFor206, and same for the other one?  That will
require updating all callsites, but that also forces the change in front of all
consumers, and they can sanity check the change (Yes, it looks fine, but I'm
paranoid, and could imagine breaking external embedders as well)

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

https://codereview.chromium.org/2549143003/diff/40001/net/http/http_util.cc#n...
net/http/http_util.cc:313: }
nit:  Suggest a blank line here.

[sclittle, 2016-12-08 20:37:55]

sclittle@chromium.org changed reviewers:
+ benwells@chromium.org, dmurph@chromium.org, michaeln@chromium.org,
rdsmith@chromium.org

[sclittle, 2016-12-08 20:37:56]

benwells: extensions/*
michaeln: appcache/*
dmurph: blob_storage/*
rdsmith: content/browser/download/*

Thanks in advance!

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

https://codereview.chromium.org/2549143003/diff/40001/net/http/http_response_...
net/http/http_response_headers.h:302: bool GetContentRange(int64_t*
first_byte_position,
On 2016/12/06 19:18:06, mmenke wrote:
> Maybe rename to GetContentRangeFor206, and same for the other one?  That will
> require updating all callsites, but that also forces the change in front of
all
> consumers, and they can sanity check the change (Yes, it looks fine, but I'm
> paranoid, and could imagine breaking external embedders as well)

Done.

[mmenke, 2016-12-08 20:38:58]

Thanks so much!  LGTM!

[dmurph, 2016-12-08 20:59:36]

lgtm

[Randy Smith (Not in Mondays), 2016-12-08 21:03:41]

content/browser/download LGTM (based on previous review of change).

[michaeln, 2016-12-08 23:24:46]

appcache/* lgtm

[sclittle, 2016-12-14 00:17:56]

sclittle@chromium.org changed reviewers:
+ mek@chromium.org
- benwells@chromium.org

[sclittle, 2016-12-14 00:17:57]

+mek for extensions/*

-benwells

[Marijn Kruisselbrink, 2016-12-14 03:24:52]

On 2016/12/14 at 00:17:57, sclittle wrote:
> +mek for extensions/*
lgtm

[sclittle, 2016-12-14 03:25:15]

The CQ bit was checked by sclittle@chromium.org

[commit-bot: I haz the power, 2016-12-14 03:26:00]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 03:33:18]

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

[commit-bot: I haz the power, 2016-12-14 03:33:19]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[sclittle, 2016-12-17 00:43:46]

The CQ bit was checked by sclittle@chromium.org

[sclittle, 2016-12-17 00:43:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org, mek@chromium.org, rdsmith@chromium.org,
mmenke@chromium.org, dmurph@chromium.org
Link to the patchset: https://codereview.chromium.org/2549143003/#ps80001
(title: "Fixed unittest")

[commit-bot: I haz the power, 2016-12-17 00:44:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-17 03:20:51]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481935426825860,
"parent_rev": "ea555eb21aa8828c2779610b6aa34cadbb68c8b2", "commit_rev":
"6cbdb7067661df5ce800dd432f78abe4e0aebb11"}

[commit-bot: I haz the power, 2016-12-17 03:21:20]

Description was changed from

==========
Cleaned up the API of HttpUtil::ParseContentRangeHeader().

HttpUtil::ParseContentRangeHeader() is used to validate and extract
values from a Content-Range header for a 206 response.

Before this CL, ParseContentRangeHeader() also had partial support for
extracting values from a Content-Range header for a 416 response,
without validating the returned values.

This behavior didn't appear to be used by any production code, so this
CL removes that support and makes ParseContentRangeHeader return true
only if all three of |first_byte_position|, |last_byte_position|, and
|instance_length| are specified and valid for a 206 response; otherwise,
all outputs are set to -1 and returns false.

BUG=670913
==========

to

==========
Cleaned up the API of HttpUtil::ParseContentRangeHeader().

HttpUtil::ParseContentRangeHeader() is used to validate and extract
values from a Content-Range header for a 206 response.

Before this CL, ParseContentRangeHeader() also had partial support for
extracting values from a Content-Range header for a 416 response,
without validating the returned values.

This behavior didn't appear to be used by any production code, so this
CL removes that support and makes ParseContentRangeHeader return true
only if all three of |first_byte_position|, |last_byte_position|, and
|instance_length| are specified and valid for a 206 response; otherwise,
all outputs are set to -1 and returns false.

BUG=670913

Review-Url: https://codereview.chromium.org/2549143003
==========

[commit-bot: I haz the power, 2016-12-17 03:21:21]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-12-17 03:23:16]

Description was changed from

==========
Cleaned up the API of HttpUtil::ParseContentRangeHeader().

HttpUtil::ParseContentRangeHeader() is used to validate and extract
values from a Content-Range header for a 206 response.

Before this CL, ParseContentRangeHeader() also had partial support for
extracting values from a Content-Range header for a 416 response,
without validating the returned values.

This behavior didn't appear to be used by any production code, so this
CL removes that support and makes ParseContentRangeHeader return true
only if all three of |first_byte_position|, |last_byte_position|, and
|instance_length| are specified and valid for a 206 response; otherwise,
all outputs are set to -1 and returns false.

BUG=670913

Review-Url: https://codereview.chromium.org/2549143003
==========

to

==========
Cleaned up the API of HttpUtil::ParseContentRangeHeader().

HttpUtil::ParseContentRangeHeader() is used to validate and extract
values from a Content-Range header for a 206 response.

Before this CL, ParseContentRangeHeader() also had partial support for
extracting values from a Content-Range header for a 416 response,
without validating the returned values.

This behavior didn't appear to be used by any production code, so this
CL removes that support and makes ParseContentRangeHeader return true
only if all three of |first_byte_position|, |last_byte_position|, and
|instance_length| are specified and valid for a 206 response; otherwise,
all outputs are set to -1 and returns false.

BUG=670913

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

[commit-bot: I haz the power, 2016-12-17 03:23:17]

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