Implement MAX_CONCURRENT_STREAMS SETTINGS header

Implement MAX_CONCURRENT_STREAMS SETTINGS header

This CL helps chrome respect the SETTINGS header
MAX_CONCURRENT_STREAMS.  Note that this means that
SpdySession::CreateStream can now return ERR_IO_PENDING, so it
requires a callback.  There's a noted TODO that if an
http_network_transaction dissapears betweeen STATE_SPDY_GET_STREAM and
STATE_SPDY_SEND_REQUEST I don't know if we end up with an orphan stream
in our spdy_session.

As well, spdy_test_util.cc had a lot of functions with default arguments;
I didn't fix them all, but the functions I modified no longer take default
arguments and meet the coding standard.  I'd like to circle back at some point
and possibly make the tests call SpdyFramer directly: these test utils seem
sometimes more trouble than they're worth if the framer was a bit more
convenient for direct use.

BUG=34750
TEST=net_unittests Spdy.ThreeGets*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=52791

[Paweł Hajdan Jr., 2010-07-14 18:35:50]

Drive-by with a minor review comment.

http://codereview.chromium.org/2919011/diff/1/9
File net/spdy/spdy_network_transaction_unittest.cc (right):

http://codereview.chromium.org/2919011/diff/1/9#newcode294
net/spdy/spdy_network_transaction_unittest.cc:294: EXPECT_TRUE(0);
nit: Could you rather write FAIL()? Also applies to other places in this file.

[Mike Belshe, 2010-07-14 19:37:59]

thanks for doing this, Gavin!  It's quite close; despite my lengthy list of
comments!

http://codereview.chromium.org/2919011/diff/1/2
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/2919011/diff/1/2#newcode1328
net/http/http_network_transaction.cc:1328:
spdy_session->GetPushStream(request_->url, &spdy_stream_, net_log_);
Have this function return a SpdyStreamId instead of the actual stream.

(see my comment in spdy_session.h)

http://codereview.chromium.org/2919011/diff/1/2#newcode1337
net/http/http_network_transaction.cc:1337: &spdy_stream_,
Modify this one to return a SpdyStreamId too...

http://codereview.chromium.org/2919011/diff/1/2#newcode1355
net/http/http_network_transaction.cc:1355: 
Here, use the SpdySession::FindStream(spdy_stream_id_);

http://codereview.chromium.org/2919011/diff/1/3
File net/http/http_network_transaction.h (right):

http://codereview.chromium.org/2919011/diff/1/3#newcode272
net/http/http_network_transaction.h:272: UploadDataStream* upload_data_;
The upload_data_ is on the request_ already, so I don't think we need this
pointer?  You can just use request->upload_data_?

http://codereview.chromium.org/2919011/diff/1/3#newcode273
net/http/http_network_transaction.h:273: scoped_refptr<SpdyStream> spdy_stream_;
I don't think we're going to need this as a member either...  see my comments in
the .cc.

http://codereview.chromium.org/2919011/diff/1/8
File net/spdy/spdy_network_transaction.h (right):

http://codereview.chromium.org/2919011/diff/1/8#newcode96
net/spdy/spdy_network_transaction.h:96: UploadDataStream* upload_data_;
Same comments as HttpNetworkTransaction.  Sorry!  we're getting rid of this code
duplication soon!

http://codereview.chromium.org/2919011/diff/1/9
File net/spdy/spdy_network_transaction_unittest.cc (right):

http://codereview.chromium.org/2919011/diff/1/9#newcode225
net/spdy/spdy_network_transaction_unittest.cc:225: // TODO: create a working
generalized TransactionHelper that can
style: format for TODOs is "TODO(username):"

http://codereview.chromium.org/2919011/diff/1/9#newcode227
net/spdy/spdy_network_transaction_unittest.cc:227: 
nit: remove line 227

http://codereview.chromium.org/2919011/diff/1/9#newcode350
net/spdy/spdy_network_transaction_unittest.cc:350: 
nit: delete line 350

