net: remove GetNormalizedHeaders() function

net: remove GetNormalizedHeaders() function

It is only used by some logging and test functions. It should be safe to
replace those callers with ConvertHeadersBackToHTTPResponse().

This cleans up Darin's TODO.

BUG=488593
TEST=net_unittests
R=eroman@chromium.org,rogerta@chromium.org,sky@chromium.org

Review-Url: https://codereview.chromium.org/2670013002
Cr-Commit-Position: refs/heads/master@{#451455}
Committed: https://chromium.googlesource.com/chromium/src/+/ff7fdbb297f5430b4fa34ececfb736743c4d9994

[tfarina, 2017-02-02 02:21:08]

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

[commit-bot: I haz the power, 2017-02-02 02:21:28]

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

[eroman, 2017-02-02 03:14:12]

Looks good once http_response_headers_unittest.cc is updated!

https://codereview.chromium.org/2670013002/diff/1/google_apis/drive/base_requ...
File google_apis/drive/base_requests.cc (right):

https://codereview.chromium.org/2670013002/diff/1/google_apis/drive/base_requ...
google_apis/drive/base_requests.cc:104:
headers.assign(net::HttpUtil::ConvertHeadersBackToHTTPResponse(
Please use assignment here instead:
  headers = ConvertHeadersBackToXXX(...);

[commit-bot: I haz the power, 2017-02-02 03:17:08]

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

[commit-bot: I haz the power, 2017-02-02 03:17:09]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[tfarina, 2017-02-13 10:29:30]

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

[commit-bot: I haz the power, 2017-02-13 10:29:42]

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

[tfarina, 2017-02-13 10:30:27]

On 2017/02/02 03:14:12, eroman (slow) wrote:
> Looks good once http_response_headers_unittest.cc is updated!
> 
Eric, could you help me with that?

I'm looking at fixing the http_cache tests as well.

https://codereview.chromium.org/2670013002/diff/1/google_apis/drive/base_requ...
File google_apis/drive/base_requests.cc (right):

https://codereview.chromium.org/2670013002/diff/1/google_apis/drive/base_requ...
google_apis/drive/base_requests.cc:104:
headers.assign(net::HttpUtil::ConvertHeadersBackToHTTPResponse(
On 2017/02/02 03:14:12, eroman (slow) wrote:
> Please use assignment here instead:
>   headers = ConvertHeadersBackToXXX(...);

Done.

[commit-bot: I haz the power, 2017-02-13 10:55:38]

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

[commit-bot: I haz the power, 2017-02-13 10:55:39]

Dry run: 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...)

[eroman, 2017-02-13 20:49:17]

Sure, I will send a CL to update the http_response_headers_unittests

[tfarina, 2017-02-14 11:26:52]

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

[commit-bot: I haz the power, 2017-02-14 11:27:06]

Dry run: 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, 2017-02-14 11:32:15]

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

[commit-bot: I haz the power, 2017-02-14 11:32:16]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  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...)

[tfarina, 2017-02-14 21:42:47]

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

[commit-bot: I haz the power, 2017-02-14 21:43:49]

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

[tfarina, 2017-02-14 22:00:55]

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

[commit-bot: I haz the power, 2017-02-14 22:01:48]

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

[tfarina, 2017-02-14 22:11:30]

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

[commit-bot: I haz the power, 2017-02-14 22:12:02]

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

[tfarina, 2017-02-14 22:13:18]

Hi Eric, could you take another look?

https://codereview.chromium.org/2670013002/diff/160001/net/http/http_cache_un...
File net/http/http_cache_unittest.cc (right):

https://codereview.chromium.org/2670013002/diff/160001/net/http/http_cache_un...
net/http/http_cache_unittest.cc:83: std::string ToSimpleString(const
scoped_refptr<HttpResponseHeaders>& parsed) {
Sorry, I had to steal your function. Maybe we can come up with some simpler?

[commit-bot: I haz the power, 2017-02-14 23:46:36]

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

[commit-bot: I haz the power, 2017-02-14 23:46:36]

Dry run: This issue passed the CQ dry run.

[eroman, 2017-02-16 20:37:35]

lgtm

https://codereview.chromium.org/2670013002/diff/160001/net/http/http_cache_un...
File net/http/http_cache_unittest.cc (right):

https://codereview.chromium.org/2670013002/diff/160001/net/http/http_cache_un...
net/http/http_cache_unittest.cc:83: std::string ToSimpleString(const
scoped_refptr<HttpResponseHeaders>& parsed) {
On 2017/02/14 22:13:18, tfarina wrote:
> Sorry, I had to steal your function. Maybe we can come up with some simpler?

Can you add a TODO explaining this is a duplicate of the other function?

[tfarina, 2017-02-16 22:17:37]

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

[tfarina, 2017-02-16 22:18:01]

tfarina@chromium.org changed reviewers:
+ rogerta@chromium.org, sky@chromium.org

[tfarina, 2017-02-16 22:18:02]

Adding owners:

Scott -> content
Roger -> google_apis

https://codereview.chromium.org/2670013002/diff/160001/net/http/http_cache_un...
File net/http/http_cache_unittest.cc (right):

https://codereview.chromium.org/2670013002/diff/160001/net/http/http_cache_un...
net/http/http_cache_unittest.cc:83: std::string ToSimpleString(const
scoped_refptr<HttpResponseHeaders>& parsed) {
On 2017/02/16 20:37:35, eroman wrote:
> On 2017/02/14 22:13:18, tfarina wrote:
> > Sorry, I had to steal your function. Maybe we can come up with some simpler?
> 
> Can you add a TODO explaining this is a duplicate of the other function?

Done.

[commit-bot: I haz the power, 2017-02-16 22:19:32]

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

[Roger Tawa OOO till Jul 10th, 2017-02-16 22:27:42]

Comments in the function being removed mention being careful about spacing and
commas and such, which I don't see in the replacement:

https://cs.chromium.org/chromium/src/net/http/http_util.cc?rcl=1033be6d096d43...

but if you say they are equivalent then google_apis lgtm

[eroman, 2017-02-16 22:43:46]

The new approach that Thiago is using does not have those same limitations, so
the comment is no longer applicable.

Previously, GetNormalizedHeaders() would try to coalesce headers with the same
name. For instance if the original headers (raw_headers) had been:

   Set-Cookie:    a
   Set-Cookie:  b

Then GetNormalizedHeaders() would have (incorrectly) returned:

   Set-Cookie: a, b

Whereas after this change, the headers string that will be logged is the same
two Set-Cookie lines as the actual headers included (just without the
superfluous spaces):

   Set-Cookie: a
   Set-Cookie: b

In this case what GetNormalizedHeaders() was returning was wrong as it has a
different semantic meaning. Set-Cookie is expected to be a human-readable date
string which may in itself contain commas, so combining it like this was what
the "Danger" was referring to. (Combining repeated headers with a comma does
make sense for other header types like Cache-Control).

The new approach proposed by this CL preserves the source headers and values
without trying to combine repeated headers. For the sake of logging in
google_apis I believe this is an overall better choice.

[eroman, 2017-02-16 22:45:47]

(Small correction, the whitespace would be preserved too)

[commit-bot: I haz the power, 2017-02-17 01:37:58]

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

[commit-bot: I haz the power, 2017-02-17 01:38:00]

Dry run: This issue passed the CQ dry run.

[Roger Tawa OOO till Jul 10th, 2017-02-17 14:55:29]

Thanks for the explanation Eric.  Still lgtm

[sky, 2017-02-17 19:27:55]

LGTM

[tfarina, 2017-02-17 20:50:19]

The CQ bit was checked by tfarina@chromium.org

[tfarina, 2017-02-17 20:50:20]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2670013002/#ps200001
(title: "n")

[commit-bot: I haz the power, 2017-02-17 20:50:56]

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

[tfarina, 2017-02-17 20:51:05]

Description was changed from

==========
net: remove GetNormalizedHeaders() function

It is only used by some logging and test functions. It should be safe to
replace those callers with ConvertHeadersBackToHTTPResponse().

This cleans up Darin's TODO.

BUG=488593
TEST=net_unittests
R=eroman@chromium.org
==========

to

==========
net: remove GetNormalizedHeaders() function

It is only used by some logging and test functions. It should be safe to
replace those callers with ConvertHeadersBackToHTTPResponse().

This cleans up Darin's TODO.

BUG=488593
TEST=net_unittests
R=eroman@chromium.org,rogerta@chromium.org,sky@chromium.org
==========

[commit-bot: I haz the power, 2017-02-17 22:54:48]

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

[commit-bot: I haz the power, 2017-02-17 22:54:50]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[tfarina, 2017-02-17 22:57:48]

The CQ bit was checked by tfarina@chromium.org

[commit-bot: I haz the power, 2017-02-17 22:59:42]

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, 2017-02-18 01:11:01]

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

[commit-bot: I haz the power, 2017-02-18 01:11:03]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)

[tfarina, 2017-02-18 01:18:50]

The CQ bit was checked by tfarina@chromium.org

[commit-bot: I haz the power, 2017-02-18 01:20:37]

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, 2017-02-18 01:39:09]

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

[commit-bot: I haz the power, 2017-02-18 01:39:11]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[tfarina, 2017-02-18 10:53:08]

The CQ bit was checked by tfarina@chromium.org

[commit-bot: I haz the power, 2017-02-18 10:53:18]

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, 2017-02-18 11:40:39]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1487415188168990,
"parent_rev": "5cd9ae8889c732428043460caa5d0fd8043041c1", "commit_rev":
"ff7fdbb297f5430b4fa34ececfb736743c4d9994"}

[commit-bot: I haz the power, 2017-02-18 11:41:16]

Description was changed from

==========
net: remove GetNormalizedHeaders() function

It is only used by some logging and test functions. It should be safe to
replace those callers with ConvertHeadersBackToHTTPResponse().

This cleans up Darin's TODO.

BUG=488593
TEST=net_unittests
R=eroman@chromium.org,rogerta@chromium.org,sky@chromium.org
==========

to

==========
net: remove GetNormalizedHeaders() function

It is only used by some logging and test functions. It should be safe to
replace those callers with ConvertHeadersBackToHTTPResponse().

This cleans up Darin's TODO.

BUG=488593
TEST=net_unittests
R=eroman@chromium.org,rogerta@chromium.org,sky@chromium.org

Review-Url: https://codereview.chromium.org/2670013002
Cr-Commit-Position: refs/heads/master@{#451455}
Committed:
https://chromium.googlesource.com/chromium/src/+/ff7fdbb297f5430b4fa34ececfb7...
==========

[commit-bot: I haz the power, 2017-02-18 11:41:17]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/ff7fdbb297f5430b4fa34ececfb7...