Implement a parser that parses the "Range" HTTP header...

Implement a parser that parses the "Range" HTTP header
Parses "Range" HTTP request header so this request information
can be used in URLRequestFileJob and HttpCache.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=14784

[Alpha Left Google, 2009-04-22 09:38:29]

On 2009/04/22 09:32:19, Alpha wrote:
> 

Oops, it should be the "Range" header.

[awong, 2009-04-22 18:17:22]

Looks pretty solid.  Here's some comments.

Also, there seem to be some lint warnings.  Could you look at those too?

http://codereview.chromium.org/92006/diff/1034/1039
File net/http/http_byte_range.cc (right):

http://codereview.chromium.org/92006/diff/1034/1039#newcode42
Line 42: first_byte_position_ >= 0 && last_byte_position_ >= 0;
The second check is unecessary.  last_byte_position >=0 is implied by
first_byte_position >= 0 && fire_bypte_position_ <= last_byte_position_;

http://codereview.chromium.org/92006/diff/1034/1039#newcode55
Line 55: if (first_byte_position_ == -1 && last_byte_position_ == -1 &&
Use your Has* functions?

http://codereview.chromium.org/92006/diff/1034/1039#newcode61
Line 61: if (IsValid()) {
How about doing the sanity checking all at the top?  So

if (size < 0)
  return false;
if (!IsValid()) {
  return false;
}

That'd remove some of the indentation levels.

http://codereview.chromium.org/92006/diff/1034/1039#newcode67
Line 67: } else if (first_byte_position_ < size) {
Should this check be in IsValid?

http://codereview.chromium.org/92006/diff/1034/1040
File net/http/http_byte_range.h (right):

http://codereview.chromium.org/92006/diff/1034/1040#newcode18
Line 18: // Copy constructor, since this is just POD, it's fast and safe to
copy.
Don't you want to also define the assignment operator?

Alternately, you can leave the Copy Constructor, Assignment Operator, and
destructor undefined, and leave the comment about the POD.

http://codereview.chromium.org/92006/diff/1034/1040#newcode33
Line 33: bool IsSuffixByteRange() const;
Blah, I didn't read this right yesterday. I'm sorry.

This is actually okay to leave as a struct if you make these helpers static.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Structs_vs._Cl...

However, it's okay as is.

http://codereview.chromium.org/92006/diff/1034/1038
File net/http/http_byte_range_unittest.cc (right):

http://codereview.chromium.org/92006/diff/1034/1038#newcode15
Line 15: { -1,
I think putting these subclauses in one line is actually easier to read...

const struct { .... } test[] =
{ {-1, -1, 0, false},
  {-10, 0, 0, false),
   ...
}

Also, do people use anonymous struct syntax to create a struct without declaring
the type in Chromium code?

If it's not common, consider breaking it up to be

const struct TestData {
  ...
};

TestData test[] = {....};

http://codereview.chromium.org/92006/diff/1034/1036
File net/http/http_util.cc (right):

http://codereview.chromium.org/92006/diff/1034/1036#newcode198
Line 198: std::vector<HttpByteRange>& ranges) {
Aren't output parameters supposed to be poitners?

http://codereview.chromium.org/92006/diff/1034/1036#newcode231
Line 231: if (!StringToInt64(std::string(value.begin(),
is "value" an std::string?  If it is, can you call substr?  Might be easier to
read.

http://codereview.chromium.org/92006/diff/1034/1037
File net/http/http_util.h (right):

http://codereview.chromium.org/92006/diff/1034/1037#newcode48
Line 48: // Scans the headers and look for the first "Range" header in
|headers|,
|headers|, if

This is a run-on sentence.  Break apart.

http://codereview.chromium.org/92006/diff/1034/1037#newcode49
Line 49: // if "Range" exists and the first one of it is well formatted then
returns
"the first one of it" -> "the first one"

then returns -> then return

http://codereview.chromium.org/92006/diff/1034/1037#newcode50
Line 50: // true, |ranges| will contain a list of valid ranges. If return
, -> ;

...I think...I'm bad with semicolons.  Anyone else want to comment on the
grammatical correctness here :)

http://codereview.chromium.org/92006/diff/1034/1037#newcode52
Line 52: // "Range" header is defined in RFC 2616 Section 14.35.1.
of "Range" -> of a "Range"

http://codereview.chromium.org/92006/diff/1034/1035
File net/http/http_util_unittest.cc (right):

http://codereview.chromium.org/92006/diff/1034/1035#newcode573
Line 573: for (size_t i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) {
Does "arraysize()" not work here?

[Alpha Left Google, 2009-04-22 19:09:24]

http://codereview.chromium.org/92006/diff/1034/1039
File net/http/http_byte_range.cc (right):

http://codereview.chromium.org/92006/diff/1034/1039#newcode42
Line 42: first_byte_position_ >= 0 && last_byte_position_ >= 0;
On 2009/04/22 18:17:22, awong wrote:
> The second check is unecessary.  last_byte_position >=0 is implied by
> first_byte_position >= 0 && fire_bypte_position_ <= last_byte_position_;

Done.

http://codereview.chromium.org/92006/diff/1034/1039#newcode55
Line 55: if (first_byte_position_ == -1 && last_byte_position_ == -1 &&
On 2009/04/22 18:17:22, awong wrote:
> Use your Has* functions?

Done.

http://codereview.chromium.org/92006/diff/1034/1039#newcode61
Line 61: if (IsValid()) {
On 2009/04/22 18:17:22, awong wrote:
> How about doing the sanity checking all at the top?  So
> 
> if (size < 0)
>   return false;
> if (!IsValid()) {
>   return false;
> }
> 
> That'd remove some of the indentation levels.

Done. By if (!IsValid()) is moved down a bit. ComputeBounds should work on empty
values which is not valid.

http://codereview.chromium.org/92006/diff/1034/1039#newcode67
Line 67: } else if (first_byte_position_ < size) {
On 2009/04/22 18:17:22, awong wrote:
> Should this check be in IsValid?

IsValid() doesn't have the size information.

http://codereview.chromium.org/92006/diff/1034/1040
File net/http/http_byte_range.h (right):

http://codereview.chromium.org/92006/diff/1034/1040#newcode18
Line 18: // Copy constructor, since this is just POD, it's fast and safe to
copy.
On 2009/04/22 18:17:22, awong wrote:
> Don't you want to also define the assignment operator?
> 
> Alternately, you can leave the Copy Constructor, Assignment Operator, and
> destructor undefined, and leave the comment about the POD.
> 

Done.

http://codereview.chromium.org/92006/diff/1034/1040#newcode33
Line 33: bool IsSuffixByteRange() const;
On 2009/04/22 18:17:22, awong wrote:
> Blah, I didn't read this right yesterday. I'm sorry.
> 
> This is actually okay to leave as a struct if you make these helpers static.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Structs_vs._Cl...
> 
> However, it's okay as is.

Done.

http://codereview.chromium.org/92006/diff/1034/1038
File net/http/http_byte_range_unittest.cc (right):

http://codereview.chromium.org/92006/diff/1034/1038#newcode15
Line 15: { -1,
On 2009/04/22 18:17:22, awong wrote:
> I think putting these subclauses in one line is actually easier to read...
> 
> const struct { .... } test[] =
> { {-1, -1, 0, false},
>   {-10, 0, 0, false),
>    ...
> }
> 
> Also, do people use anonymous struct syntax to create a struct without
declaring
> the type in Chromium code?
> 
> If it's not common, consider breaking it up to be
> 
> const struct TestData {
>   ...
> };
> 
> TestData test[] = {....};
> 

They do use anonymous struct syntax for test data. :)

http://codereview.chromium.org/92006/diff/1034/1036
File net/http/http_util.cc (right):

http://codereview.chromium.org/92006/diff/1034/1036#newcode198
Line 198: std::vector<HttpByteRange>& ranges) {
On 2009/04/22 18:17:22, awong wrote:
> Aren't output parameters supposed to be poitners?

Done.

http://codereview.chromium.org/92006/diff/1034/1036#newcode231
Line 231: if (!StringToInt64(std::string(value.begin(),
On 2009/04/22 18:17:22, awong wrote:
> is "value" an std::string?  If it is, can you call substr?  Might be easier to
> read.

Done.

[awong, 2009-04-22 23:03:37]

2 more comments.

http://codereview.chromium.org/92006/diff/49/55
File net/http/http_byte_range.h (right):

http://codereview.chromium.org/92006/diff/49/55#newcode19
Line 19: // and destructor untouched.
Instead of "leave untouched" say we use the "default compiler generated" or
something like that.

http://codereview.chromium.org/92006/diff/49/51
File net/http/http_util.cc (right):

http://codereview.chromium.org/92006/diff/49/51#newcode222
Line 222: const std::string& range_string = vit.value();
I wan't actually suggesting changing the name from value -> range_string, though
the new name is file as well.

What I was saying was in all the palces you do

std::string(range_string.being(), range_string.beg() + separator_offset)

can't you just do  range_string.substr(0,separator_offset)?

[rvargas (doing something else), 2009-04-22 23:36:08]

http://codereview.chromium.org/92006/diff/49/54
File net/http/http_byte_range.cc (right):

http://codereview.chromium.org/92006/diff/49/54#newcode8
Line 8: const int kPositionNotSpecified = -1;
nit: empty lines above and below this one.

http://codereview.chromium.org/92006/diff/49/54#newcode20
Line 20: return suffix_length_ != 0;
just curious, why use 0 here instead of kPositionNotSpecified?

http://codereview.chromium.org/92006/diff/49/54#newcode33
Line 33: return suffix_length_ > 0;
nit: I guess it's simpler just to say if (suffix_length_ > 0) return true. The
other ifs on this function also look a little redundant with the expression used
by the return.

http://codereview.chromium.org/92006/diff/49/54#newcode34
Line 34: else if (HasFirstBytePosition() && HasLastBytePosition())
no need for else after return.

http://codereview.chromium.org/92006/diff/49/54#newcode35
Line 35: return first_byte_position_ >= 0&&
nit: space before &&

http://codereview.chromium.org/92006/diff/49/54#newcode60
Line 60: *upper_bound = size;
nit: return true here?

http://codereview.chromium.org/92006/diff/49/54#newcode61
Line 61: if (*lower_bound < 0)
how is this possible?

http://codereview.chromium.org/92006/diff/49/54#newcode63
Line 63: } else if (first_byte_position_ < size) {
nit: use the opposite condition and return early.

http://codereview.chromium.org/92006/diff/49/55
File net/http/http_byte_range.h (right):

http://codereview.chromium.org/92006/diff/49/55#newcode44
Line 44: bool ComputeBounds(int64 size, int64* lower_bound, int64* upper_bound)
const;
nit: could you re-word the comment?. I guess it's better if you keep it simple,
not saying "output value x to argument y" but instead just "returns z". 
(s/give/given, but it's not clear what "instance" means).

http://codereview.chromium.org/92006/diff/49/51
File net/http/http_util.cc (right):

http://codereview.chromium.org/92006/diff/49/51#newcode197
Line 197: bool HttpUtil::ParseRanges(const std::string& headers,
nit: please add a comment with "static"

http://codereview.chromium.org/92006/diff/49/51#newcode199
Line 199: net::HttpUtil::HeadersIterator it(headers.begin(), headers.end(),
"\r\n");
nit: net:: seems redundant.

http://codereview.chromium.org/92006/diff/49/51#newcode217
Line 217: return false;
Shouldn't we worry about whitespace between "bytes" and "="?

http://codereview.chromium.org/92006/diff/49/51#newcode244
Line 244: std::string(range_string.begin() + separator_offset + 1,
I guess we fail if there is whitespace here (xx - yy)

http://codereview.chromium.org/92006/diff/49/50
File net/http/http_util_unittest.cc (right):

http://codereview.chromium.org/92006/diff/49/50#newcode571
Line 571: };
We should have some tests with white spaces.

[Alpha Left Google, 2009-04-23 23:35:21]

http://codereview.chromium.org/92006/diff/49/54
File net/http/http_byte_range.cc (right):

http://codereview.chromium.org/92006/diff/49/54#newcode8
Line 8: const int kPositionNotSpecified = -1;
On 2009/04/22 23:36:08, rvargas wrote:
> nit: empty lines above and below this one.

Done.

http://codereview.chromium.org/92006/diff/49/54#newcode20
Line 20: return suffix_length_ != 0;
On 2009/04/22 23:36:08, rvargas wrote:
> just curious, why use 0 here instead of kPositionNotSpecified?

Done.

http://codereview.chromium.org/92006/diff/49/54#newcode33
Line 33: return suffix_length_ > 0;
On 2009/04/22 23:36:08, rvargas wrote:
> nit: I guess it's simpler just to say if (suffix_length_ > 0) return true. The
> other ifs on this function also look a little redundant with the expression
used
> by the return.

Done.

http://codereview.chromium.org/92006/diff/49/54#newcode34
Line 34: else if (HasFirstBytePosition() && HasLastBytePosition())
On 2009/04/22 23:36:08, rvargas wrote:
> no need for else after return.

Done.

http://codereview.chromium.org/92006/diff/49/54#newcode60
Line 60: *upper_bound = size;
On 2009/04/22 23:36:08, rvargas wrote:
> nit: return true here?

Done.

http://codereview.chromium.org/92006/diff/49/54#newcode61
Line 61: if (*lower_bound < 0)
On 2009/04/22 23:36:08, rvargas wrote:
> how is this possible?

You are right. removed.

http://codereview.chromium.org/92006/diff/49/54#newcode63
Line 63: } else if (first_byte_position_ < size) {
On 2009/04/22 23:36:08, rvargas wrote:
> nit: use the opposite condition and return early.

Done.

http://codereview.chromium.org/92006/diff/49/55
File net/http/http_byte_range.h (right):

http://codereview.chromium.org/92006/diff/49/55#newcode19
Line 19: // and destructor untouched.
On 2009/04/22 23:03:37, awong wrote:
> Instead of "leave untouched" say we use the "default compiler generated" or
> something like that.

Done.

http://codereview.chromium.org/92006/diff/49/55#newcode44
Line 44: bool ComputeBounds(int64 size, int64* lower_bound, int64* upper_bound)
const;
On 2009/04/22 23:36:08, rvargas wrote:
> nit: could you re-word the comment?. I guess it's better if you keep it
simple,
> not saying "output value x to argument y" but instead just "returns z". 
> (s/give/given, but it's not clear what "instance" means).

Done.

http://codereview.chromium.org/92006/diff/49/51
File net/http/http_util.cc (right):

http://codereview.chromium.org/92006/diff/49/51#newcode197
Line 197: bool HttpUtil::ParseRanges(const std::string& headers,
On 2009/04/22 23:36:08, rvargas wrote:
> nit: please add a comment with "static"

Done.

http://codereview.chromium.org/92006/diff/49/51#newcode199
Line 199: net::HttpUtil::HeadersIterator it(headers.begin(), headers.end(),
"\r\n");
On 2009/04/22 23:36:08, rvargas wrote:
> nit: net:: seems redundant.

Done.

http://codereview.chromium.org/92006/diff/49/51#newcode217
Line 217: return false;
On 2009/04/22 23:36:08, rvargas wrote:
> Shouldn't we worry about whitespace between "bytes" and "="?

I think we really should fail if there's a space, the spec is quite specific
about the format (in which there's no space mentioned).

http://codereview.chromium.org/92006/diff/49/51#newcode222
Line 222: const std::string& range_string = vit.value();
On 2009/04/22 23:03:37, awong wrote:
> I wan't actually suggesting changing the name from value -> range_string,
though
> the new name is file as well.
> 
> What I was saying was in all the palces you do
> 
> std::string(range_string.being(), range_string.beg() + separator_offset)
> 
> can't you just do  range_string.substr(0,separator_offset)?

Done.

http://codereview.chromium.org/92006/diff/49/51#newcode244
Line 244: std::string(range_string.begin() + separator_offset + 1,
On 2009/04/22 23:36:08, rvargas wrote:
> I guess we fail if there is whitespace here (xx - yy)

According to spec http://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html,
14.35.1.
There should be no space for "xx - yy", so I think failing here is the right
thing to do.

[scherkus (not reviewing), 2009-04-24 18:29:10]

few nits

http://codereview.chromium.org/92006/diff/3005/3010
File net/http/http_byte_range.cc (right):

http://codereview.chromium.org/92006/diff/3005/3010#newcode35
Line 35: bool HttpByteRange::IsValid() const {
it looks like you can rewrite this as the following:

if (suffix_length_ > 0)
  return true;
return first_byte_position_ >= 0 &&
    (last_byte_position_ == kPositionNotSpecified ||
     last_byte_position_ >= first_byte_position_)

either that or you can place it in the "else if" clause and return true and
ditch the extra "return false"

... just make sure you run your unit tests to verify it works correctly :)

http://codereview.chromium.org/92006/diff/3005/3010#newcode77
Line 77: }  // namespace
should be "namespace net"

http://codereview.chromium.org/92006/diff/3005/3011
File net/http/http_byte_range.h (right):

http://codereview.chromium.org/92006/diff/3005/3011#newcode18
Line 18: // Since this class is POD, we use constructor, assignment operator
add a space before this comment

http://codereview.chromium.org/92006/diff/3005/3011#newcode52
Line 52: #endif  // NET_HTTP_HTTP_BYTE_RANGE_REQUEST_H_
should be NET_HTTP_HTTP_BYTE_RANGE_H_

http://codereview.chromium.org/92006/diff/3005/3007
File net/http/http_util.cc (right):

http://codereview.chromium.org/92006/diff/3005/3007#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
bump copyright year

http://codereview.chromium.org/92006/diff/3005/3007#newcode207
Line 207: // Reach here then we parse  the first "Range" header found.
can just reword this comment to:
Parse the first "Range" header found.

http://codereview.chromium.org/92006/diff/3005/3008
File net/http/http_util.h (right):

http://codereview.chromium.org/92006/diff/3005/3008#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
bump copyright year

[rvargas (doing something else), 2009-04-24 19:01:01]

Also, make sure you fix the lint warnings.

http://codereview.chromium.org/92006/diff/49/51
File net/http/http_util.cc (right):

http://codereview.chromium.org/92006/diff/49/51#newcode217
Line 217: return false;
On 2009/04/23 23:35:22, Alpha wrote:
> On 2009/04/22 23:36:08, rvargas wrote:
> > Shouldn't we worry about whitespace between "bytes" and "="?
> 
> I think we really should fail if there's a space, the spec is quite specific
> about the format (in which there's no space mentioned).

I guess we are not reading the same thing. see:
http://www.w3.org/Protocols/rfc2616/rfc2616-sec2.html#sec2.1
specifically #rule and implied *LWS.

http://codereview.chromium.org/92006/diff/3005/3010
File net/http/http_byte_range.cc (right):

http://codereview.chromium.org/92006/diff/3005/3010#newcode66
Line 66: } else if (first_byte_position_ < size) {
nit: no need for else after return

http://codereview.chromium.org/92006/diff/3005/3010#newcode74
Line 74: return false;
nit: line not reached

[Alpha Left Google, 2009-04-24 19:55:08]

http://codereview.chromium.org/92006/diff/3005/3010
File net/http/http_byte_range.cc (right):

http://codereview.chromium.org/92006/diff/3005/3010#newcode35
Line 35: bool HttpByteRange::IsValid() const {
On 2009/04/24 18:29:10, scherkus wrote:
> it looks like you can rewrite this as the following:
> 
> if (suffix_length_ > 0)
>   return true;
> return first_byte_position_ >= 0 &&
>     (last_byte_position_ == kPositionNotSpecified ||
>      last_byte_position_ >= first_byte_position_)
> 
> either that or you can place it in the "else if" clause and return true and
> ditch the extra "return false"
> 
> ... just make sure you run your unit tests to verify it works correctly :)

Done.

http://codereview.chromium.org/92006/diff/3005/3010#newcode66
Line 66: } else if (first_byte_position_ < size) {
On 2009/04/24 19:01:01, rvargas wrote:
> nit: no need for else after return

Done.

http://codereview.chromium.org/92006/diff/3005/3010#newcode74
Line 74: return false;
On 2009/04/24 19:01:01, rvargas wrote:
> nit: line not reached

hum? there may be a case where first_byte_position_ >= size

http://codereview.chromium.org/92006/diff/3005/3011
File net/http/http_byte_range.h (right):

http://codereview.chromium.org/92006/diff/3005/3011#newcode18
Line 18: // Since this class is POD, we use constructor, assignment operator
On 2009/04/24 18:29:10, scherkus wrote:
> add a space before this comment

Done.

http://codereview.chromium.org/92006/diff/3005/3011#newcode52
Line 52: #endif  // NET_HTTP_HTTP_BYTE_RANGE_REQUEST_H_
On 2009/04/24 18:29:10, scherkus wrote:
> should be NET_HTTP_HTTP_BYTE_RANGE_H_

Done.

http://codereview.chromium.org/92006/diff/3005/3008
File net/http/http_util.h (right):

http://codereview.chromium.org/92006/diff/3005/3008#newcode1
Line 1: // Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
On 2009/04/24 18:29:10, scherkus wrote:
> bump copyright year

Done.

[rvargas (doing something else), 2009-04-24 20:31:37]

http://codereview.chromium.org/92006/diff/3005/3010
File net/http/http_byte_range.cc (right):

http://codereview.chromium.org/92006/diff/3005/3010#newcode74
Line 74: return false;
On 2009/04/24 19:55:08, Alpha wrote:
> On 2009/04/24 19:01:01, rvargas wrote:
> > nit: line not reached
> 
> hum? there may be a case where first_byte_position_ >= size

sure, ignore me!

[Alpha Left Google, 2009-04-25 01:15:47]

oh, I forgot to fix those lint bugs, they are just newline issues, will fix them
quickly.

[Alpha Left Google, 2009-04-27 17:46:59]

Please ignore the lint errors, it's just a lint bug, newline chars are there.

[Alpha Left Google, 2009-04-27 23:12:26]

I updated the unit test only and the expectations since I'm using ValueIterator
in the code to tokenize the values and ValueIterator handles LWS nicely for me.

[Alpha Left Google, 2009-04-28 02:20:42]

sorry I overlooked the implied *LWS comment, code updated and unit tests to
check for those cases.

[rvargas (doing something else), 2009-04-28 18:14:06]

Sorry, one last comment.

Just to clarify a little more my comment, I think that is fine to have a method
that changes the internal state of an object from "building" to "built". Also,
the contract for that method can be that it should be called only once, and the
test to enforce that behavior can be present only for debug builds.

http://codereview.chromium.org/92006/diff/6001/7005
File net/http/http_byte_range.cc (right):

http://codereview.chromium.org/92006/diff/6001/7005#newcode63
Line 63: last_byte_position_ = std::min(size - 1, last_byte_position_);
I think you have to make sure that calling this function n times in a row with
the same argument doesn't keep shrinking the range. To me, DCHECK if it is
called more than once is fine, but you could also return false or see if the
argument is consistent (or the same) as the previously used and do nothing...

[Alpha Left Google, 2009-04-28 18:49:20]

On 2009/04/28 18:14:06, rvargas wrote:
> Sorry, one last comment.
> 
> Just to clarify a little more my comment, I think that is fine to have a
method
> that changes the internal state of an object from "building" to "built". Also,
> the contract for that method can be that it should be called only once, and
the
> test to enforce that behavior can be present only for debug builds.
> 
> http://codereview.chromium.org/92006/diff/6001/7005
> File net/http/http_byte_range.cc (right):
> 
> http://codereview.chromium.org/92006/diff/6001/7005#newcode63
> Line 63: last_byte_position_ = std::min(size - 1, last_byte_position_);
> I think you have to make sure that calling this function n times in a row with
> the same argument doesn't keep shrinking the range. To me, DCHECK if it is
> called more than once is fine, but you could also return false or see if the
> argument is consistent (or the same) as the previously used and do nothing...

I did this by returning false when this method is called more than once, this
case is also tested by unit test.

[rvargas (doing something else), 2009-04-28 19:08:00]

http://codereview.chromium.org/92006/diff/5015/4040
File net/http/http_byte_range.h (right):

http://codereview.chromium.org/92006/diff/5015/4040#newcode49
Line 49: bool SetInstanceSize(int64 size);
Given that there is an instance_size_ and instance_size(), it is a little
counter intuitive having a method called SetInstanceSize() that does way more
than just setting said member. Setting all internal members is not really a side
effect of "ComputeBounds", but it is a serious side effect if the method is
called SetInstanceSize.

[Alpha Left Google, 2009-04-28 19:21:58]

http://codereview.chromium.org/92006/diff/5015/4040
File net/http/http_byte_range.h (right):

http://codereview.chromium.org/92006/diff/5015/4040#newcode49
Line 49: bool SetInstanceSize(int64 size);
On 2009/04/28 19:08:00, rvargas wrote:
> Given that there is an instance_size_ and instance_size(), it is a little
> counter intuitive having a method called SetInstanceSize() that does way more
> than just setting said member. Setting all internal members is not really a
side
> effect of "ComputeBounds", but it is a serious side effect if the method is
> called SetInstanceSize.

Makes sense, I reverted it back to its old name and use a bool to prevent it
from being called more than once.

[rvargas (doing something else), 2009-04-28 19:39:05]

LGTM