Fix out of order SYN_STEAM frames.

Instead of enqueueing SPDY frames, instead enqueue SPDY streams that are ready to produce data.  This allows us to lazily allocate a stream id.

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

[Ryan Hamilton, 2012-06-06 16:42:33]

willchan: fyi
raman: can you please review this CL?  It was a bit more invasive than I
initially expected, but I think it's pretty reasonable.

[willchan no longer on Chromium, 2012-06-07 23:02:06]

CL description needs more detail about what's changing, and SpdySession is
complicated enough now that we need a high-level comment somewhere about the
various stages of "writing" and "activation" and output queue and what not.

I need to read in more detail, but the SpdyFrameProducer thing was surprising.
It may make sense, I just need to read through this some more.

http://codereview.chromium.org/10448083/diff/8001/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

http://codereview.chromium.org/10448083/diff/8001/net/spdy/spdy_session.h#new...
net/spdy/spdy_session.h:577: // Set of all created streams that have not yet
been activated.
This comment makes no sense without context of what "activation" means. Please
write more so someone unfamiliar with the code can understand.

http://codereview.chromium.org/10448083/diff/8001/net/spdy/spdy_session.h#new...
net/spdy/spdy_session.h:581: OutputQueue queue_;
Can this die?

[Ryan Hamilton, 2012-06-08 00:02:26]

I'll also take a look at updating the class comment, though I've not done that
yet.

http://codereview.chromium.org/10448083/diff/8001/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

http://codereview.chromium.org/10448083/diff/8001/net/spdy/spdy_session.h#new...
net/spdy/spdy_session.h:577: // Set of all created streams that have not yet
been activated.
On 2012/06/07 23:02:06, willchan wrote:
> This comment makes no sense without context of what "activation" means. Please
> write more so someone unfamiliar with the code can understand.

Done.

http://codereview.chromium.org/10448083/diff/8001/net/spdy/spdy_session.h#new...
net/spdy/spdy_session.h:581: OutputQueue queue_;
On 2012/06/07 23:02:06, willchan wrote:
> Can this die?

Indeed!  Done.

[Ryan Hamilton, 2012-06-15 18:53:09]

On 2012/06/06 16:42:33, Ryan Hamilton wrote:
> willchan: fyi
> raman: can you please review this CL?  It was a bit more invasive than I
> initially expected, but I think it's pretty reasonable.

Raman, can you please review...

[ramant (doing other things), 2012-06-20 01:50:40]

Hi Ryan,
  Sorry for the delay. Some minor comments.

thanks,
raman

http://codereview.chromium.org/10448083/diff/6002/net/spdy/spdy_frame_producer.h
File net/spdy/spdy_frame_producer.h (right):

http://codereview.chromium.org/10448083/diff/6002/net/spdy/spdy_frame_produce...
net/spdy/spdy_frame_producer.h:17: public:
nit: A small comment describing purpose of this class please.

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

http://codereview.chromium.org/10448083/diff/6002/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:654: const scoped_refptr<SpdyStream>& stream =
active_streams_[stream_id];
nit: Can IsStreamActive be false in non-debug mode? If not, could we change it
to a CHECK?

http://codereview.chromium.org/10448083/diff/6002/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:1033: continue;
nit: indent "if (frame == NULL)"?

nit: Should we consider recording in a histogram how often frame becomes NULL?

http://codereview.chromium.org/10448083/diff/6002/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:1102: if (stream)
nit: Move "stream->LogStreamError" to under "if (stream)"?

http://codereview.chromium.org/10448083/diff/6002/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:1268: WriteQueue old = write_queue_;
overly nit: consider ending the sentence with a period.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Punctuation,_S...

http://codereview.chromium.org/10448083/diff/6002/net/spdy/spdy_stream.cc
File net/spdy/spdy_stream.cc (right):

