Implement server push protocol 2.

Implement server push protocol 2.
TEST=net_unittests
BUG=34761
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=55095

[erikchen, 2010-07-26 23:08:16]

Server push protocol 2.

[Mike Belshe, 2010-07-27 00:40:22]

http://codereview.chromium.org/3020032/diff/2001/3002
File net/spdy/spdy_network_transaction_unittest.cc (right):

http://codereview.chromium.org/3020032/diff/2001/3002#newcode1412
net/spdy/spdy_network_transaction_unittest.cc:1412: }
A couple ideas for more tests:

* I wonder if we should add a test to verify that you can push non-200 response
codes through the transaction.  E.g. a cacheable redirect.

* Test to verify that cacheable/non-cacheable responses are appropriately
cached.  These should behave just like other streams.

http://codereview.chromium.org/3020032/diff/2001/3002#newcode1436
net/spdy/spdy_network_transaction_unittest.cc:1436: arraysize(kPushBodyFrame1),
4),
the comments are cut-and-paste

http://codereview.chromium.org/3020032/diff/2001/3003
File net/spdy/spdy_session.cc (left):

http://codereview.chromium.org/3020032/diff/2001/3003#oldcode912
net/spdy/spdy_session.cc:912: pushed_streams_.erase(it);
why is the stream no longer removed from the unclaimed_pushed_streams?

http://codereview.chromium.org/3020032/diff/2001/3003#oldcode913
net/spdy/spdy_session.cc:913: used_push_streams.Increment();
looks like you dropped the used_push_streams.Increment()

http://codereview.chromium.org/3020032/diff/2001/3003
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/3020032/diff/2001/3003#newcode459
net/spdy/spdy_session.cc:459: // Set the priority to highest unless the stream
exists and has other
This statement reads a little funny.  3 is the "highest number", but the "lowest
priority".

/Set the priority to highest/Set to lowest priority/

http://codereview.chromium.org/3020032/diff/2001/3003#newcode884
net/spdy/spdy_session.cc:884: LOG(INFO) << "Push stream: " << path << " found.";
might be nice to do something like this here:
net_log_.AddEvent(NetLog::TYPE_SPDY_STREAM_CLAIMED_PUSH_STREAM, NULL);

http://codereview.chromium.org/3020032/diff/2001/3003#newcode984
net/spdy/spdy_session.cc:984: << associated_stream_id;
Shouldn't this case need to ResetStream()?  (and test case?)

http://codereview.chromium.org/3020032/diff/2001/3003#newcode1000
net/spdy/spdy_session.cc:1000: // TODO(erikchen): add this stream to that of
associated id.
I don't understand the comment in 1000.

http://codereview.chromium.org/3020032/diff/2001/3003#newcode1003
net/spdy/spdy_session.cc:1003: // Overwrite existing pushed_stream, if it
exists.
Good comment here - shouldn't this be an error case that we error explicitly? 
Otherwise, we've leaked a stream?

http://codereview.chromium.org/3020032/diff/2001/3005
File net/spdy/spdy_session_unittest.cc (right):

http://codereview.chromium.org/3020032/diff/2001/3005#newcode199
net/spdy/spdy_session_unittest.cc:199: EXPECT_EQ(0, second_stream->priority());
From looking at the code, I thought the pushed stream had priority 3 (see
spdy_test_util.cc).  Maybe I misread.

http://codereview.chromium.org/3020032/diff/2001/3007
File net/spdy/spdy_test_util.cc (right):

http://codereview.chromium.org/3020032/diff/2001/3007#newcode343
net/spdy/spdy_test_util.cc:343: 
nit: blank lines

[cbentzel, 2010-07-27 00:57:27]

I added Matthew Lloyd, who uncovered a number of the issues with the initial
implementation of server push.

[erikchen, 2010-08-03 21:51:09]

http://codereview.chromium.org/3020032/diff/2001/3002
File net/spdy/spdy_network_transaction_unittest.cc (right):

http://codereview.chromium.org/3020032/diff/2001/3002#newcode1412
net/spdy/spdy_network_transaction_unittest.cc:1412: }