http://codereview.chromium.org/2919011/diff/1/9#newcode369
net/spdy/spdy_network_transaction_unittest.cc:369: spdy::SettingsFlagsAndId
flagandid(0);
style: flagandid.  I'd just call it "flags"

http://codereview.chromium.org/2919011/diff/1/9#newcode437
net/spdy/spdy_network_transaction_unittest.cc:437: }
It would be nice if we could see that the session has a pending create stream.

Can we expose a method to get the "session->pending_stream_count()" here, and
verify that it is 1?

http://codereview.chromium.org/2919011/diff/1/9#newcode449
net/spdy/spdy_network_transaction_unittest.cc:449: //   
session->spdy_session_pool()->ClearSessions();
remove line 449 and 455

http://codereview.chromium.org/2919011/diff/1/9#newcode479
net/spdy/spdy_network_transaction_unittest.cc:479: 
I think need to add 2 more tests:
  a) Since the PendingCreateStreams is a priority queue - can you add a test to
verify that we wake high priority streams first after we reached throttle?

  b) we need to test the HttpNetworkTransaction being cancelled (and hopefully
deleted) while a stream is pending creation.  I think this would catch a crash
right now.

http://codereview.chromium.org/2919011/diff/1/10
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/2919011/diff/1/10#newcode312
net/spdy/spdy_session.cc:312: //       transaction has since dissapeared?
Actually, we can't do it quite this way.  Our queue doesn't keep a reference to
the HttpNetworkTransaction.  So, the HTN can be deleted, and then when a
SpdySession frees up, we'll try to call the callback which will likely crash...

Maybe we should just add a CancelPendingStreamCreation().  It's not hard, and
just needs to be called if the HttpNetworkTransaction is deleted...

If you can make this so that it won't crash but want to leave the TODO, I'm okay
with that too.