http://codereview.chromium.org/10448083/diff/6002/net/spdy/spdy_stream.cc#new...
net/spdy/spdy_stream.cc:112: domain_bound_cert_, priority_);
nit: consider adding a CHECK_GT(stream_id_, 0u)?

http://codereview.chromium.org/10448083/diff/6002/net/spdy/spdy_stream.cc#new...
net/spdy/spdy_stream.cc:121: set_stream_id(frame->stream_id());
nit: can we do CHECK_EQ(stream_id_, 0u)? Or do we want to overwrite the
stream_id_ with frame's stream_id? or should we assert stream_id_ is same as
frame->stream_id()? thanks.

http://codereview.chromium.org/10448083/diff/6002/net/spdy/spdy_stream.cc#new...
net/spdy/spdy_stream.cc:573: DCHECK_GT(stream_id_, 0u);
nit: can we change it to CHECK_GT to assert that set_stream_id was called before
WriteStreamData in non-debug builds also?

http://codereview.chromium.org/10448083/diff/6002/net/spdy/spdy_stream.cc#new...
net/spdy/spdy_stream.cc:726: origin, domain_bound_cert_type_,
domain_bound_private_key_,
nit: SpdyStream::ProduceNextFrame also seems to be doing CreateCredentialFrame.
Looks like we don't need to create CredentialFrame here.

[Ryan Hamilton, 2012-06-21 18:14:58]

OK, I've fixed raman's comments.  Will stopped by yesterday with a few more. 
I've added these comments to the review but have not addressed them.  I'll do
that next.  Raman, hold off on re-reviewing until after I address them, please. 
(Well, actually you can take another look if you want, but it'd probably save
you time to wait :>)

http://codereview.chromium.org/10448083/diff/6002/net/spdy/spdy_frame_producer.h
File net/spdy/spdy_frame_producer.h (right):

http://codereview.chromium.org/10448083/diff/6002/net/spdy/spdy_frame_produce...
net/spdy/spdy_frame_producer.h:16: class NET_EXPORT_PRIVATE SpdyFrameProducer {
move to nested class of SpdySession

http://codereview.chromium.org/10448083/diff/6002/net/spdy/spdy_frame_produce...
net/spdy/spdy_frame_producer.h:17: public:
On 2012/06/20 01:50:40, ramant wrote:
> nit: A small comment describing purpose of this class please.

Done.

http://codereview.chromium.org/10448083/diff/6002/net/spdy/spdy_frame_produce...
net/spdy/spdy_frame_producer.h:24: virtual SpdyStream* GetSpdyStream() const =
0;
remove const

http://codereview.chromium.org/10448083/diff/6002/net/spdy/spdy_frame_produce...
net/spdy/spdy_frame_producer.h:27: virtual ~SpdyFrameProducer() {}
Make destructor public.  Change SpdyStream to not inherit.  Instead add an
adapter class to wrap the spdy stream (which will be a friend so it can all the
protected ProduceNextFrame).

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

http://codereview.chromium.org/10448083/diff/6002/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:654: const scoped_refptr<SpdyStream>& stream =
active_streams_[stream_id];
On 2012/06/20 01:50:40, ramant wrote:
> nit: Can IsStreamActive be false in non-debug mode? If not, could we change it
> to a CHECK?

Done.

http://codereview.chromium.org/10448083/diff/6002/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:1024: }
Move thislogic to a static method of SpdyFrameProducer (which will be a friend
of spdy session)

http://codereview.chromium.org/10448083/diff/6002/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:1033: continue;
On 2012/06/20 01:50:40, ramant wrote:
> nit: indent "if (frame == NULL)"?

Done.  (Slightly reformatted)

> nit: Should we consider recording in a histogram how often frame becomes NULL?

We definitely could, how 'bout we do this in a subsequent CL?

http://codereview.chromium.org/10448083/diff/6002/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:1102: if (stream)
On 2012/06/20 01:50:40, ramant wrote:
> nit: Move "stream->LogStreamError" to under "if (stream)"?

Nice catch.  Actually, as I look at it now I think if (stream) is redundant.  Do
you agree?  We never insert NULL into created_streams_ so there should never be
a case where we pull NULL out.  I think I'll remove the if.  Work for you?

http://codereview.chromium.org/10448083/diff/6002/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:1268: WriteQueue old = write_queue_;
On 2012/06/20 01:50:40, ramant wrote:
> overly nit: consider ending the sentence with a period.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Punctuation%2C...

Done.

http://codereview.chromium.org/10448083/diff/6002/net/spdy/spdy_stream.cc
File net/spdy/spdy_stream.cc (right):

http://codereview.chromium.org/10448083/diff/6002/net/spdy/spdy_stream.cc#new...
net/spdy/spdy_stream.cc:112: domain_bound_cert_, priority_);
On 2012/06/20 01:50:40, ramant wrote:
> nit: consider adding a CHECK_GT(stream_id_, 0u)?

