Add a QuicHttpStream class.

Add a QuicHttpStream class.


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

[Ryan Hamilton, 2012-11-17 01:38:04]

willchan: Since raman is out, can you take a look.

[willchan no longer on Chromium, 2012-11-17 03:13:38]

I'll land in Maui late Monday, and will take a look afterward.

[willchan no longer on Chromium, 2012-11-21 22:50:03]

Streaming first set of comments.

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_client_sess...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_client_sess...
net/quic/quic_client_session.cc:68: //closed_streams_.push_back(it->second);
?

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_client_sess...
File net/quic/quic_client_session.h (right):

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_client_sess...
net/quic/quic_client_session.h:25: virtual QuicReliableClientStream*
CreateOutgoingReliableStream() OVERRIDE;
Y'know, if we wanted to leverage it, we could use scoped_ptr here. Returning a
scoped_ptr is nicer semantically. I know that might be an issue when sync'ing
with the internal code. Serverside, they should use unique_ptr (I think that's
still disallowed, but we're getting there soon).

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_client_sess...
File net/quic/quic_client_session_test.cc (right):

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_client_sess...
net/quic/quic_client_session_test.cc:70:
EXPECT_TRUE(session_.CreateOutgoingReliableStream());
Is this just leaking?

[willchan no longer on Chromium, 2012-11-21 22:55:05]

Part 2. I'm going to do the core additions in part 3.

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_reliable_cl...
File net/quic/quic_reliable_client_stream.cc (right):

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_reliable_cl...
net/quic/quic_reliable_client_stream.cc:56: return
ReliableQuicStream::WriteData(data, fin);
This looks weird. Why are we overriding to just call the parent class's
implementation? If we expect to modify the implementation later, then this will
become more like implementation inheritance instead of composition. It's setting
off a code smell for me. Just calling this out so you're aware of it.

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_reliable_cl...
File net/quic/quic_reliable_client_stream.h (right):

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_reliable_cl...
net/quic/quic_reliable_client_stream.h:70: IPEndPoint GetPeerAddress();
Looks weird to me that this is non-const. Is there a reason for that?

[Ryan Hamilton, 2012-11-21 23:06:35]

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_client_sess...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_client_sess...
net/quic/quic_client_session.cc:68: //closed_streams_.push_back(it->second);
On 2012/11/21 22:50:03, willchan wrote:
> ?

Removed.

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_client_sess...
File net/quic/quic_client_session.h (right):

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_client_sess...
net/quic/quic_client_session.h:25: virtual QuicReliableClientStream*
CreateOutgoingReliableStream() OVERRIDE;
On 2012/11/21 22:50:03, willchan wrote:
> Y'know, if we wanted to leverage it, we could use scoped_ptr here. Returning a
> scoped_ptr is nicer semantically. I know that might be an issue when sync'ing
> with the internal code. Serverside, they should use unique_ptr (I think that's
> still disallowed, but we're getting there soon).

I don't think that would make sense, because the session owns the stream, not
the caller.

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_client_sess...
File net/quic/quic_client_session_test.cc (right):

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_client_sess...
net/quic/quic_client_session_test.cc:70:
EXPECT_TRUE(session_.CreateOutgoingReliableStream());
On 2012/11/21 22:50:03, willchan wrote:
> Is this just leaking?

No, the stream is owned by the session.

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_reliable_cl...
File net/quic/quic_reliable_client_stream.cc (right):

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_reliable_cl...
net/quic/quic_reliable_client_stream.cc:56: return
ReliableQuicStream::WriteData(data, fin);
On 2012/11/21 22:55:05, willchan wrote:
> This looks weird. Why are we overriding to just call the parent class's
> implementation? If we expect to modify the implementation later, then this
will
> become more like implementation inheritance instead of composition. It's
setting
> off a code smell for me. Just calling this out so you're aware of it.

I overrode it so that I could make it public.  Would you prefer I make the base
class method public instead?

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_reliable_cl...
File net/quic/quic_reliable_client_stream.h (right):

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_reliable_cl...
net/quic/quic_reliable_client_stream.h:70: IPEndPoint GetPeerAddress();
On 2012/11/21 22:55:05, willchan wrote:
> Looks weird to me that this is non-const. Is there a reason for that?

Hm.  Perhaps it could be.  The issue is that the implementation calls session()
wich is non const (since it returns a pointer to the session).  I guess the
solution would be a const session() accessor?

[willchan no longer on Chromium, 2012-11-21 23:07:04]

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_http_stream.h
File net/quic/quic_http_stream.h (right):

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_http_stream...
net/quic/quic_http_stream.h:89: QuicReliableClientStream* stream_;  //
Non-owning.
Is there a comment somewhere that describes the ownership relationships between
the core classes, as we discussed offline in person & email? That would help a
lot as a class or file level comment.

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_http_stream...
net/quic/quic_http_stream.h:92: const HttpRequestInfo* request_info_;
This is also non-owning. I think that this must outlive the stream is part of
the HttpStream contract, but was never really documented. Perhaps updating
http_stream.h to document this would be great.

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_http_stream...
net/quic/quic_http_stream.h:94: std::string request_;
I can't seem to find where this gets used. What's it for?

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_http_stream...
net/quic/quic_http_stream.h:96: // The request body to send, if any, owned by
the caller.
Like HttpRequestInfo, we should document this in the HttpStream contract.

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_http_stream...
net/quic/quic_http_stream.h:100: // when a the reponse headers are read.  It is
not owned by this stream.
s/reponse/response/
Again, document ownership in HttpStream contract.

Perhaps group all of these together with a block comment saying this group of
variables is owned by the consumer of HttpStream.

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_http_stream...
net/quic/quic_http_stream.h:105: scoped_refptr<GrowableIOBuffer> read_buf_;
I really hate GrowableIOBuffer and DrainableIOBuffer. They exist for good
reasons, since we needed to provide things compatible with the IOBuffer
interface. If you don't need that, I'd prefer if you don't use these classes.

Push back since you understand project priorities here better than I, but I'm
concerned about these classes in particular, because they have a tendency to
leak out through APIs, so it might take nontrivial time to fix later on. At this
level, we need to be very careful about buffer management to reduce copies, so
these classes are very important.

I don't have time to think about it right now, but you probably do :) Please
consider this, and if you're comfortable with it, we can keep it as is.

[willchan no longer on Chromium, 2012-11-21 23:27:30]

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_http_stream.cc
File net/quic/quic_http_stream.cc (right):

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_http_stream...
net/quic/quic_http_stream.cc:174: bool QuicHttpStream::CanFindEndOfResponse()
const {
Would be great to clean up this method name since it's confusing.

[Ryan Hamilton, 2012-11-21 23:35:09]

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_http_stream.h
File net/quic/quic_http_stream.h (right):

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_http_stream...
net/quic/quic_http_stream.h:89: QuicReliableClientStream* stream_;  //
Non-owning.
On 2012/11/21 23:07:04, willchan wrote:
> Is there a comment somewhere that describes the ownership relationships
between
> the core classes, as we discussed offline in person & email? That would help a
> lot as a class or file level comment.

There is not.  I'll update the comments in the header files.

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_http_stream...
net/quic/quic_http_stream.h:92: const HttpRequestInfo* request_info_;
On 2012/11/21 23:07:04, willchan wrote:
> This is also non-owning. I think that this must outlive the stream is part of
> the HttpStream contract, but was never really documented. Perhaps updating
> http_stream.h to document this would be great.

updated http_stream_base.h

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_http_stream...
net/quic/quic_http_stream.h:94: std::string request_;
On 2012/11/21 23:07:04, willchan wrote:
> I can't seem to find where this gets used. What's it for?

It is the serialized HTTP request which is created in SendRequest and used
(later) in DoSendHeaders.  (Soon I'll switch to a SPDY serialization for the
request, so this will likely change).

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_http_stream...
net/quic/quic_http_stream.h:96: // The request body to send, if any, owned by
the caller.
On 2012/11/21 23:07:04, willchan wrote:
> Like HttpRequestInfo, we should document this in the HttpStream contract.

Updated http_stream_base.h

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_http_stream...
net/quic/quic_http_stream.h:100: // when a the reponse headers are read.  It is
not owned by this stream.
On 2012/11/21 23:07:04, willchan wrote:
> s/reponse/response/
> Again, document ownership in HttpStream contract.
> 
> Perhaps group all of these together with a block comment saying this group of
> variables is owned by the consumer of HttpStream.

Done.

https://codereview.chromium.org/11364068/diff/18001/net/quic/quic_http_stream...
net/quic/quic_http_stream.h:105: scoped_refptr<GrowableIOBuffer> read_buf_;
On 2012/11/21 23:07:04, willchan wrote:
> I really hate GrowableIOBuffer and DrainableIOBuffer. They exist for good
> reasons, since we needed to provide things compatible with the IOBuffer
> interface. If you don't need that, I'd prefer if you don't use these classes.
> 
> Push back since you understand project priorities here better than I, but I'm
> concerned about these classes in particular, because they have a tendency to
> leak out through APIs, so it might take nontrivial time to fix later on. At
this
> level, we need to be very careful about buffer management to reduce copies, so
> these classes are very important.
> 
> I don't have time to think about it right now, but you probably do :) Please
> consider this, and if you're comfortable with it, we can keep it as is.

Well, I definitely don't *need* GrowableIOBuffer, but it's *really* convenient. 
In order to make use of the existing HTTP response parsing code, I need to have
the complete HTTP response in a single block of memory.  The GrowableIOBuffer
seems to be ideal for this.  

Good point about the need to avoid buffer copies.  I wasn't thinking yet about
the performance.  Eventually (soon) i'll rip out the HTTP parsing and replace it
with SPDY parsing.  When that happen we won't need this buffer at all (Since
we'll simply send it to a SpdyFramer).  Is it ok to keep it around for now?

[willchan no longer on Chromium, 2012-11-22 00:46:14]

https://codereview.chromium.org/11364068/diff/16017/net/quic/quic_http_stream.cc
File net/quic/quic_http_stream.cc (right):

https://codereview.chromium.org/11364068/diff/16017/net/quic/quic_http_stream...
net/quic/quic_http_stream.cc:68: if (request_body_stream_ &&
(request_body_stream_->size() ||
I know you're just copying from SpdyHttpStream. This is fine for now. Long-term,
we should revisit this. Really, similar to how we talked about the
SpdyFrameProducer thingy, we should do the same with this.

https://codereview.chromium.org/11364068/diff/16017/net/quic/quic_http_stream...
net/quic/quic_http_stream.cc:372: int QuicHttpStream::DoSendBody(int rv) {
Can you add an empty DoSendBodyComplete() so we maintain convention or always
DoFoo() and DoFooComplete()?

And DoSendBody() shouldn't have an input, right? None of our other DoFoo()s have
it. Maybe you need a member variable.

https://codereview.chromium.org/11364068/diff/16017/net/quic/quic_http_stream...
net/quic/quic_http_stream.cc:379: return ERR_FAILED;
ERR_UNEXPECTED indicates a bug. You can ask wtc as to why we do this, I kinda
forget.

https://codereview.chromium.org/11364068/diff/16017/net/quic/quic_http_stream...
net/quic/quic_http_stream.cc:410: return ERR_FAILED;
Ditto on ERR_UNEXPECTED

https://codereview.chromium.org/11364068/diff/16017/net/quic/quic_http_stream...
net/quic/quic_http_stream.cc:426: }
I was going to ask about validation of |delta| here, but since this is going to
die Real Soon Now anyway, whatever.

[Ryan Hamilton, 2012-11-22 00:58:53]

https://codereview.chromium.org/11364068/diff/16017/net/quic/quic_http_stream.cc
File net/quic/quic_http_stream.cc (right):

https://codereview.chromium.org/11364068/diff/16017/net/quic/quic_http_stream...
net/quic/quic_http_stream.cc:372: int QuicHttpStream::DoSendBody(int rv) {
On 2012/11/22 00:46:14, willchan wrote:
> Can you add an empty DoSendBodyComplete() so we maintain convention or always
> DoFoo() and DoFooComplete()?

Done.  Good point.


> And DoSendBody() shouldn't have an input, right? None of our other DoFoo()s
have
> it. Maybe you need a member variable.

Removed it.  Wasn't actually needed and was a result of some earlier version of
the code.  Nice catch.

https://codereview.chromium.org/11364068/diff/16017/net/quic/quic_http_stream...
net/quic/quic_http_stream.cc:379: return ERR_FAILED;
On 2012/11/22 00:46:14, willchan wrote:
> ERR_UNEXPECTED indicates a bug. You can ask wtc as to why we do this, I kinda
> forget.

Done.

https://codereview.chromium.org/11364068/diff/16017/net/quic/quic_http_stream...
net/quic/quic_http_stream.cc:379: return ERR_FAILED;
On 2012/11/22 00:46:14, willchan wrote:
> ERR_UNEXPECTED indicates a bug. You can ask wtc as to why we do this, I kinda
> forget.

Done.

https://codereview.chromium.org/11364068/diff/16017/net/quic/quic_http_stream...
net/quic/quic_http_stream.cc:410: return ERR_FAILED;
On 2012/11/22 00:46:14, willchan wrote:
> Ditto on ERR_UNEXPECTED

Done.

https://codereview.chromium.org/11364068/diff/16017/net/quic/quic_http_stream...
net/quic/quic_http_stream.cc:426: }
On 2012/11/22 00:46:14, willchan wrote:
> I was going to ask about validation of |delta| here, but since this is going
to
> die Real Soon Now anyway, whatever.

Good deal.

[willchan no longer on Chromium, 2012-11-22 03:09:34]

https://codereview.chromium.org/11364068/diff/16017/net/quic/quic_http_stream...
File net/quic/quic_http_stream_test.cc (right):

https://codereview.chromium.org/11364068/diff/16017/net/quic/quic_http_stream...
net/quic/quic_http_stream_test.cc:38: const char kUploadData[] = "hello world!";
Unlike the internal code which probably links each test.cc into a separate
executable, we link all ours together. That means you've got to worry about
symbol collision. kUploadData is kinda generic. Really we should be using
anonymous namespaces in these test.cc files.

[Ryan Hamilton, 2012-11-22 03:16:03]

https://codereview.chromium.org/11364068/diff/16017/net/quic/quic_http_stream...
File net/quic/quic_http_stream_test.cc (right):

https://codereview.chromium.org/11364068/diff/16017/net/quic/quic_http_stream...
net/quic/quic_http_stream_test.cc:38: const char kUploadData[] = "hello world!";
On 2012/11/22 03:09:34, willchan wrote:
> Unlike the internal code which probably links each test.cc into a separate
> executable, we link all ours together. That means you've got to worry about
> symbol collision. kUploadData is kinda generic. Really we should be using
> anonymous namespaces in these test.cc files.

I thought that const provided static linkage?

http://stackoverflow.com/questions/998425/why-does-const-imply-internal-linka...

[willchan no longer on Chromium, 2012-11-22 03:17:48]

https://codereview.chromium.org/11364068/diff/16017/net/quic/quic_http_stream...
File net/quic/quic_http_stream_test.cc (right):

https://codereview.chromium.org/11364068/diff/16017/net/quic/quic_http_stream...
net/quic/quic_http_stream_test.cc:38: const char kUploadData[] = "hello world!";
On 2012/11/22 03:16:04, Ryan Hamilton wrote:
> On 2012/11/22 03:09:34, willchan wrote:
> > Unlike the internal code which probably links each test.cc into a separate
> > executable, we link all ours together. That means you've got to worry about
> > symbol collision. kUploadData is kinda generic. Really we should be using
> > anonymous namespaces in these test.cc files.
> 
> I thought that const provided static linkage?
> 
>
http://stackoverflow.com/questions/998425/why-does-const-imply-internal-linka...

Indeed, I forgot that. But the other stuff in this section is similarly
generically named, like TestSession and TestConnection.

[Ryan Hamilton, 2012-11-22 03:52:45]

I also switch to "using ReliableClientStream::WriteData" as we discussed
offline.

https://codereview.chromium.org/11364068/diff/16017/net/quic/quic_http_stream...
File net/quic/quic_http_stream_test.cc (right):

https://codereview.chromium.org/11364068/diff/16017/net/quic/quic_http_stream...
net/quic/quic_http_stream_test.cc:38: const char kUploadData[] = "hello world!";
On 2012/11/22 03:17:48, willchan wrote:
> On 2012/11/22 03:16:04, Ryan Hamilton wrote:
> > On 2012/11/22 03:09:34, willchan wrote:
> > > Unlike the internal code which probably links each test.cc into a separate
> > > executable, we link all ours together. That means you've got to worry
about
> > > symbol collision. kUploadData is kinda generic. Really we should be using
> > > anonymous namespaces in these test.cc files.
> > 
> > I thought that const provided static linkage?
> > 
> >
>
http://stackoverflow.com/questions/998425/why-does-const-imply-internal-linka...
> 
> Indeed, I forgot that. But the other stuff in this section is similarly
> generically named, like TestSession and TestConnection.

Good point.  Moved into an anon namespace.

[willchan no longer on Chromium, 2012-11-23 03:17:43]

https://codereview.chromium.org/11364068/diff/19037/net/http/http_stream_base.h
File net/http/http_stream_base.h (right):

https://codereview.chromium.org/11364068/diff/19037/net/http/http_stream_base...
net/http/http_stream_base.h:94: // TODO(rch): Rename this method.
Can you explain why this should be renamed (I agree as we discussed offline,
just want it written here so people know why).

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_client_sess...
File net/quic/quic_client_session.h (right):

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_client_sess...
net/quic/quic_client_session.h:40: QuicCryptoClientStream crypto_stream_;
Is this a magic stream?

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_http_stream.cc
File net/quic/quic_http_stream.cc (right):

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_http_stream...
net/quic/quic_http_stream.cc:49: int QuicHttpStream::SendRequest(const
HttpRequestHeaders& request_headers,
It looks to me that what you're doing here with a state machine is superior to
the SpdyHttpStream approach. If we share the implementation, perhaps it'll let
us clean up SpdyHttpStream and SpdyStream's interaction as well.

Just thought I'd point it out as a good direction for future cleanup work.

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_http_stream...
net/quic/quic_http_stream.cc:115: int QuicHttpStream::ReadResponseBody(
When I read this implementation which is basically copied wholesale from
SpdyHttpStream, I wonder if in the future there's room to share part of the
implementation. Food for thought.

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_http_stream...
net/quic/quic_http_stream.cc:434: BufferResponseBody(read_buf_->data(), delta);
I found this weird (ParseResponseHeaders() can also buffer the response body),
but since this will be going away soon as discussed offline, it's ok.

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_http_stream...
File net/quic/quic_http_stream_test.cc (right):

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_http_stream...
net/quic/quic_http_stream_test.cc:42: class TestSession : public
QuicClientSession {
Can you name these TestQuicSession and TestQuicConnection? It wasn't obvious
what they were at first.

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_http_stream...
net/quic/quic_http_stream_test.cc:82: guid_(2),
What's up with this guy, is he a constant? He seems to be used in a constant
manner.

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_http_stream...
net/quic/quic_http_stream_test.cc:132: helper_ = new
QuicConnectionHelper(runner_.get(), &clock_, socket_.get());
Why is helper_ a member variable? I don't see it getting used anywhere else.

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_http_stream...
net/quic/quic_http_stream_test.cc:278: char kResponseHeaders[] = "HTTP/1.1 404
OK\r\n"
Can this be a const char[]?

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_http_stream...
net/quic/quic_http_stream_test.cc:297: 
Add a test for response headers and body in the same QUIC packet.

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_reliable_cl...
File net/quic/quic_reliable_client_stream.cc (right):

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_reliable_cl...
net/quic/quic_reliable_client_stream.cc:26: // TODO(rch): buffere data if we
don't have a delegate.
s/buffere/buffer/

https://codereview.chromium.org/11364068/diff/19037/net/quic/reliable_quic_st...
File net/quic/reliable_quic_stream.cc (right):

https://codereview.chromium.org/11364068/diff/19037/net/quic/reliable_quic_st...
net/quic/reliable_quic_stream.cc:119: LOG(INFO) << "Closing stream: " << id();
Can we use DVLOGs? There's a big push against LOG stuff for various reasons like
binary bloat and what not.

[willchan no longer on Chromium, 2012-11-23 03:22:57]

OK, my last new comment! I'm going to revisit all the ones you've answered to
make sure they're kosher. And as discussed offline, I wasn't completely
comfortable with the test setup and QUIC packet stuff, so it'd be great if jar@
took a quick look at that part.

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_http_stream...
File net/quic/quic_http_stream_test.cc (right):

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_http_stream...
net/quic/quic_http_stream_test.cc:352: }
Can you add a test for synchronous completion of the whole request body? Just to
make sure the state machine will always be able to handle that all happening
synchronously.

I'm thinking of when UploadDataStream is just backed by a string in memory so it
completes synchronously. I think that even in the future refactor, this test
case will be relevant, so we should have it here.

[Ryan Hamilton, 2012-11-23 04:19:53]

https://codereview.chromium.org/11364068/diff/19037/net/http/http_stream_base.h
File net/http/http_stream_base.h (right):

https://codereview.chromium.org/11364068/diff/19037/net/http/http_stream_base...
net/http/http_stream_base.h:94: // TODO(rch): Rename this method.
On 2012/11/23 03:17:43, willchan wrote:
> Can you explain why this should be renamed (I agree as we discussed offline,
> just want it written here so people know why).

Done.

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_client_sess...
File net/quic/quic_client_session.h (right):

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_client_sess...
net/quic/quic_client_session.h:40: QuicCryptoClientStream crypto_stream_;
On 2012/11/23 03:17:43, willchan wrote:
> Is this a magic stream?

Yes, stream 1 is the "crypto" stream.  We run a special crypto framer on top of
this stream to parse the handshake messages.

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_http_stream.cc
File net/quic/quic_http_stream.cc (right):

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_http_stream...
net/quic/quic_http_stream.cc:49: int QuicHttpStream::SendRequest(const
HttpRequestHeaders& request_headers,
On 2012/11/23 03:17:43, willchan wrote:
> It looks to me that what you're doing here with a state machine is superior to
> the SpdyHttpStream approach. If we share the implementation, perhaps it'll let
> us clean up SpdyHttpStream and SpdyStream's interaction as well.
> 
> Just thought I'd point it out as a good direction for future cleanup work.

Good point.

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_http_stream...
net/quic/quic_http_stream.cc:115: int QuicHttpStream::ReadResponseBody(
On 2012/11/23 03:17:43, willchan wrote:
> When I read this implementation which is basically copied wholesale from
> SpdyHttpStream, I wonder if in the future there's room to share part of the
> implementation. Food for thought.

Definitely a good idea.  Seems quite plausible.  I'll investigate doing that
after I add SPDY framing support, if that works for you.

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_http_stream...
net/quic/quic_http_stream.cc:434: BufferResponseBody(read_buf_->data(), delta);
On 2012/11/23 03:17:43, willchan wrote:
> I found this weird (ParseResponseHeaders() can also buffer the response body),
> but since this will be going away soon as discussed offline, it's ok.

Thanks.  The SPDY framing is next on my list.

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_http_stream...
File net/quic/quic_http_stream_test.cc (right):

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_http_stream...
net/quic/quic_http_stream_test.cc:42: class TestSession : public
QuicClientSession {
On 2012/11/23 03:17:43, willchan wrote:
> Can you name these TestQuicSession and TestQuicConnection? It wasn't obvious
> what they were at first.

Done.  (Though it turned out the Session wasn't even used :|)

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_http_stream...
net/quic/quic_http_stream_test.cc:82: guid_(2),
On 2012/11/23 03:17:43, willchan wrote:
> What's up with this guy, is he a constant? He seems to be used in a constant
> manner.

Yeah, he's const.  I added a const annotation to the member declaration.

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_http_stream...
net/quic/quic_http_stream_test.cc:132: helper_ = new
QuicConnectionHelper(runner_.get(), &clock_, socket_.get());
On 2012/11/23 03:17:43, willchan wrote:
> Why is helper_ a member variable? I don't see it getting used anywhere else.

Inlined it into the connection_ initialization.

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_http_stream...
net/quic/quic_http_stream_test.cc:278: char kResponseHeaders[] = "HTTP/1.1 404
OK\r\n"
On 2012/11/23 03:17:43, willchan wrote:
> Can this be a const char[]?

Done.

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_http_stream...
net/quic/quic_http_stream_test.cc:297: 
On 2012/11/23 03:17:43, willchan wrote:
> Add a test for response headers and body in the same QUIC packet.

Done.

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_http_stream...
net/quic/quic_http_stream_test.cc:352: }
On 2012/11/23 03:22:58, willchan wrote:
> Can you add a test for synchronous completion of the whole request body? Just
to
> make sure the state machine will always be able to handle that all happening
> synchronously.
> 
> I'm thinking of when UploadDataStream is just backed by a string in memory so
it
> completes synchronously. I think that even in the future refactor, this test
> case will be relevant, so we should have it here.

Actually, the existing test already does that.  Cool?

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_reliable_cl...
File net/quic/quic_reliable_client_stream.cc (right):

https://codereview.chromium.org/11364068/diff/19037/net/quic/quic_reliable_cl...
net/quic/quic_reliable_client_stream.cc:26: // TODO(rch): buffere data if we
don't have a delegate.
On 2012/11/23 03:17:43, willchan wrote:
> s/buffere/buffer/

Done.

https://codereview.chromium.org/11364068/diff/19037/net/quic/reliable_quic_st...
File net/quic/reliable_quic_stream.cc (right):

https://codereview.chromium.org/11364068/diff/19037/net/quic/reliable_quic_st...
net/quic/reliable_quic_stream.cc:119: LOG(INFO) << "Closing stream: " << id();
On 2012/11/23 03:17:43, willchan wrote:
> Can we use DVLOGs? There's a big push against LOG stuff for various reasons
like
> binary bloat and what not.

Switched to DLOG()  is that OK?

[Ryan Hamilton, 2012-11-26 17:36:36]

Hi Jim,

From willchan on his last review, "I wasn't completely comfortable with the test
setup and QUIC packet stuff, so it'd be great if jar@ took a quick look at that
part."  Can you take a look?

Cheers,

Ryan

[jar (doing other things), 2012-12-03 20:16:51]

quic_http_stream_test LGTM (with upstream changes).

https://codereview.chromium.org/11364068/diff/22008/net/quic/quic_http_stream...
File net/quic/quic_http_stream_test.cc (right):

https://codereview.chromium.org/11364068/diff/22008/net/quic/quic_http_stream...
net/quic/quic_http_stream_test.cc:45: IPEndPoint address,
nit: indent

https://codereview.chromium.org/11364068/diff/22008/net/quic/quic_http_stream...
net/quic/quic_http_stream_test.cc:82: Initialize();
nit: add comment "Do null initialization to help some tests."

https://codereview.chromium.org/11364068/diff/22008/net/quic/quic_http_stream...
net/quic/quic_http_stream_test.cc:169: bool with_ack) {
nit: (per your suggestion): with_congestion_info

https://codereview.chromium.org/11364068/diff/22008/net/quic/quic_http_stream...
net/quic/quic_http_stream_test.cc:338: AddWrite(SYNCHRONOUS,
ConstructDataPacket(2, true, strlen(kRequestData),
nit: use kFin and kNoFin

[commit-bot: I haz the power, 2012-12-03 20:53:31]

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

[commit-bot: I haz the power, 2012-12-03 20:53:39]

Presubmit check for 11364068-15038 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit Warnings **
License must match:
.*? Copyright (\(c\) )?(2012|2011|2010|2009|2008|2007|2006|2006-2008) The
Chromium Authors\. All rights reserved\.\n.*? Use of this source code is
governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?:
\*/)?\n
Found a bad license header in these files:
  net/quic/quic_http_stream.cc

Presubmit checks took 2.1s to calculate.

[commit-bot: I haz the power, 2012-12-03 23:09:23]

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

[commit-bot: I haz the power, 2012-12-03 23:37:52]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-04 00:40:06]

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

[commit-bot: I haz the power, 2012-12-04 01:06:27]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-12-04 03:24:48]

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

[commit-bot: I haz the power, 2012-12-04 06:08:06]

Retried try job too often on mac_rel for step(s) browser_tests

[commit-bot: I haz the power, 2012-12-04 15:09:23]

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

[commit-bot: I haz the power, 2012-12-04 15:56:21]

Change committed as 170968