Properly handle spdy3 responses.

Properly handle spdy3 responses.
Properly handle spdy3 to SPDY proxies.

BUG=120890
TEST=Run chrome with --enable-spdy3 and visit a spdy3 server

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

[Ryan Hamilton, 2012-03-30 19:46:03]

rtenneti: Please take a look.  I believe this fixes handling of spdy 3 responses
and spdy 3 proxies.

[ramant (doing other things), 2012-03-30 22:19:57]

LGTM. Mostly nits.

http://codereview.chromium.org/9958023/diff/3055/net/spdy/spdy_http_utils.cc
File net/spdy/spdy_http_utils.cc (right):

http://codereview.chromium.org/9958023/diff/3055/net/spdy/spdy_http_utils.cc#...
net/spdy/spdy_http_utils.cc:39: it = headers.find(version_key);
nit: Please consider deleting the comment or fixing it. thanks,

http://codereview.chromium.org/9958023/diff/3055/net/spdy/spdy_http_utils.cc#...
net/spdy/spdy_http_utils.cc:146: // SPDY 2 server push urls are specified in an
single "url" header.
nit: an single -> a single

http://codereview.chromium.org/9958023/diff/3055/net/spdy/spdy_http_utils.cc#...
net/spdy/spdy_http_utils.cc:148: // assemble from the response
nit: Consider "Assemble the response from headers." or no need to comment.

http://codereview.chromium.org/9958023/diff/3055/net/spdy/spdy_http_utils.h
File net/spdy/spdy_http_utils.h (right):

http://codereview.chromium.org/9958023/diff/3055/net/spdy/spdy_http_utils.h#n...
net/spdy/spdy_http_utils.h:22: // |info| output parameter for the
HttpResponseInfo.
nit: please fix |info| to |response|. thanks.