Done.

http://codereview.chromium.org/10448083/diff/6002/net/spdy/spdy_stream.cc#new...
net/spdy/spdy_stream.cc:121: set_stream_id(frame->stream_id());
On 2012/06/20 01:50:40, ramant wrote:
> nit: can we do CHECK_EQ(stream_id_, 0u)? Or do we want to overwrite the
> stream_id_ with frame's stream_id? or should we assert stream_id_ is same as
> frame->stream_id()? thanks.

Sorry, this line of code was a bug.  We should not be doing the set_stream_id().
 I can add a check there, but that seems a bit silly since we dont' do that for
priority, flags, slot, or the request headers.

http://codereview.chromium.org/10448083/diff/6002/net/spdy/spdy_stream.cc#new...
net/spdy/spdy_stream.cc:573: DCHECK_GT(stream_id_, 0u);
On 2012/06/20 01:50:40, ramant wrote:
> nit: can we change it to CHECK_GT to assert that set_stream_id was called
before
> WriteStreamData in non-debug builds also?

Done, but I feel like I don't have a good sense for when we want to use DCHECK
and when we want to use CHECK.  I'd love it if the style guide had some advice
here.  Perhaps I'll send a message to chromium dev.

http://codereview.chromium.org/10448083/diff/6002/net/spdy/spdy_stream.cc#new...
net/spdy/spdy_stream.cc:726: origin, domain_bound_cert_type_,
domain_bound_private_key_,
On 2012/06/20 01:50:40, ramant wrote:
> nit: SpdyStream::ProduceNextFrame also seems to be doing
CreateCredentialFrame.
> Looks like we don't need to create CredentialFrame here.

Done.

http://codereview.chromium.org/10448083/diff/6002/net/spdy/spdy_stream.h
File net/spdy/spdy_stream.h (right):

http://codereview.chromium.org/10448083/diff/6002/net/spdy/spdy_stream.h#newc...
net/spdy/spdy_stream.h:45: class NET_EXPORT_PRIVATE SpdyStream
Add a factory method to return a new SpdyFrameProducer adapter which will be
owned by the caller.

http://codereview.chromium.org/10448083/diff/6002/net/spdy/spdy_stream.h#newc...
net/spdy/spdy_stream.h:45: class NET_EXPORT_PRIVATE SpdyStream
Add private nested adpater class here.  fwd declare in private: section then
define in .cc

add factory method to return newly created adapter whichwill be owned by the
caller

[Ryan Hamilton, 2012-06-21 23:47:22]

Raman, I think I've got this working correctly.  Can you take another look?

[willchan no longer on Chromium, 2012-06-21 23:51:03]

https://chromiumcodereview.appspot.com/10448083/diff/22001/net/spdy/spdy_sess...
File net/spdy/spdy_session.h (right):