On 2010/07/27 00:40:22, Mike Belshe wrote:
> A couple ideas for more tests:
> 
> * I wonder if we should add a test to verify that you can push non-200
response
> codes through the transaction.  E.g. a cacheable redirect.
Test added.

> 
> * Test to verify that cacheable/non-cacheable responses are appropriately
> cached.  These should behave just like other streams.

http://codereview.chromium.org/3020032/diff/2001/3002#newcode1436
net/spdy/spdy_network_transaction_unittest.cc:1436: arraysize(kPushBodyFrame1),
4),
On 2010/07/27 00:40:22, Mike Belshe wrote:
> the comments are cut-and-paste
> 

Done.

http://codereview.chromium.org/3020032/diff/2001/3003
File net/spdy/spdy_session.cc (left):

http://codereview.chromium.org/3020032/diff/2001/3003#oldcode912
net/spdy/spdy_session.cc:912: pushed_streams_.erase(it);
On 2010/07/27 00:40:22, Mike Belshe wrote:
> why is the stream no longer removed from the unclaimed_pushed_streams?

Done.

http://codereview.chromium.org/3020032/diff/2001/3003#oldcode913
net/spdy/spdy_session.cc:913: used_push_streams.Increment();
On 2010/07/27 00:40:22, Mike Belshe wrote:
> looks like you dropped the used_push_streams.Increment()
> 

Done.

http://codereview.chromium.org/3020032/diff/2001/3003
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/3020032/diff/2001/3003#newcode459
net/spdy/spdy_session.cc:459: // Set the priority to highest unless the stream
exists and has other
On 2010/07/27 00:40:22, Mike Belshe wrote:
> This statement reads a little funny.  3 is the "highest number", but the
"lowest
> priority".
> 
> /Set the priority to highest/Set to lowest priority/

Done.

http://codereview.chromium.org/3020032/diff/2001/3003#newcode884
net/spdy/spdy_session.cc:884: LOG(INFO) << "Push stream: " << path << " found.";
On 2010/07/27 00:40:22, Mike Belshe wrote:
> might be nice to do something like this here:
> net_log_.AddEvent(NetLog::TYPE_SPDY_STREAM_CLAIMED_PUSH_STREAM, NULL);

Done.

http://codereview.chromium.org/3020032/diff/2001/3003#newcode984
net/spdy/spdy_session.cc:984: << associated_stream_id;
On 2010/07/27 00:40:22, Mike Belshe wrote:
> Shouldn't this case need to ResetStream()?  (and test case?)

Done.

http://codereview.chromium.org/3020032/diff/2001/3003#newcode1000
net/spdy/spdy_session.cc:1000: // TODO(erikchen): add this stream to that of
associated id.
On 2010/07/27 00:40:22, Mike Belshe wrote:
> I don't understand the comment in 1000.

Done.

http://codereview.chromium.org/3020032/diff/2001/3003#newcode1003
net/spdy/spdy_session.cc:1003: // Overwrite existing pushed_stream, if it
exists.
On 2010/07/27 00:40:22, Mike Belshe wrote:
> Good comment here - shouldn't this be an error case that we error explicitly? 
> Otherwise, we've leaked a stream?

Done.

http://codereview.chromium.org/3020032/diff/2001/3005
File net/spdy/spdy_session_unittest.cc (right):

http://codereview.chromium.org/3020032/diff/2001/3005#newcode199
net/spdy/spdy_session_unittest.cc:199: EXPECT_EQ(0, second_stream->priority());
It only has priority 3 if the stream doesn't already have a priority (or the
stream doesn't exist, etc.)
On 2010/07/27 00:40:22, Mike Belshe wrote:
> From looking at the code, I thought the pushed stream had priority 3 (see
> spdy_test_util.cc).  Maybe I misread.

http://codereview.chromium.org/3020032/diff/2001/3007
File net/spdy/spdy_test_util.cc (right):

http://codereview.chromium.org/3020032/diff/2001/3007#newcode343
net/spdy/spdy_test_util.cc:343: 
On 2010/07/27 00:40:22, Mike Belshe wrote:
> nit: blank lines
> 

Done.

[Mike Belshe, 2010-08-05 17:42:48]