http://codereview.chromium.org/9958023/diff/3055/net/spdy/spdy_http_utils.h#n...
net/spdy/spdy_http_utils.h:37: GURL GetUrlFromHeaderBlock(const SpdyHeaderBlock&
headers,
nit: Would be great if we could have a comment for this method. "Extracts the
URL from the various fields in |headers|."

http://codereview.chromium.org/9958023/diff/3055/net/spdy/spdy_network_transa...
File net/spdy/spdy_network_transaction_spdy3_unittest.cc (right):

http://codereview.chromium.org/9958023/diff/3055/net/spdy/spdy_network_transa...
net/spdy/spdy_network_transaction_spdy3_unittest.cc:2708:
push(ConstructSpdyPush(extra_headers, arraysize(extra_headers)/2, 2, 1));
nit: " arraysize(extra_headers)/2" -> " arraysize(extra_headers) / 2"

http://codereview.chromium.org/9958023/diff/3055/net/spdy/spdy_proxy_client_s...
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/9958023/diff/3055/net/spdy/spdy_proxy_client_s...
net/spdy/spdy_proxy_client_socket.cc:478: return ERR_INCOMPLETE_SPDY_HEADERS;
Good catch.

http://codereview.chromium.org/9958023/diff/3055/net/spdy/spdy_test_util_spdy...
File net/spdy/spdy_test_util_spdy3.cc (right):

http://codereview.chromium.org/9958023/diff/3055/net/spdy/spdy_test_util_spdy...
net/spdy/spdy_test_util_spdy3.cc:481: ":status",  "200",
nit: Please consider deleting commented out code.

http://codereview.chromium.org/9958023/diff/3055/net/spdy/spdy_test_util_spdy...
net/spdy/spdy_test_util_spdy3.cc:519: 
nit: please consider making a common method for code in lines 351-377, 501-518,
553-570, 603-620.

http://codereview.chromium.org/9958023/diff/3055/net/spdy/spdy_test_util_spdy...
net/spdy/spdy_test_util_spdy3.cc:878: //next->first.length(),
nit: please consider deleting commented out code.

http://codereview.chromium.org/9958023/diff/2002/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/9958023/diff/2002/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:432: 
nit: extra line.

[Ryan Hamilton, 2012-03-30 22:46:02]

Thanks for the quick review!

http://codereview.chromium.org/9958023/diff/3055/net/spdy/spdy_http_utils.cc
File net/spdy/spdy_http_utils.cc (right):

http://codereview.chromium.org/9958023/diff/3055/net/spdy/spdy_http_utils.cc#...
net/spdy/spdy_http_utils.cc:39: it = headers.find(version_key);
On 2012/03/30 22:19:58, ramant wrote:
> nit: Please consider deleting the comment or fixing it. thanks,

Done.

http://codereview.chromium.org/9958023/diff/3055/net/spdy/spdy_http_utils.cc#...
net/spdy/spdy_http_utils.cc:146: // SPDY 2 server push urls are specified in an
single "url" header.
On 2012/03/30 22:19:58, ramant wrote:
> nit: an single -> a single

Done.

http://codereview.chromium.org/9958023/diff/3055/net/spdy/spdy_http_utils.cc#...
net/spdy/spdy_http_utils.cc:148: // assemble from the response
On 2012/03/30 22:19:58, ramant wrote:
> nit: Consider "Assemble the response from headers." or no need to comment.

Done.

http://codereview.chromium.org/9958023/diff/3055/net/spdy/spdy_http_utils.h
File net/spdy/spdy_http_utils.h (right):

http://codereview.chromium.org/9958023/diff/3055/net/spdy/spdy_http_utils.h#n...
net/spdy/spdy_http_utils.h:22: // |info| output parameter for the
HttpResponseInfo.
On 2012/03/30 22:19:58, ramant wrote:
> nit: please fix |info| to |response|. thanks.

Done.

http://codereview.chromium.org/9958023/diff/3055/net/spdy/spdy_http_utils.h#n...
net/spdy/spdy_http_utils.h:37: GURL GetUrlFromHeaderBlock(const SpdyHeaderBlock&
headers,
On 2012/03/30 22:19:58, ramant wrote:
> nit: Would be great if we could have a comment for this method. "Extracts the
> URL from the various fields in |headers|." 

Done.

http://codereview.chromium.org/9958023/diff/3055/net/spdy/spdy_network_transa...
File net/spdy/spdy_network_transaction_spdy3_unittest.cc (right):

http://codereview.chromium.org/9958023/diff/3055/net/spdy/spdy_network_transa...
net/spdy/spdy_network_transaction_spdy3_unittest.cc:2708:
push(ConstructSpdyPush(extra_headers, arraysize(extra_headers)/2, 2, 1));
On 2012/03/30 22:19:58, ramant wrote:
> nit: " arraysize(extra_headers)/2" -> " arraysize(extra_headers) / 2"

Done.

http://codereview.chromium.org/9958023/diff/3055/net/spdy/spdy_proxy_client_s...
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/9958023/diff/3055/net/spdy/spdy_proxy_client_s...
net/spdy/spdy_proxy_client_socket.cc:478: return ERR_INCOMPLETE_SPDY_HEADERS;
On 2012/03/30 22:19:58, ramant wrote:
> Good catch.

Thanks!  I managed to hit the bug when I had this CL in an inconsistent state.

http://codereview.chromium.org/9958023/diff/3055/net/spdy/spdy_test_util_spdy...
File net/spdy/spdy_test_util_spdy3.cc (right):

http://codereview.chromium.org/9958023/diff/3055/net/spdy/spdy_test_util_spdy...
net/spdy/spdy_test_util_spdy3.cc:481: ":status",  "200",
On 2012/03/30 22:19:58, ramant wrote:
> nit: Please consider deleting commented out code.

Whoops, sorry about that!

http://codereview.chromium.org/9958023/diff/3055/net/spdy/spdy_test_util_spdy...
net/spdy/spdy_test_util_spdy3.cc:519: 
On 2012/03/30 22:19:58, ramant wrote:
> nit: please consider making a common method for code in lines 351-377,
501-518,
> 553-570, 603-620. 

Agreed.  I'll cook up a CL to do just that.

http://codereview.chromium.org/9958023/diff/3055/net/spdy/spdy_test_util_spdy...
net/spdy/spdy_test_util_spdy3.cc:878: //next->first.length(),
On 2012/03/30 22:19:58, ramant wrote:
> nit: please consider deleting commented out code.

Done.

http://codereview.chromium.org/9958023/diff/2002/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/9958023/diff/2002/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:432: 
On 2012/03/30 22:19:58, ramant wrote:
> nit: extra line.

Done.

[ramant (doing other things), 2012-03-30 22:48:07]

lgtm

[commit-bot: I haz the power, 2012-04-03 17:52:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/9958023/5021

[commit-bot: I haz the power, 2012-04-03 17:55:28]

Try job failure for 9958023-5021 on linux_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-04-03 18:06:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/9958023/5023

[commit-bot: I haz the power, 2012-04-03 18:45:58]

Try job failure for 9958023-5023 (retry) on mac_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...