Net: Standardize HttpByteRange printing

Net: Standardize HttpByteRange printing

BUG=318450
TBR=benm@chromium.org,sky@chromium.org

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

[tommycli, 2013-11-21 20:35:28]

thestig: This standardizes the printing, but seems to increase the SLOC count by
quite a bit.

Do you think it's worth it?

[tommycli, 2013-11-21 20:36:01]

I did this because I need to print an HttpByteRange in the metadata code also.

[Lei Zhang, 2013-11-21 20:53:22]

I'm not really the right reviewer for this code.

[tommycli, 2013-11-21 21:04:31]

hclam: I added some standardized http byte range printing, as I need to do this
and I see printing logic scattered in a few places in the codebase.

Let me know if you think this is a worthwhile change.

[Alpha Left Google, 2013-11-21 22:21:31]

On 2013/11/21 21:04:31, tommycli wrote:
> hclam: I added some standardized http byte range printing, as I need to do
this
> and I see printing logic scattered in a few places in the codebase.
> 
> Let me know if you think this is a worthwhile change.

It was a long time ago that I touched this code. And I don't think I'm familiar
with the current code to review this. Please find rvargas@ or cbentzel@ for
review.

[tommycli, 2013-11-21 22:27:53]

cbentzel: Need OWNER review

[cbentzel, 2013-11-22 16:24:34]

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

https://codereview.chromium.org/78343004/diff/110001/net/http/http_util.h#new...
net/http/http_util.h:74: static std::string PrintRange(const HttpByteRange&
range);
Remove PrintRanges and place it in the only test file which uses it.

Move PrintRange over to HttpByteRange and remove this usage. No callers used
PrintInterval that I saw, and the byte units are really part of the header.

[tommycli, 2013-11-22 20:24:10]

cbentzel: Addressed your issues.

[cbentzel, 2013-11-23 14:37:20]

LGTM

Thanks for the cleanup!

I'll leave it up to you to decide whether you want to change the method name
(see the nit below).

I'm going to be on vacation next week so no need to wait for any more review
comments from me.

https://codereview.chromium.org/78343004/diff/300002/net/http/http_byte_range.h
File net/http/http_byte_range.h (right):

https://codereview.chromium.org/78343004/diff/300002/net/http/http_byte_range...
net/http/http_byte_range.h:51: std::string PrintHeader() const;
I'd rather call this GetHeaderValue or GenerateHeader rather than PrintHeader()
since Print seems to indicate that the text will output somewhere (a printer,
console, or screen).

[tommycli, 2013-11-25 19:00:04]

https://codereview.chromium.org/78343004/diff/300002/net/http/http_byte_range.h
File net/http/http_byte_range.h (right):

https://codereview.chromium.org/78343004/diff/300002/net/http/http_byte_range...
net/http/http_byte_range.h:51: std::string PrintHeader() const;
On 2013/11/23 14:37:20, cbentzel wrote:
> I'd rather call this GetHeaderValue or GenerateHeader rather than
PrintHeader()
> since Print seems to indicate that the text will output somewhere (a printer,
> console, or screen).

Done.

[tommycli, 2013-11-25 19:07:08]

scherkus: Need a review on content/renderer stuff.

[scherkus (not reviewing), 2013-11-25 22:53:09]

lgtm w/ nits

https://codereview.chromium.org/78343004/diff/490001/content/renderer/media/b...
File content/renderer/media/buffered_resource_loader.cc (right):

https://codereview.chromium.org/78343004/diff/490001/content/renderer/media/b...
content/renderer/media/buffered_resource_loader.cc:13: #include
"base/strings/stringprintf.h"
can we remove some of these string #includes?

https://codereview.chromium.org/78343004/diff/490001/net/http/http_byte_range.cc
File net/http/http_byte_range.cc (right):

https://codereview.chromium.org/78343004/diff/490001/net/http/http_byte_range...
net/http/http_byte_range.cc:75: } else if (HasLastBytePosition()) {
nit: you don't really need these elses

[tommycli, 2013-11-25 23:05:58]

https://codereview.chromium.org/78343004/diff/490001/content/renderer/media/b...
File content/renderer/media/buffered_resource_loader.cc (right):

https://codereview.chromium.org/78343004/diff/490001/content/renderer/media/b...
content/renderer/media/buffered_resource_loader.cc:13: #include
"base/strings/stringprintf.h"
On 2013/11/25 22:53:10, scherkus wrote:
> can we remove some of these string #includes?

Done.

https://codereview.chromium.org/78343004/diff/490001/net/http/http_byte_range.cc
File net/http/http_byte_range.cc (right):

https://codereview.chromium.org/78343004/diff/490001/net/http/http_byte_range...
net/http/http_byte_range.cc:75: } else if (HasLastBytePosition()) {
On 2013/11/25 22:53:10, scherkus wrote:
> nit: you don't really need these elses

Done.

[scherkus (not reviewing), 2013-11-25 23:06:46]

lgtm++

thanks for the cleanup!

[commit-bot: I haz the power, 2013-11-26 00:29:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/78343004/510001

[commit-bot: I haz the power, 2013-11-26 01:09:17]

Retried try job too often on mac_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-11-26 18:40:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/78343004/510001

[commit-bot: I haz the power, 2013-11-26 20:16:40]

Change committed as 237400