lgtm with nits

http://codereview.chromium.org/3020032/diff/27001/28002
File net/http/http_network_layer.h (right):

http://codereview.chromium.org/3020032/diff/27001/28002#newcode52
net/http/http_network_layer.h:52: explicit HttpNetworkLayer(HttpNetworkSession*
session);
style: spacing.

http://codereview.chromium.org/3020032/diff/27001/28004
File net/spdy/spdy_network_transaction_unittest.cc (right):

http://codereview.chromium.org/3020032/diff/27001/28004#newcode289
net/spdy/spdy_network_transaction_unittest.cc:289: HttpResponseInfo* response,
HttpResponseInfo* response2,
nit: spacing :-)

http://codereview.chromium.org/3020032/diff/27001/28004#newcode363
net/spdy/spdy_network_transaction_unittest.cc:363: ::testing::Values(SPDYNOSSL,
SPDYSSL, SPDYNPN));
accidental change?

http://codereview.chromium.org/3020032/diff/27001/28010
File net/spdy/spdy_stream.h (right):

http://codereview.chromium.org/3020032/diff/27001/28010#newcode217
net/spdy/spdy_stream.h:217: // There is a small period of time between when a
server pushed stream is
nit: add blank line between 217/218

http://codereview.chromium.org/3020032/diff/27001/28011
File net/spdy/spdy_test_util.cc (right):

http://codereview.chromium.org/3020032/diff/27001/28011#newcode740
net/spdy/spdy_test_util.cc:740: type,             // Kind = Syn
nit: spacing alignment  on the comment

http://codereview.chromium.org/3020032/diff/27001/28013
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/3020032/diff/27001/28013#newcode40
net/url_request/url_request_unittest.cc:40: #include
"net/http/http_network_transaction.h"
nit: gratuitous?

[erikchen, 2010-08-05 17:57:23]

http://codereview.chromium.org/3020032/diff/27001/28002
File net/http/http_network_layer.h (right):

http://codereview.chromium.org/3020032/diff/27001/28002#newcode52
net/http/http_network_layer.h:52: explicit HttpNetworkLayer(HttpNetworkSession*
session);
On 2010/08/05 17:42:48, Mike Belshe wrote:
> style: spacing.

Done.

http://codereview.chromium.org/3020032/diff/27001/28004
File net/spdy/spdy_network_transaction_unittest.cc (right):

http://codereview.chromium.org/3020032/diff/27001/28004#newcode289
net/spdy/spdy_network_transaction_unittest.cc:289: HttpResponseInfo* response,
HttpResponseInfo* response2,
On 2010/08/05 17:42:48, Mike Belshe wrote:
> nit: spacing :-)

Done.

http://codereview.chromium.org/3020032/diff/27001/28004#newcode363
net/spdy/spdy_network_transaction_unittest.cc:363: ::testing::Values(SPDYNOSSL,
SPDYSSL, SPDYNPN));
On 2010/08/05 17:42:48, Mike Belshe wrote:
> accidental change?

Just wanted to make the order match the order of the switch statement above.

http://codereview.chromium.org/3020032/diff/27001/28010
File net/spdy/spdy_stream.h (right):

http://codereview.chromium.org/3020032/diff/27001/28010#newcode217
net/spdy/spdy_stream.h:217: // There is a small period of time between when a
server pushed stream is
On 2010/08/05 17:42:48, Mike Belshe wrote:
> nit: add blank line between 217/218

Done.

http://codereview.chromium.org/3020032/diff/27001/28011
File net/spdy/spdy_test_util.cc (right):

http://codereview.chromium.org/3020032/diff/27001/28011#newcode740
net/spdy/spdy_test_util.cc:740: type,             // Kind = Syn
On 2010/08/05 17:42:48, Mike Belshe wrote:
> nit: spacing alignment  on the comment

Done.

http://codereview.chromium.org/3020032/diff/27001/28013
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/3020032/diff/27001/28013#newcode40
net/url_request/url_request_unittest.cc:40: #include
"net/http/http_network_transaction.h"
On 2010/08/05 17:42:48, Mike Belshe wrote:
> nit: gratuitous?

Done.