Modified to resolve TODO in parseversion in http_response_headers.cc

Modified to resolve TODO in parseversion in http_response_headers.cc

BUG=

[arun87.kumar, 2014-09-02 10:03:34]

arun87.kumar@samsung.com changed reviewers:
+ mattm@chromium.org

[arun87.kumar, 2014-09-02 10:03:34]

Hi Mattam,
I've tried on TODO mentioned in ParseVersion() in http_response_headers.cc. PTAL


Thanks,
Arun

[arun87.kumar, 2014-09-02 10:16:41]

I am new to chromium opensource and not sure about whom to add to review this
patch. If I have missed any, please add them also.




Thanks,
Arun

[mattm, 2014-09-02 16:53:48]

mattm@chromium.org changed reviewers:
+ eroman@chromium.org

[mattm, 2014-09-02 16:53:49]

lgtm

Arun: Have you submitted the CLA? (see
http://dev.chromium.org/developers/contributing-code#TOC-Get-your-code-ready)

For review, adding eroman who wrote that TODO.

[mattm, 2014-09-02 16:54:04]

not lgtm, pressed the wrong button

[eroman, 2014-09-02 23:57:18]

This change requires unittests. Please update: http_response_headers_unittest.cc

[arun87.kumar, 2014-09-04 12:01:19]

On 2014/09/02 23:57:18, eroman wrote:

Hi Eroman,

I have added unittests and also verified them along with existing ones for
success.

PTAL




Regards,
Arun

[eroman, 2014-09-04 22:03:43]

not lgtm.

please explain what you are trying to accomplish, this contains policy changes
on how the http version is interpreted.

There is also an implementation problem here, in that the computation of the
version number can overflow.

From a code organization standpoint:
  (1) Use a shared function for getting the major and minor number. It can take
as input a start and end iterator.
  (2) Rather than writing a function to convert the string to a number, try
re-using the functions in base/strings/string_number_conversions.h. The notable
issue here is they recognize more characters than just digits (for instance '+'
as a recall).
  (3) The unittests contain a number of formatting changes. It is hard for me to
read the diff. Please make sure the diff is clean and reflects only the changes
you are making

[arun87.kumar, 2014-09-05 15:17:55]

On 2014/09/04 22:03:43, eroman wrote:

Dear Eroman,
Thank you very much for your valuable comments. I have tried to address all the
above mentioned points in the latest patch.

GOAL - To handle leading zeros and multiple digits in HTTP Version string.

With this patch, the code is modified to interpret the HTTP Version as follows:

1. If parseVersion failed, keep http_version as 1.0 as per old logic
2. If version is 0.9 and has headers, keep http_version as 1.0 as per old logic
3. If version is 0.9 and has no headers, keep http_version as 0.9 as per old
logic
4. Else in all other cases, http_version will be same as parsed_http_version

Also the version number overflow issue is addressed by using uint32 in
httpVersion.h

(1) I have implemented a shared function - ParseVersionInternal() which is used
for both major and minor number computation.
(2) I am using  base::StringToUint() and base::UintToString() from
string_number_conversions.h to convert String to number and vice-versa
(3) I had kept only my changes in unittests file. But when I ran git cl format,
it looks like the extra whitespaces in http_response_headers_unittest.cc are
trimmed. Will keep a note of this in all future patches.


PTAL



Regards,
Arun

[arun87.kumar, 2014-09-15 07:23:42]

On 2014/09/05 15:17:55, arun87.kumar wrote:
> On 2014/09/04 22:03:43, eroman wrote:
> 
> Dear Eroman,
> Thank you very much for your valuable comments. I have tried to address all
the
> above mentioned points in the latest patch.
> 
> GOAL - To handle leading zeros and multiple digits in HTTP Version string.
> 
> With this patch, the code is modified to interpret the HTTP Version as
follows:
> 
> 1. If parseVersion failed, keep http_version as 1.0 as per old logic
> 2. If version is 0.9 and has headers, keep http_version as 1.0 as per old
logic
> 3. If version is 0.9 and has no headers, keep http_version as 0.9 as per old
> logic
> 4. Else in all other cases, http_version will be same as parsed_http_version
> 
> Also the version number overflow issue is addressed by using uint32 in
> httpVersion.h
> 
> (1) I have implemented a shared function - ParseVersionInternal() which is
used
> for both major and minor number computation.
> (2) I am using  base::StringToUint() and base::UintToString() from
> string_number_conversions.h to convert String to number and vice-versa
> (3) I had kept only my changes in unittests file. But when I ran git cl
format,
> it looks like the extra whitespaces in http_response_headers_unittest.cc are
> trimmed. Will keep a note of this in all future patches.
> 
> 
> PTAL
> 
> 
> 
> Regards,
> Arun


@mattm,

i hav submitted CLA


@all,

Please let me know if more changes or additional test cases are required.




Regards,
Arun

[eroman, 2014-09-23 21:12:29]

not lgtm

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

https://codereview.chromium.org/527883002/diff/60001/net/http/http_response_h...
net/http/http_response_headers.cc:623: // Shared function for major and minor
section of HTTP Version string
This wording needs work.

https://codereview.chromium.org/527883002/diff/60001/net/http/http_response_h...
net/http/http_response_headers.cc:634: bool is_leading_zero = true;  // handle
leading zeroes
Is there really a need to check for leading zeros? I expect StringToUint()
already handles that. The only thing to exclude is any character not in [0-9],
like + and - which it will accept.

https://codereview.chromium.org/527883002/diff/60001/net/http/http_response_h...
net/http/http_response_headers.cc:649: base::StringToUint(StringPiece(start, p),
value);
This is still not checking for overflow.

[arun87.kumar, 2014-09-24 12:04:06]

Hi eroman,

Thanks for your valuable suggestions. Have modified code as per your above
comments.


PTAL

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

https://codereview.chromium.org/527883002/diff/60001/net/http/http_response_h...
net/http/http_response_headers.cc:623: // Shared function for major and minor
section of HTTP Version string
On 2014/09/23 21:12:28, eroman wrote:
> This wording needs work.

Done.

https://codereview.chromium.org/527883002/diff/60001/net/http/http_response_h...
net/http/http_response_headers.cc:634: bool is_leading_zero = true;  // handle
leading zeroes
On 2014/09/23 21:12:28, eroman wrote:
> Is there really a need to check for leading zeros? I expect StringToUint()
> already handles that. The only thing to exclude is any character not in [0-9],
> like + and - which it will accept.

Done.

https://codereview.chromium.org/527883002/diff/60001/net/http/http_response_h...
net/http/http_response_headers.cc:649: base::StringToUint(StringPiece(start, p),
value);
On 2014/09/23 21:12:28, eroman wrote:
> This is still not checking for overflow.

Added overflow check

[arun87.kumar, 2014-10-06 04:26:19]

arun87.kumar@samsung.com changed reviewers:
+ agl@chromium.org, asanka@chromium.org, cbentzel@chromium.org,
davidben@chromium.org, gavinp@chromium.org, jar@chromium.org,
jgraettinger@chromium.org, mef@chromium.org, mmenke@chromium.org,
pauljensen@chromium.org, rch@chromium.org, rdsmith@chromium.org,
rsleevi@chromium.org, rtenneti@google.com, rvargas@chromium.org,
ttuttle@chromium.org, wtc@chromium.org

[arun87.kumar, 2014-10-06 04:26:57]

On 2014/09/24 12:04:06, arun87.kumar wrote:

Added more reviewers from OWNERS file.

[Ryan Sleevi, 2014-10-06 04:40:32]

On 2014/10/06 04:26:57, arun87.kumar wrote:
> On 2014/09/24 12:04:06, arun87.kumar wrote:
> 
> Added more reviewers from OWNERS file.

Please do not batch add owners to this. You have Eric and Matt's explicit NOT
LGTMs.

I'm removing all reviewers until you've addressed their points.

[Ryan Sleevi, 2014-10-06 04:40:45]

rsleevi@chromium.org changed reviewers:
- agl@chromium.org, asanka@chromium.org, cbentzel@chromium.org,
davidben@chromium.org, gavinp@chromium.org, jar@chromium.org,
jgraettinger@chromium.org, mef@chromium.org, mmenke@chromium.org,
pauljensen@chromium.org, rch@chromium.org, rdsmith@chromium.org,
rsleevi@chromium.org, rtenneti@google.com, rvargas@chromium.org,
ttuttle@chromium.org, wtc@chromium.org

[arun87.kumar, 2014-10-06 04:53:30]

On 2014/10/06 04:40:32, Ryan Sleevi wrote:

Hi Ryan,

1. Matt NOT LGTM was by mistake. I mean he had pressed the wrong button

2. I have addressed eroman's points in the latest patch.


As of now, review is pending. And since i am new to Chromium, am not sure who
else can review this. 
So referred to OWNERS file and added the corresponding reviewers.


If you know somebody who can review this, please add them.

[mmenke, 2014-10-06 12:18:15]

On 2014/10/06 04:53:30, arun87.kumar wrote:
> On 2014/10/06 04:40:32, Ryan Sleevi wrote:
> 
> Hi Ryan,
> 
> 1. Matt NOT LGTM was by mistake. I mean he had pressed the wrong button
> 
> 2. I have addressed eroman's points in the latest patch.
> 
> 
> As of now, review is pending. And since i am new to Chromium, am not sure who
> else can review this. 
> So referred to OWNERS file and added the corresponding reviewers.
> 
> 
> If you know somebody who can review this, please add them.

It looks pretty clear to me that matt's initial signoff was by mistake,
actually, not his second response.

Once someone explicitly not L-G-T-M's a patch, you need that same person to
explicitly L-G-T-M before you can land it.

[wtc, 2014-10-06 18:21:00]

wtc@chromium.org changed reviewers:
+ wtc@chromium.org

[wtc, 2014-10-06 18:21:01]

Arun:

eroman is a good reviewer for this CL because I remember he was the original
author of these files. mmenke and rvargas are also good reviewers for this CL.

[arun87.kumar, 2014-10-07 03:39:00]

arun87.kumar@samsung.com changed reviewers:
+ mmenke@chromium.org, rvargas@chromium.org

[arun87.kumar, 2014-10-07 03:41:13]

On 2014/10/06 18:21:01, wtc wrote:

Hi wtc,

Thanks for your quick reply. I guess eroman is on leave. So wanted to add more
reviewers.

As per your suggestion, am adding mmenke and rvargas as reviewers.

[arun87.kumar, 2014-10-16 17:43:45]

eroman@ - <ping!>

PTAL.

[eroman, 2014-10-16 18:42:02]

I have lost confidence in this review.

 * The code doesn't meet the style guideline (wrong number of spaces various
places)

 * Didn't address my earlier feedback -- Won't numbers like "+34" be recognized
as valid when they shouldn't?

 * The logging isn't really relevant


Given that this change only solves something academically relevant (there isn't
support anywhere else in Chrome for multi-digit HTTP versions), I am having a
hard time prioritizing re-review of this changelist.

I appreciate the effort, but I would suggest maybe revisting this changelist
once you have some more Chromium experience.

Cheers.

[mmenke, 2014-11-20 19:34:40]

mmenke@chromium.org changed reviewers:
- mmenke@chromium.org

[rvargas (doing something else), 2014-11-21 19:46:39]

rvargas@chromium.org changed reviewers:
- rvargas@chromium.org

[lgombos, 2015-06-18 03:40:48]

Closing based on eroman's feedback and the fact that this work is progressing
here now - https://codereview.chromium.org/1129983003 .