https://chromiumcodereview.appspot.com/10448083/diff/22001/net/spdy/spdy_sess...
net/spdy/spdy_session.h:100: class NET_EXPORT_PRIVATE SpdyFrameProducer {
Indentation

https://chromiumcodereview.appspot.com/10448083/diff/22001/net/spdy/spdy_sess...
net/spdy/spdy_session.h:111: virtual SpdyStream* GetSpdyStream() = 0;
Do you still need this one?

https://chromiumcodereview.appspot.com/10448083/diff/22001/net/spdy/spdy_sess...
net/spdy/spdy_session.h:116: void ActivateStream(SpdySession* spdy_session,
SpdyStream* spdy_stream);
Can this be static?

[Ryan Hamilton, 2012-06-22 16:17:32]

https://chromiumcodereview.appspot.com/10448083/diff/22001/net/spdy/spdy_sess...
File net/spdy/spdy_session.h (right):

https://chromiumcodereview.appspot.com/10448083/diff/22001/net/spdy/spdy_sess...
net/spdy/spdy_session.h:100: class NET_EXPORT_PRIVATE SpdyFrameProducer {
On 2012/06/21 23:51:03, willchan wrote:
> Indentation

Done.

https://chromiumcodereview.appspot.com/10448083/diff/22001/net/spdy/spdy_sess...
net/spdy/spdy_session.h:111: virtual SpdyStream* GetSpdyStream() = 0;
On 2012/06/21 23:51:03, willchan wrote:
> Do you still need this one?

It is still used because when we decide that we need to close a stream, we
remove the corresponding SpdyFrameProducer from the the write_queue.  To do this
we look at the SpdyStream associated with each Producer.  If you would prefer,
however, I could remove this method and add a IsAssociateWith(SpdyStream*)
method, or some such?

https://chromiumcodereview.appspot.com/10448083/diff/22001/net/spdy/spdy_sess...
net/spdy/spdy_session.h:116: void ActivateStream(SpdySession* spdy_session,
SpdyStream* spdy_stream);
On 2012/06/21 23:51:03, willchan wrote:
> Can this be static?

Done.

[willchan no longer on Chromium, 2012-06-22 21:10:44]

https://chromiumcodereview.appspot.com/10448083/diff/22001/net/spdy/spdy_sess...
File net/spdy/spdy_session.h (right):

https://chromiumcodereview.appspot.com/10448083/diff/22001/net/spdy/spdy_sess...
net/spdy/spdy_session.h:111: virtual SpdyStream* GetSpdyStream() = 0;
On 2012/06/22 16:17:32, Ryan Hamilton wrote:
> On 2012/06/21 23:51:03, willchan wrote:
> > Do you still need this one?
> 
> It is still used because when we decide that we need to close a stream, we
> remove the corresponding SpdyFrameProducer from the the write_queue.  To do
this
> we look at the SpdyStream associated with each Producer.  If you would prefer,
> however, I could remove this method and add a IsAssociateWith(SpdyStream*)
> method, or some such?

I see. I think I'd rather keep it out of this interface and have the SpdySession
maintain the mapping of streams to SpdyFrameProducers, so it can delete those
SpdyFrameProducers when the stream is canceled.

[Ryan Hamilton, 2012-06-22 22:54:05]

On 2012/06/22 21:10:44, willchan wrote:
>
https://chromiumcodereview.appspot.com/10448083/diff/22001/net/spdy/spdy_sess...
> File net/spdy/spdy_session.h (right):
> 
>
https://chromiumcodereview.appspot.com/10448083/diff/22001/net/spdy/spdy_sess...
> net/spdy/spdy_session.h:111: virtual SpdyStream* GetSpdyStream() = 0;
> On 2012/06/22 16:17:32, Ryan Hamilton wrote:
> > On 2012/06/21 23:51:03, willchan wrote:
> > > Do you still need this one?
> > 
> > It is still used because when we decide that we need to close a stream, we
> > remove the corresponding SpdyFrameProducer from the the write_queue.  To do
> this
> > we look at the SpdyStream associated with each Producer.  If you would
prefer,
> > however, I could remove this method and add a IsAssociateWith(SpdyStream*)
> > method, or some such?
> 
> I see. I think I'd rather keep it out of this interface and have the
SpdySession
> maintain the mapping of streams to SpdyFrameProducers, so it can delete those
> SpdyFrameProducers when the stream is canceled.

Whoops.  I did what you proposed but hit a glitch. There is one additional place
where we use the SpdyStream (sorry that I forgot about this one).  When we queue
up a write, we create a SpdyIOBuffer, which needs to be associated with the
SpdyStream, as per the docs:

// Further, they need   
// to track the SpdyStream which they are associated with so that incremental   
// completion of the IO can notify the appropriate stream of completion.

You'll see this code in spdy_session.cc:

      in_flight_write_ = SpdyIOBuffer(buffer, size, priority, stream);

So I ended up tweaking the class to produce SpdyIOBuffers instead.  How does
that work for you?

[willchan no longer on Chromium, 2012-06-22 23:07:11]

On 2012/06/22 22:54:05, Ryan Hamilton wrote:
> On 2012/06/22 21:10:44, willchan wrote:
> >
>
https://chromiumcodereview.appspot.com/10448083/diff/22001/net/spdy/spdy_sess...
> > File net/spdy/spdy_session.h (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10448083/diff/22001/net/spdy/spdy_sess...
> > net/spdy/spdy_session.h:111: virtual SpdyStream* GetSpdyStream() = 0;
> > On 2012/06/22 16:17:32, Ryan Hamilton wrote:
> > > On 2012/06/21 23:51:03, willchan wrote:
> > > > Do you still need this one?
> > > 
> > > It is still used because when we decide that we need to close a stream, we
> > > remove the corresponding SpdyFrameProducer from the the write_queue.  To
do
> > this
> > > we look at the SpdyStream associated with each Producer.  If you would
> prefer,
> > > however, I could remove this method and add a IsAssociateWith(SpdyStream*)
> > > method, or some such?
> > 
> > I see. I think I'd rather keep it out of this interface and have the
> SpdySession
> > maintain the mapping of streams to SpdyFrameProducers, so it can delete
those
> > SpdyFrameProducers when the stream is canceled.
> 
> Whoops.  I did what you proposed but hit a glitch. There is one additional
place
> where we use the SpdyStream (sorry that I forgot about this one).  When we
queue
> up a write, we create a SpdyIOBuffer, which needs to be associated with the
> SpdyStream, as per the docs:
> 
> // Further, they need   
> // to track the SpdyStream which they are associated with so that incremental 
 
> // completion of the IO can notify the appropriate stream of completion.
> 
> You'll see this code in spdy_session.cc:
> 
>       in_flight_write_ = SpdyIOBuffer(buffer, size, priority, stream);
> 
> So I ended up tweaking the class to produce SpdyIOBuffers instead.  How does
> that work for you?

SGTM. Does the new protected member function need to be there? It looks like
it's only used for SpdyFrameProducers that are streams. Perhaps it should be in
spdy_stream.cc instead of SpdyFrameProducer?

[Ryan Hamilton, 2012-06-22 23:47:06]

On 2012/06/22 23:07:11, willchan wrote:
> On 2012/06/22 22:54:05, Ryan Hamilton wrote:
> > On 2012/06/22 21:10:44, willchan wrote:
> > >
> >
>
https://chromiumcodereview.appspot.com/10448083/diff/22001/net/spdy/spdy_sess...
> > > File net/spdy/spdy_session.h (right):
> > > 
> > >
> >
>
https://chromiumcodereview.appspot.com/10448083/diff/22001/net/spdy/spdy_sess...
> > > net/spdy/spdy_session.h:111: virtual SpdyStream* GetSpdyStream() = 0;
> > > On 2012/06/22 16:17:32, Ryan Hamilton wrote:
> > > > On 2012/06/21 23:51:03, willchan wrote:
> > > > > Do you still need this one?
> > > > 
> > > > It is still used because when we decide that we need to close a stream,
we
> > > > remove the corresponding SpdyFrameProducer from the the write_queue.  To
> do
> > > this
> > > > we look at the SpdyStream associated with each Producer.  If you would
> > prefer,
> > > > however, I could remove this method and add a
IsAssociateWith(SpdyStream*)
> > > > method, or some such?
> > > 
> > > I see. I think I'd rather keep it out of this interface and have the
> > SpdySession
> > > maintain the mapping of streams to SpdyFrameProducers, so it can delete
> those
> > > SpdyFrameProducers when the stream is canceled.
> > 
> > Whoops.  I did what you proposed but hit a glitch. There is one additional
> place
> > where we use the SpdyStream (sorry that I forgot about this one).  When we
> queue
> > up a write, we create a SpdyIOBuffer, which needs to be associated with the
> > SpdyStream, as per the docs:
> > 
> > // Further, they need   
> > // to track the SpdyStream which they are associated with so that
incremental 
>  
> > // completion of the IO can notify the appropriate stream of completion.
> > 
> > You'll see this code in spdy_session.cc:
> > 
> >       in_flight_write_ = SpdyIOBuffer(buffer, size, priority, stream);
> > 
> > So I ended up tweaking the class to produce SpdyIOBuffers instead.  How does
> > that work for you?
> 
> SGTM. Does the new protected member function need to be there? It looks like
> it's only used for SpdyFrameProducers that are streams. Perhaps it should be
in
> spdy_stream.cc instead of SpdyFrameProducer?

It's used it both subclasses.  I made it static, in case that's better...

[willchan no longer on Chromium, 2012-06-26 17:07:27]

No major comments left. Just a stylistic nit. I defer to rtenneti for the rest.

http://codereview.chromium.org/10448083/diff/36004/net/spdy/spdy_stream.cc
File net/spdy/spdy_stream.cc (right):

http://codereview.chromium.org/10448083/diff/36004/net/spdy/spdy_stream.cc#ne...
net/spdy/spdy_stream.cc:103: SpdySession::SpdyIOBufferProducer*
SpdyStream::CreateProducer() {
I don't feel strongly. I don't really know why I feel this way, but I don't like
how this has SpdyStream calling SpdySession::SetStreamHasWriteAvailable(this),
then SpdySession calls back here into the same SpdyStream. Meh, not a big deal,
feel free to ignore my comment, but if I wrote it, I'd just pass in both the
SpdyStream and the SpdyIOBufferProducer to the SpdySession so it doesn't have to
call back into the SpdyStream.

[Ryan Hamilton, 2012-06-26 17:55:24]

willchan: thanks for the final review
ramant: over to you!

http://codereview.chromium.org/10448083/diff/36004/net/spdy/spdy_stream.cc
File net/spdy/spdy_stream.cc (right):

http://codereview.chromium.org/10448083/diff/36004/net/spdy/spdy_stream.cc#ne...
net/spdy/spdy_stream.cc:103: SpdySession::SpdyIOBufferProducer*
SpdyStream::CreateProducer() {
On 2012/06/26 17:07:27, willchan wrote:
> I don't feel strongly. I don't really know why I feel this way, but I don't
like
> how this has SpdyStream calling SpdySession::SetStreamHasWriteAvailable(this),
> then SpdySession calls back here into the same SpdyStream. Meh, not a big
deal,
> feel free to ignore my comment, but if I wrote it, I'd just pass in both the
> SpdyStream and the SpdyIOBufferProducer to the SpdySession so it doesn't have
to
> call back into the SpdyStream.

Done.  Heh, fair enough.  Both approaches work for me, and since yours works
better for you that makes it better.  This also allowed me to make private the
ability to create a producer for a stream.

[ramant (doing other things), 2012-06-26 23:30:25]

lgtm

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_network_tran...
File net/spdy/spdy_network_transaction_spdy2_unittest.cc (right):

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_network_tran...
net/spdy/spdy_network_transaction_spdy2_unittest.cc:5583: // than they order
they were created.
nit: consider "they order" -> "the order". Sorry for not being an expert in
grammar.

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

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_session.cc#n...
net/spdy/spdy_session.cc:427: return CreateStreamImpl(url, priority,
spdy_stream, stream_net_log);
nit: "When expressions are wrapped, the operator should be on the end of the
broken line, not the start of the new line"

http://dev.chromium.org/developers/coding-style

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_session.cc#n...
net/spdy/spdy_session.cc:989: stream->OnClose(status);
nit: consider sharing of code between lines 976-979 with 986-988.

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_session.cc#n...
net/spdy/spdy_session.cc:1103: : frame_(frame),
nit: indent "RequestPriority ..."

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_session.cc#n...
net/spdy/spdy_session.cc:1125: write_queue_.push(producer);
nit: " = " should be in the previous line.

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_session.cc#n...
net/spdy/spdy_session.cc:1649: SendSettings(settings_map);
nit: delete "sent_settings_ = true;"?

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_session.cc#n...
net/spdy/spdy_session.cc:1682: SendSettings(settings_map_new);
nit: delete "sent_settings_ = true;"?

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_session.h#ne...
net/spdy/spdy_session.h:621: // producer from |write_queue_|
overly nit: punctuation at the end of "|write_queue_|".

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_stream.cc
File net/spdy/spdy_stream.cc (right):

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_stream.cc#ne...
net/spdy/spdy_stream.cc:100: 
overly nit: extra blank line.

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_stream.cc#ne...
net/spdy/spdy_stream.cc:115: SpdyCredentialControlFrame* frame =
session_->CreateCredentialFrame(
nit: should we DCHECK that "origin" ends with slash? Can we DCHECK
origin.length() is > 0.

overly nit: Consider "trim" -> "Trim" and punctuation. :-).

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Punctu...

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_stream.cc#ne...
net/spdy/spdy_stream.cc:127: set_stream_id(frame->stream_id());
nit: why call set_stream_id again? stream_id_ is non-zero and we had sent it to
CreateSynStream and it doesn't look like CreateSynStream changes the stream_id.

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_websocket_st...
File net/spdy/spdy_websocket_stream_spdy2_unittest.cc (right):

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_websocket_st...
net/spdy/spdy_websocket_stream_spdy2_unittest.cc:169: created_stream_id_ =
websocket_stream_->stream_->stream_id();
nit: record -> Record and consider punctuation.

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_websocket_st...
File net/spdy/spdy_websocket_stream_spdy3_unittest.cc (right):

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_websocket_st...
net/spdy/spdy_websocket_stream_spdy3_unittest.cc:169: created_stream_id_ =
websocket_stream_->stream_->stream_id();
nit: record -> Record

[Ryan Hamilton, 2012-06-27 16:58:28]

Thanks for the review!

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_network_tran...
File net/spdy/spdy_network_transaction_spdy2_unittest.cc (right):

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_network_tran...
net/spdy/spdy_network_transaction_spdy2_unittest.cc:5583: // than they order
they were created.
On 2012/06/26 23:30:25, ramant wrote:
> nit: consider "they order" -> "the order". Sorry for not being an expert in
> grammar.

Done.

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

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_session.cc#n...
net/spdy/spdy_session.cc:427: return CreateStreamImpl(url, priority,
spdy_stream, stream_net_log);
On 2012/06/26 23:30:25, ramant wrote:
> nit: "When expressions are wrapped, the operator should be on the end of the
> broken line, not the start of the new line"
> 
> http://dev.chromium.org/developers/coding-style

Done.

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_session.cc#n...
net/spdy/spdy_session.cc:989: stream->OnClose(status);
On 2012/06/26 23:30:25, ramant wrote:
> nit: consider sharing of code between lines 976-979 with 986-988.

Done.

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_session.cc#n...
net/spdy/spdy_session.cc:1103: : frame_(frame),
On 2012/06/26 23:30:25, ramant wrote:
> nit: indent "RequestPriority ..."

Done.

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_session.cc#n...
net/spdy/spdy_session.cc:1125: write_queue_.push(producer);
On 2012/06/26 23:30:25, ramant wrote:
> nit: " = " should be in the previous line.

Done.

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_session.cc#n...
net/spdy/spdy_session.cc:1125: write_queue_.push(producer);
On 2012/06/26 23:30:25, ramant wrote:
> nit: " = " should be in the previous line.

Done.

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_session.cc#n...
net/spdy/spdy_session.cc:1649: SendSettings(settings_map);
On 2012/06/26 23:30:25, ramant wrote:
> nit: delete "sent_settings_ = true;"?

Done.

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_session.cc#n...
net/spdy/spdy_session.cc:1682: SendSettings(settings_map_new);
On 2012/06/26 23:30:25, ramant wrote:
> nit: delete "sent_settings_ = true;"?

Done.

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_session.h#ne...
net/spdy/spdy_session.h:621: // producer from |write_queue_|
On 2012/06/26 23:30:25, ramant wrote:
> overly nit: punctuation at the end of "|write_queue_|".

Done.

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_stream.cc
File net/spdy/spdy_stream.cc (right):

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_stream.cc#ne...
net/spdy/spdy_stream.cc:100: 
On 2012/06/26 23:30:25, ramant wrote:
> overly nit: extra blank line.

Done.

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_stream.cc#ne...
net/spdy/spdy_stream.cc:115: SpdyCredentialControlFrame* frame =
session_->CreateCredentialFrame(
On 2012/06/26 23:30:25, ramant wrote:
> nit: should we DCHECK that "origin" ends with slash? Can we DCHECK
> origin.length() is > 0.
> 
> overly nit: Consider "trim" -> "Trim" and punctuation. :-).
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Punctu...

Done.

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_stream.cc#ne...
net/spdy/spdy_stream.cc:127: set_stream_id(frame->stream_id());
On 2012/06/26 23:30:25, ramant wrote:
> nit: why call set_stream_id again? stream_id_ is non-zero and we had sent it
to
> CreateSynStream and it doesn't look like CreateSynStream changes the
stream_id.

I *swear* I removed that line of code earlier, but I sure removed it now :>

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_websocket_st...
File net/spdy/spdy_websocket_stream_spdy2_unittest.cc (right):

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_websocket_st...
net/spdy/spdy_websocket_stream_spdy2_unittest.cc:169: created_stream_id_ =
websocket_stream_->stream_->stream_id();
On 2012/06/26 23:30:25, ramant wrote:
> nit: record -> Record and consider punctuation.

Done.

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_websocket_st...
File net/spdy/spdy_websocket_stream_spdy3_unittest.cc (right):

http://codereview.chromium.org/10448083/diff/37018/net/spdy/spdy_websocket_st...
net/spdy/spdy_websocket_stream_spdy3_unittest.cc:169: created_stream_id_ =
websocket_stream_->stream_->stream_id();
On 2012/06/26 23:30:25, ramant wrote:
> nit: record -> Record

Done.

[commit-bot: I haz the power, 2012-06-27 16:59:06]

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

[commit-bot: I haz the power, 2012-06-27 17:09:53]

Try job failure for 10448083-50001 (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...

[commit-bot: I haz the power, 2012-06-27 19:14:18]

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

[commit-bot: I haz the power, 2012-06-27 19:34:33]

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

[commit-bot: I haz the power, 2012-06-27 19:57:01]

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

[commit-bot: I haz the power, 2012-06-27 20:19:44]

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

[commit-bot: I haz the power, 2012-06-28 00:27:29]

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

[commit-bot: I haz the power, 2012-06-28 02:21:43]

Change committed as 144649