http://codereview.chromium.org/2919011/diff/1/10#newcode316
net/spdy/spdy_session.cc:316: for (int i=0;i<NUM_PRIORITIES;++i) {
style: spacing

http://codereview.chromium.org/2919011/diff/1/10#newcode320
net/spdy/spdy_session.cc:320: int error = CreateStreamImpl( *pending_create.url,
style: extra space before *pending_create

http://codereview.chromium.org/2919011/diff/1/10#newcode330
net/spdy/spdy_session.cc:330: try_another_unqueue: ;
nit:  Can you rework to avoid the goto?  We don't have a lot of gotos...

http://codereview.chromium.org/2919011/diff/1/10#newcode754
net/spdy/spdy_session.cc:754: 
Here, before we call DeleteStream(), I'd clear out all the pending
CreateStreams, and wake them with an error code of ERR_ABORTED.

http://codereview.chromium.org/2919011/diff/1/10#newcode920
net/spdy/spdy_session.cc:920: UnqueuePendingCreates();
I'd move this function into DeleteStream(); otherwise we might miss it in the
future when calling DeleteStream.

http://codereview.chromium.org/2919011/diff/1/10#newcode1212
net/spdy/spdy_session.cc:1212:
UMA_HISTOGRAM_CUSTOM_COUNTS("Net.SpdyStreamsPerSesison",
I assume this is an accidental change?

http://codereview.chromium.org/2919011/diff/1/11
File net/spdy/spdy_session.h (right):

http://codereview.chromium.org/2919011/diff/1/11#newcode67
net/spdy/spdy_session.h:67: const BoundNetLog& stream_net_log);
I propose we change GetPushStream to be FindPushStream, and have it return a
SpdyStreamId instead of a scoped_refptr<SpdyStream>.

Also add a method for:

scoped_refptr<SpdyStream> FindStream(SpdyStreamId);

http://codereview.chromium.org/2919011/diff/1/11#newcode132
net/spdy/spdy_session.h:132: struct pending_create_stream {
style:  PendingCreateStream

http://codereview.chromium.org/2919011/diff/1/11#newcode156
net/spdy/spdy_session.h:156: void UnqueuePendingCreates();
nit: rename to "ProcessPendingCreateStreams()"?

http://codereview.chromium.org/2919011/diff/1/11#newcode254
net/spdy/spdy_session.h:254: // Queue, for each priority, of createstreams that
have not yet
nit: /createstreams/pending Create Streams/

[willchan no longer on Chromium, 2010-07-14 22:24:15]

There are lots of little style nits to mention.  It seems Mike has covered most
of them.

I have a different suggestion from Mike's.

Conceptually, I think HttpNetworkTransaction and SpdyNetworkTransaction should
not deal with SpdyStream, just SpdyHttpStream.  Likewise, the WebSocket code
will deal with SpdyWebSocketStreams, not SpdyStreams.

Based on that goal, I believe that STATE_GET_STREAM should create a
SpdyHttpStream.  We should then invoke an initialization method, let's call it
SpdyHttpStream::Init() or something, where we pass in the SpdySession and all
the parameters for SpdySession::CreateStream, since in this vision
SpdyHttpStream will call SpdySession::CreateStream(), instead of having
HttpNetworkTransaction do it.

I'd prefer not to expose a stream lookup function on SpdySession.  I like just
having a stream creation.  I think it limits the surface area for bugs.

[gavinp, 2010-07-15 00:50:16]

Thanks everyone for the comments; Will, yours came in as I was finishing, so it
is not reflected here.  I'll hopefully tomorrow give it serious consideration.

http://codereview.chromium.org/2919011/diff/1/2
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/2919011/diff/1/2#newcode1328
net/http/http_network_transaction.cc:1328:
spdy_session->GetPushStream(request_->url, &spdy_stream_, net_log_);
On 2010/07/14 19:37:59, Mike Belshe wrote:
> Have this function return a SpdyStreamId instead of the actual stream.
> 
> (see my comment in spdy_session.h)

No can do.  GetPushStream sometimes returns a SpdyStream with no stream_id, so
we're stuck with this gunk.

http://codereview.chromium.org/2919011/diff/1/3
File net/http/http_network_transaction.h (right):

http://codereview.chromium.org/2919011/diff/1/3#newcode272
net/http/http_network_transaction.h:272: UploadDataStream* upload_data_;
On 2010/07/14 19:37:59, Mike Belshe wrote:
> The upload_data_ is on the request_ already, so I don't think we need this
> pointer?  You can just use request->upload_data_?

Done.  It was an upload_data_stream (badly named!), and it wasn't needed except
as an automatic, I moved it back into xxxSendRequest().

http://codereview.chromium.org/2919011/diff/1/9
File net/spdy/spdy_network_transaction_unittest.cc (right):

http://codereview.chromium.org/2919011/diff/1/9#newcode294
net/spdy/spdy_network_transaction_unittest.cc:294: EXPECT_TRUE(0);
On 2010/07/14 18:35:50, Paweł Hajdan Jr. wrote:
> nit: Could you rather write FAIL()? Also applies to other places in this file.

Done.

http://codereview.chromium.org/2919011/diff/1/9#newcode350
net/spdy/spdy_network_transaction_unittest.cc:350: 
On 2010/07/14 19:37:59, Mike Belshe wrote:
> nit: delete line 350

Done.

http://codereview.chromium.org/2919011/diff/1/9#newcode369
net/spdy/spdy_network_transaction_unittest.cc:369: spdy::SettingsFlagsAndId
flagandid(0);
On 2010/07/14 19:37:59, Mike Belshe wrote:
> style: flagandid.  I'd just call it "flags"

Done.

http://codereview.chromium.org/2919011/diff/1/9#newcode437
net/spdy/spdy_network_transaction_unittest.cc:437: }
On 2010/07/14 19:37:59, Mike Belshe wrote:
> It would be nice if we could see that the session has a pending create stream.
> 
> Can we expose a method to get the "session->pending_stream_count()" here, and
> verify that it is 1?

Tricky.  That has to go through HttpNetworkSession --> SpsySessionPool -->
SpdySession -> etc...

The check on like 446 below with 7U does actually check this: had the limit not
existed, the read would be way past here.

http://codereview.chromium.org/2919011/diff/1/9#newcode449
net/spdy/spdy_network_transaction_unittest.cc:449: //   
session->spdy_session_pool()->ClearSessions();
On 2010/07/14 19:37:59, Mike Belshe wrote:
> remove line 449 and 455

Done.

http://codereview.chromium.org/2919011/diff/1/10
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/2919011/diff/1/10#newcode316
net/spdy/spdy_session.cc:316: for (int i=0;i<NUM_PRIORITIES;++i) {
On 2010/07/14 19:37:59, Mike Belshe wrote:
> style: spacing

Done.

http://codereview.chromium.org/2919011/diff/1/10#newcode320
net/spdy/spdy_session.cc:320: int error = CreateStreamImpl( *pending_create.url,
On 2010/07/14 19:37:59, Mike Belshe wrote:
> style: extra space before *pending_create
> 

Done.

http://codereview.chromium.org/2919011/diff/1/10#newcode330
net/spdy/spdy_session.cc:330: try_another_unqueue: ;
On 2010/07/14 19:37:59, Mike Belshe wrote:
> nit:  Can you rework to avoid the goto?  We don't have a lot of gotos...

Done.

http://codereview.chromium.org/2919011/diff/1/10#newcode754
net/spdy/spdy_session.cc:754: 
On 2010/07/14 19:37:59, Mike Belshe wrote:
> Here, before we call DeleteStream(), I'd clear out all the pending
> CreateStreams, and wake them with an error code of ERR_ABORTED.

Done.

http://codereview.chromium.org/2919011/diff/1/10#newcode920
net/spdy/spdy_session.cc:920: UnqueuePendingCreates();
On 2010/07/14 19:37:59, Mike Belshe wrote:
> I'd move this function into DeleteStream(); otherwise we might miss it in the
> future when calling DeleteStream.

Done.

http://codereview.chromium.org/2919011/diff/1/10#newcode1212
net/spdy/spdy_session.cc:1212:
UMA_HISTOGRAM_CUSTOM_COUNTS("Net.SpdyStreamsPerSesison",
On 2010/07/14 19:37:59, Mike Belshe wrote:
> I assume this is an accidental change?

Oui.

http://codereview.chromium.org/2919011/diff/1/11
File net/spdy/spdy_session.h (right):

http://codereview.chromium.org/2919011/diff/1/11#newcode132
net/spdy/spdy_session.h:132: struct pending_create_stream {
On 2010/07/14 19:37:59, Mike Belshe wrote:
> style:  PendingCreateStream

Done.

http://codereview.chromium.org/2919011/diff/1/11#newcode156
net/spdy/spdy_session.h:156: void UnqueuePendingCreates();
On 2010/07/14 19:37:59, Mike Belshe wrote:
> nit: rename to "ProcessPendingCreateStreams()"?

Done.

http://codereview.chromium.org/2919011/diff/1/11#newcode254
net/spdy/spdy_session.h:254: // Queue, for each priority, of createstreams that
have not yet
On 2010/07/14 19:37:59, Mike Belshe wrote:
> nit: /createstreams/pending Create Streams/

Done.

[Paweł Hajdan Jr., 2010-07-15 00:53:37]

http://codereview.chromium.org/2919011/diff/16001/17008
File net/spdy/spdy_network_transaction_unittest.cc (right):

http://codereview.chromium.org/2919011/diff/16001/17008#newcode294
net/spdy/spdy_network_transaction_unittest.cc:294: EXPECT_TRUE(0);
I still see EXPECT_TRUE(0) here. Could you please replace it with FAIL()? Also
check other occurrences in this file.

[Mike Belshe, 2010-07-15 05:28:21]

Thanks for adding those tests.  lgtm.

http://codereview.chromium.org/2919011/diff/16001/17001
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/2919011/diff/16001/17001#newcode462
net/http/http_network_transaction.cc:462: // let's make sure a
PendingCreateStream in a SpdySession
style:  you should make the comment be a grammatically correct sentence :-)

http://codereview.chromium.org/2919011/diff/16001/17001#newcode1314
net/http/http_network_transaction.cc:1314: // balancing call to
SpdySession::CreateStream in DoSpdyGetSession
nit: /DoSpdyGetSession/DoSpdyGetStream/

http://codereview.chromium.org/2919011/diff/16001/17002
File net/http/http_network_transaction.h (right):

http://codereview.chromium.org/2919011/diff/16001/17002#newcode39
net/http/http_network_transaction.h:39: class UploadDataStream;
nit:  I think you can nix line 39 now.

http://codereview.chromium.org/2919011/diff/16001/17008
File net/spdy/spdy_network_transaction_unittest.cc (right):

http://codereview.chromium.org/2919011/diff/16001/17008#newcode1114
net/spdy/spdy_network_transaction_unittest.cc:1114: 
delete line 114

http://codereview.chromium.org/2919011/diff/16001/17009
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/2919011/diff/16001/17009#newcode159
net/spdy/spdy_session.cc:159: max_concurrent_streams_(0),
Were you going to set this to 100?  (use a kConstant, add a TODO(mbelshe) to
remove it later)

http://codereview.chromium.org/2919011/diff/16001/17009#newcode333
net/spdy/spdy_session.cc:333: // TODO(gavinp): clean this ugliness up?
I don't like this comment, because I don't know what it means!

delete line 334

http://codereview.chromium.org/2919011/diff/16001/17009#newcode337
net/spdy/spdy_session.cc:337: for (int i = 0;i < NUM_PRIORITIES;++i) {
Is it possible for a single transaction to ever have more than one pending
stream?

If not, then this can get much simpler.  Avoid the copies and loops - and
instead just find the one and remove it.

[gavinp, 2010-07-15 16:36:30]

thanks again, I hope this second remediation addresses most issues.

http://codereview.chromium.org/2919011/diff/16001/17001
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/2919011/diff/16001/17001#newcode462
net/http/http_network_transaction.cc:462: // let's make sure a
PendingCreateStream in a SpdySession
On 2010/07/15 05:28:21, Mike Belshe wrote:
> style:  you should make the comment be a grammatically correct sentence :-)

Done.

http://codereview.chromium.org/2919011/diff/16001/17001#newcode1314
net/http/http_network_transaction.cc:1314: // balancing call to
SpdySession::CreateStream in DoSpdyGetSession
On 2010/07/15 05:28:21, Mike Belshe wrote:
> nit: /DoSpdyGetSession/DoSpdyGetStream/

Done.

http://codereview.chromium.org/2919011/diff/16001/17002
File net/http/http_network_transaction.h (right):

http://codereview.chromium.org/2919011/diff/16001/17002#newcode39
net/http/http_network_transaction.h:39: class UploadDataStream;
On 2010/07/15 05:28:21, Mike Belshe wrote:
> nit:  I think you can nix line 39 now.

Done.

http://codereview.chromium.org/2919011/diff/16001/17008
File net/spdy/spdy_network_transaction_unittest.cc (right):

http://codereview.chromium.org/2919011/diff/16001/17008#newcode294
net/spdy/spdy_network_transaction_unittest.cc:294: EXPECT_TRUE(0);
On 2010/07/15 00:53:37, Paweł Hajdan Jr. wrote:
> I still see EXPECT_TRUE(0) here. Could you please replace it with FAIL()? Also
> check other occurrences in this file.

Done.

http://codereview.chromium.org/2919011/diff/16001/17008#newcode1114
net/spdy/spdy_network_transaction_unittest.cc:1114: 
On 2010/07/15 05:28:21, Mike Belshe wrote:
> delete line 114

Done.

http://codereview.chromium.org/2919011/diff/16001/17009
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/2919011/diff/16001/17009#newcode159
net/spdy/spdy_session.cc:159: max_concurrent_streams_(0),
On 2010/07/15 05:28:21, Mike Belshe wrote:
> Were you going to set this to 100?  (use a kConstant, add a TODO(mbelshe) to
> remove it later)

Done.

http://codereview.chromium.org/2919011/diff/16001/17009#newcode333
net/spdy/spdy_session.cc:333: // TODO(gavinp): clean this ugliness up?
On 2010/07/15 05:28:21, Mike Belshe wrote:
> I don't like this comment, because I don't know what it means!
> 
> delete line 334

Done.

http://codereview.chromium.org/2919011/diff/16001/17009#newcode337
net/spdy/spdy_session.cc:337: for (int i = 0;i < NUM_PRIORITIES;++i) {
On 2010/07/15 05:28:21, Mike Belshe wrote:
> Is it possible for a single transaction to ever have more than one pending
> stream?

No.

> If not, then this can get much simpler.  Avoid the copies and loops - and
> instead just find the one and remove it.

Sort of, but std::queue<> doesn't support random access of any kind, so you'll
have an explicit loop as long as you stick to that data type.

Maybe having to implement this function means std::queue<> was the wrong choice,
but any removal is going to be O(n) unless we create a hash_map<> or map<>, and
so my thinking was this wasn't so bad for something done unusually.  I've
looked, and there is a size() method on queue, so there's no need to make the
copy, but I'm leaving it as is.

[Paweł Hajdan Jr., 2010-07-15 17:23:02]

http://codereview.chromium.org/2919011/diff/26001/24009
File net/spdy/spdy_network_transaction_unittest.cc (right):

http://codereview.chromium.org/2919011/diff/26001/24009#newcode293
net/spdy/spdy_network_transaction_unittest.cc:293: if (out.rv != ERR_IO_PENDING)
{
Oh wait, why not just ASSERT_NE then? Feel free to do in a follow-up CL.

[willchan no longer on Chromium, 2010-07-15 17:43:50]

I've highlighted a number of spots that I feel would be cleaned up if you moved
the SpdyStream creation into the SpdyHttpStream.  I believe it's better for
abstraction to do it.

I am leaving very soon for Africa.  I will not probably have time to make
additional comments.  Mike will need to finish the review.

http://codereview.chromium.org/2919011/diff/26001/24002
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/2919011/diff/26001/24002#newcode462
net/http/http_network_transaction.cc:462: // If we're in GET_STREAM state, we
may have a PendingCreateStream
This block should move into the SpdyHttpStream's destructor instead.

http://codereview.chromium.org/2919011/diff/26001/24002#newcode1314
net/http/http_network_transaction.cc:1314: // balancing call to
SpdySession::CreateStream in DoSpdyGetStream
You shouldn't need this balancing thing if you kept spdy_stream_ only in
SpdyHttpStream.

http://codereview.chromium.org/2919011/diff/26001/24003
File net/http/http_network_transaction.h (right):

http://codereview.chromium.org/2919011/diff/26001/24003#newcode270
net/http/http_network_transaction.h:270: scoped_refptr<SpdyStream> spdy_stream_;
I'd like to see this go away.

http://codereview.chromium.org/2919011/diff/26001/24007
File net/spdy/spdy_network_transaction.cc (right):

http://codereview.chromium.org/2919011/diff/26001/24007#newcode294
net/spdy/spdy_network_transaction.cc:294: std::cerr << "result = " << result <<
"\n";
This looks like it's for debugging.  Are you going to remove it?

http://codereview.chromium.org/2919011/diff/26001/24009
File net/spdy/spdy_network_transaction_unittest.cc (right):

http://codereview.chromium.org/2919011/diff/26001/24009#newcode507
net/spdy/spdy_network_transaction_unittest.cc:507: scoped_ptr<spdy::SpdyFrame>
req4(ConstructSpdyGet(NULL, 0, false, 5, HIGHEST));
80 columns

http://codereview.chromium.org/2919011/diff/26001/24010
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/2919011/diff/26001/24010#newcode334
net/spdy/spdy_session.cc:334: {
This needs to go on the previous line

http://codereview.chromium.org/2919011/diff/26001/24010#newcode336
net/spdy/spdy_session.cc:336: PendingCreateStreamQueue copy;
How about use tmp instead of copy.  copy gets confused with std::copy.

http://codereview.chromium.org/2919011/diff/26001/24011
File net/spdy/spdy_session.h (right):

http://codereview.chromium.org/2919011/diff/26001/24011#newcode314
net/spdy/spdy_session.h:314: enum { kDefaultMaxConcurrentStreams = 100 };  //
TODO(mbelshe) remove this
enums need to go at the beginning of the block, see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

http://codereview.chromium.org/2919011/diff/26001/24013
File net/spdy/spdy_test_util.h (right):

http://codereview.chromium.org/2919011/diff/26001/24013#newcode143
net/spdy/spdy_test_util.h:143: RequestPriority request_priority=LOWEST);
default arguments aren't allowed:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Defaul...

[gavinp, 2010-07-16 03:10:56]

Moving things into SpdyHttpStream was, as Will said, a much better option.  The
code is all a lot simpler now.

I also found a bug in RecordHistograms() in spdy_session now fixed; it's totally
legit to receive settings, and have an empty recorded settings.  My tests for
instance had a non-persisting max_concurent_streams.  So now we don't
notreached() on that.

http://codereview.chromium.org/2919011/diff/26001/24002
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/2919011/diff/26001/24002#newcode462
net/http/http_network_transaction.cc:462: // If we're in GET_STREAM state, we
may have a PendingCreateStream
On 2010/07/15 17:43:51, willchan wrote:
> This block should move into the SpdyHttpStream's destructor instead.

Done.

http://codereview.chromium.org/2919011/diff/26001/24002#newcode1314
net/http/http_network_transaction.cc:1314: // balancing call to
SpdySession::CreateStream in DoSpdyGetStream
On 2010/07/15 17:43:51, willchan wrote:
> You shouldn't need this balancing thing if you kept spdy_stream_ only in
> SpdyHttpStream.

Done.

http://codereview.chromium.org/2919011/diff/26001/24003
File net/http/http_network_transaction.h (right):

http://codereview.chromium.org/2919011/diff/26001/24003#newcode270
net/http/http_network_transaction.h:270: scoped_refptr<SpdyStream> spdy_stream_;
On 2010/07/15 17:43:51, willchan wrote:
> I'd like to see this go away.

Done.

http://codereview.chromium.org/2919011/diff/26001/24007
File net/spdy/spdy_network_transaction.cc (right):

http://codereview.chromium.org/2919011/diff/26001/24007#newcode294
net/spdy/spdy_network_transaction.cc:294: std::cerr << "result = " << result <<
"\n";
On 2010/07/15 17:43:51, willchan wrote:
> This looks like it's for debugging.  Are you going to remove it?

Done.

http://codereview.chromium.org/2919011/diff/26001/24009
File net/spdy/spdy_network_transaction_unittest.cc (right):

http://codereview.chromium.org/2919011/diff/26001/24009#newcode293
net/spdy/spdy_network_transaction_unittest.cc:293: if (out.rv != ERR_IO_PENDING)
{
On 2010/07/15 17:23:03, Paweł Hajdan Jr. wrote:
> Oh wait, why not just ASSERT_NE then? Feel free to do in a follow-up CL.

Done.

http://codereview.chromium.org/2919011/diff/26001/24009#newcode507
net/spdy/spdy_network_transaction_unittest.cc:507: scoped_ptr<spdy::SpdyFrame>
req4(ConstructSpdyGet(NULL, 0, false, 5, HIGHEST));
On 2010/07/15 17:43:51, willchan wrote:
> 80 columns

Done.

http://codereview.chromium.org/2919011/diff/26001/24010
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/2919011/diff/26001/24010#newcode334
net/spdy/spdy_session.cc:334: {
On 2010/07/15 17:43:51, willchan wrote:
> This needs to go on the previous line

Done.

http://codereview.chromium.org/2919011/diff/26001/24010#newcode336
net/spdy/spdy_session.cc:336: PendingCreateStreamQueue copy;
On 2010/07/15 17:43:51, willchan wrote:
> How about use tmp instead of copy.  copy gets confused with std::copy.

Done.

http://codereview.chromium.org/2919011/diff/26001/24011
File net/spdy/spdy_session.h (right):

http://codereview.chromium.org/2919011/diff/26001/24011#newcode314
net/spdy/spdy_session.h:314: enum { kDefaultMaxConcurrentStreams = 100 };  //
TODO(mbelshe) remove this
On 2010/07/15 17:43:51, willchan wrote:
> enums need to go at the beginning of the block, see
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Declaration_Order

Done.

http://codereview.chromium.org/2919011/diff/26001/24013
File net/spdy/spdy_test_util.h (right):

http://codereview.chromium.org/2919011/diff/26001/24013#newcode143
net/spdy/spdy_test_util.h:143: RequestPriority request_priority=LOWEST);
On 2010/07/15 17:43:51, willchan wrote:
> default arguments aren't allowed:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Defaul...

Done.

[Mike Belshe, 2010-07-16 17:27:53]

lgtm

http://codereview.chromium.org/2919011/diff/36015/21006
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/2919011/diff/36015/21006#newcode155
net/spdy/spdy_http_stream.cc:155: int error =
nit:  I found the spacing on line 155/156 to be a little funny

http://codereview.chromium.org/2919011/diff/36015/21007
File net/spdy/spdy_http_stream.h (right):

http://codereview.chromium.org/2919011/diff/36015/21007#newcode113
net/spdy/spdy_http_stream.h:113: SpdySession* spdy_session_;
Can this be a scoped_refptr<>?

http://codereview.chromium.org/2919011/diff/36015/21012
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/2919011/diff/36015/21012#newcode337
net/spdy/spdy_session.cc:337: // make a copy removing this trans
comment: "Make a copy removing this stream."

http://codereview.chromium.org/2919011/diff/36015/21012#newcode344
net/spdy/spdy_session.cc:344: // now copy it back
comment: "Now copy it back."

[gavinp, 2010-07-16 20:40:16]

I'm going to land this now.  I spent some time testing against a GFE provided
generously by the alyssar, and it worked well.  I also found one more place to
add a ProcessPendingCreateStreams() : we could receive a settings that increases
the number of streams we are permitted to open, and if that occurs, we should
dequeue them all.  Right now.

Thanks for all the reviews.

http://codereview.chromium.org/2919011/diff/36015/21006
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/2919011/diff/36015/21006#newcode155
net/spdy/spdy_http_stream.cc:155: int error =
On 2010/07/16 17:27:54, Mike Belshe wrote:
> nit:  I found the spacing on line 155/156 to be a little funny
> 

Done.

http://codereview.chromium.org/2919011/diff/36015/21007
File net/spdy/spdy_http_stream.h (right):

http://codereview.chromium.org/2919011/diff/36015/21007#newcode113
net/spdy/spdy_http_stream.h:113: SpdySession* spdy_session_;
On 2010/07/16 17:27:54, Mike Belshe wrote:
> Can this be a scoped_refptr<>?

Done.

http://codereview.chromium.org/2919011/diff/36015/21012
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/2919011/diff/36015/21012#newcode337
net/spdy/spdy_session.cc:337: // make a copy removing this trans
On 2010/07/16 17:27:54, Mike Belshe wrote:
> comment: "Make a copy removing this stream."

Done.

http://codereview.chromium.org/2919011/diff/36015/21012#newcode344
net/spdy/spdy_session.cc:344: // now copy it back
On 2010/07/16 17:27:54, Mike Belshe wrote:
> comment: "Now copy it back."

Done.

[gavinp, 2010-07-16 21:03:27]

I'm going to land this now.  I spent some time testing against a GFE provided
generously by the alyssar, and it worked well.  I also found one more place to
add a ProcessPendingCreateStreams() : we could receive a settings that increases
the number of streams we are permitted to open, and if that occurs, we should
dequeue them all.  Right now.

Thanks for all the reviews.