Make URLRequestTestJob accept \n terminated header lines.

Make URLRequestTestJob accept \n terminated header lines.

BUG=516724
Committed: https://crrev.com/f96ddc73b0344e55efbe0cf56597e1e7172db9c4
Cr-Commit-Position: refs/heads/master@{#356617}

[martijnc, 2015-10-20 20:03:26]

Description was changed from

==========
Make URLRequestTestJob accept \n terminated headers.

BUG=516724
==========

to

==========
Make URLRequestTestJob accept \n terminated header lines.

BUG=516724
==========

[martijnc, 2015-10-20 20:10:57]

Patchset #1 (id:1) has been deleted

[Peter Beverloo, 2015-10-20 21:31:32]

The CQ bit was checked by peter@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-20 21:33:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414983002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414983002/20001

[commit-bot: I haz the power, 2015-10-20 22:10:26]

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

[commit-bot: I haz the power, 2015-10-20 22:10:26]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[martijnc, 2015-10-25 21:00:48]

martijn@martijnc.be changed reviewers:
+ jam@chromium.org
- peter@chromium.org

[martijnc, 2015-10-25 21:05:30]

Hi, can you review this patch? Thanks!

[jam, 2015-10-26 17:20:00]

you should get someone from /net/OWNERS to review.

[martijnc, 2015-10-26 17:23:02]

martijn@martijnc.be changed reviewers:
+ mmenke@chromium.org
- jam@chromium.org

[martijnc, 2015-10-26 17:24:30]

@mmenke: Can you take a look? Thanks!

[mmenke, 2015-10-27 16:17:58]

On 2015/10/26 17:24:30, Martijn Croonen wrote:
> @mmenke: Can you take a look? Thanks!

Hrm...  I didn't get an email about this.  Did you post the message after adding
me to the reviewer list?  Anyhow, I'll get to this later today.

[martijnc, 2015-10-27 16:49:16]

On 2015/10/27 at 16:17:58, mmenke wrote:
> On 2015/10/26 17:24:30, Martijn Croonen wrote:
> > @mmenke: Can you take a look? Thanks!
> 
> Hrm...  I didn't get an email about this.  Did you post the message after
adding me to the reviewer list?  Anyhow, I'll get to this later today.

Yes, I first added you as a reviewer (and removed jam) and then posted that
message. I did use 'Publish' to post that message, should I use reply instead?

[mmenke, 2015-10-27 16:49:31]

LGTM, just some minor comments.

https://codereview.chromium.org/1414983002/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats_unittest.cc
(right):

https://codereview.chromium.org/1414983002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats_unittest.cc:79:
const std::string& raw_response_headers) {
nit:  Remove the "raw".  It's currently used to mean the version with \0's that
AssembleRawHeaders creates.

https://codereview.chromium.org/1414983002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats_unittest.cc:413:
std::string raw_headers(test_cases[i].headers);
nit:  Can just inline test_cases[i].headers below.

https://codereview.chromium.org/1414983002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats_unittest.cc:480:
"Via: 1.1 Chrome-Compression-Proxy\n");
nit:  Remove "raw" (I suggest adding response instead, to match
CreateURLRequestWithResponseHeaders's prototype, but that's optional).

https://codereview.chromium.org/1414983002/diff/40001/content/browser/appcach...
File content/browser/appcache/appcache_update_job_unittest.cc (right):

https://codereview.chromium.org/1414983002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_update_job_unittest.cc:439: "\r\n";
\n for consistency?

https://codereview.chromium.org/1414983002/diff/40001/net/url_request/url_req...
File net/url_request/url_request_test_job.cc (right):

https://codereview.chromium.org/1414983002/diff/40001/net/url_request/url_req...
net/url_request/url_request_test_job.cc:160: response_headers.c_str(),
response_headers.size()));
nit:  Can still do this in the initializer list.

[mmenke, 2015-10-27 16:51:20]

On 2015/10/27 16:49:16, Martijn Croonen wrote:
> On 2015/10/27 at 16:17:58, mmenke wrote:
> > On 2015/10/26 17:24:30, Martijn Croonen wrote:
> > > @mmenke: Can you take a look? Thanks!
> > 
> > Hrm...  I didn't get an email about this.  Did you post the message after
> adding me to the reviewer list?  Anyhow, I'll get to this later today.
> 
> Yes, I first added you as a reviewer (and removed jam) and then posted that
> message. I did use 'Publish' to post that message, should I use reply instead?

Hrm...That should have emailed me.  I'd chalk that up to a Rietveld bug - not a
big issue, just wanted to make sure, if the mistake was on your end, you knew
you should add reviewers first.

[martijnc, 2015-10-27 21:10:15]

martijn@martijnc.be changed reviewers:
+ jochen@chromium.org

[martijnc, 2015-10-27 21:10:16]

Thanks you for the review.

@jochen; can you take a look too? I think I'm missing an OWNER for some of these
files now.

https://codereview.chromium.org/1414983002/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats_unittest.cc
(right):

https://codereview.chromium.org/1414983002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats_unittest.cc:79:
const std::string& raw_response_headers) {
On 2015/10/27 16:49:31, mmenke wrote:
> nit:  Remove the "raw".  It's currently used to mean the version with \0's
that
> AssembleRawHeaders creates.

Done.

https://codereview.chromium.org/1414983002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats_unittest.cc:413:
std::string raw_headers(test_cases[i].headers);
On 2015/10/27 16:49:31, mmenke wrote:
> nit:  Can just inline test_cases[i].headers below.

Done.

https://codereview.chromium.org/1414983002/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_bypass_stats_unittest.cc:480:
"Via: 1.1 Chrome-Compression-Proxy\n");
On 2015/10/27 16:49:31, mmenke wrote:
> nit:  Remove "raw" (I suggest adding response instead, to match
> CreateURLRequestWithResponseHeaders's prototype, but that's optional).

Done.

https://codereview.chromium.org/1414983002/diff/40001/content/browser/appcach...
File content/browser/appcache/appcache_update_job_unittest.cc (right):

https://codereview.chromium.org/1414983002/diff/40001/content/browser/appcach...
content/browser/appcache/appcache_update_job_unittest.cc:439: "\r\n";
On 2015/10/27 16:49:31, mmenke wrote:
> \n for consistency?

Done.

https://codereview.chromium.org/1414983002/diff/40001/net/url_request/url_req...
File net/url_request/url_request_test_job.cc (right):

https://codereview.chromium.org/1414983002/diff/40001/net/url_request/url_req...
net/url_request/url_request_test_job.cc:160: response_headers.c_str(),
response_headers.size()));
On 2015/10/27 16:49:31, mmenke wrote:
> nit:  Can still do this in the initializer list.

Done.

[jochen (gone - plz use gerrit), 2015-10-28 14:19:46]

lgtm

[martijnc, 2015-10-28 18:26:31]

The CQ bit was checked by martijn@martijnc.be

[martijnc, 2015-10-28 18:26:31]

The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1414983002/#ps60001
(title: "Comments mmenke")

[commit-bot: I haz the power, 2015-10-28 18:27:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414983002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414983002/60001

[commit-bot: I haz the power, 2015-10-28 19:41:23]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2015-10-28 19:42:01]

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

[mmenke, 2015-10-28 19:44:35]

Forgot to say it earlier, but thanks for tackling this, Martijn!