Refactor SpdyHttpStream to implement HttpStream. Required adding a new...

Refactor SpdyHttpStream to implement HttpStream.  Required adding a new
method to HttpStream, and refactoring BasicHttpStream as well.

BUG=50268

[rch (use chromium not google), 2010-07-27 18:53:52]

I've refactored HttpStream, SpdyHttpStream and HttpBasicStream so that
SpdyHttpStream now implements (a slightly wider) HttpStream interface.

[Mike Belshe, 2010-07-27 19:40:35]

This looks great.

Thanks!!!

Just the one question about the SendRequest args.

http://codereview.chromium.org/3079002/diff/15002/31005
File net/http/http_stream.h (right):

http://codereview.chromium.org/3079002/diff/15002/31005#newcode39
net/http/http_stream.h:39: virtual int SendRequest(const std::string&
request_headers,
I think we should remove request_headers from here.  The logic in
HTN::SendRequest to construct them should be movable into HttpBasicStream
without too much effort.

http://codereview.chromium.org/3079002/diff/15002/31005#newcode55
net/http/http_stream.h:55: virtual const HttpResponseInfo* GetResponseInfo()
const = 0;
good

[rch (use chromium not google), 2010-07-27 20:50:45]

http://codereview.chromium.org/3079002/diff/15002/31005
File net/http/http_stream.h (right):

http://codereview.chromium.org/3079002/diff/15002/31005#newcode39
net/http/http_stream.h:39: virtual int SendRequest(const std::string&
request_headers,
On 2010/07/27 19:40:35, Mike Belshe wrote:
> I think we should remove request_headers from here.  The logic in
> HTN::SendRequest to construct them should be movable into HttpBasicStream
> without too much effort.

I totally agree with your vision here.  The code which sets the headers is
currently in HttpNetworkTransaction::DoSendRequest().  In order to construct
them, it consults a number of member fields/methods:
* ShouldApplyProxyAuth
* ShouldApplyServerAuth
* auth_controllers_
* using_ssl
* proxy_info_.is_http()
* session_->network_delegate
I'm guessing we don't want to pass all of this in.  

In order to allow the HttpBasicStream to do the needful, we could perhaps pass
in:
* authorization_headers
* network_delegate
* using_proxy (boolean - required to switch between the path and the spec URL).

Does this sound reasonable, or can you think of a better approach?

[mbelshe, 2010-07-27 20:58:24]

On Tue, Jul 27, 2010 at 1:50 PM, <rch@google.com> wrote:

>
> http://codereview.chromium.org/3079002/diff/15002/31005
> File net/http/http_stream.h (right):
>
> http://codereview.chromium.org/3079002/diff/15002/31005#newcode39
> net/http/http_stream.h:39: virtual int SendRequest(const std::string&
> request_headers,
> On 2010/07/27 19:40:35, Mike Belshe wrote:
>
>> I think we should remove request_headers from here.  The logic in
>> HTN::SendRequest to construct them should be movable into
>>
> HttpBasicStream
>
>> without too much effort.
>>
>
> I totally agree with your vision here.  The code which sets the headers
> is currently in HttpNetworkTransaction::DoSendRequest().  In order to
> construct them, it consults a number of member fields/methods:
> * ShouldApplyProxyAuth
> * ShouldApplyServerAuth
> * auth_controllers_
> * using_ssl
> * proxy_info_.is_http()
> * session_->network_delegate
> I'm guessing we don't want to pass all of this in.
>

Although ugly, I think we do :-(  It's better than the string, no?

Also, many of these may land on the request_ already?  Not sure.

Mike


>
> In order to allow the HttpBasicStream to do the needful, we could
> perhaps pass in:
> * authorization_headers
> * network_delegate
> * using_proxy (boolean - required to switch between the path and the
> spec URL).
>
> Does this sound reasonable, or can you think of a better approach?
>
>
> http://codereview.chromium.org/3079002/show
>

[vandebo (ex-Chrome), 2010-07-27 21:10:47]

On 2010/07/27 20:58:24, mbelshe wrote:
> On Tue, Jul 27, 2010 at 1:50 PM, <mailto:rch@google.com> wrote:
> 
> >
> > http://codereview.chromium.org/3079002/diff/15002/31005
> > File net/http/http_stream.h (right):
> >
> > http://codereview.chromium.org/3079002/diff/15002/31005#newcode39
> > net/http/http_stream.h:39: virtual int SendRequest(const std::string&
> > request_headers,
> > On 2010/07/27 19:40:35, Mike Belshe wrote:
> >
> >> I think we should remove request_headers from here.  The logic in
> >> HTN::SendRequest to construct them should be movable into
> >>
> > HttpBasicStream
> >
> >> without too much effort.
> >>
> >
> > I totally agree with your vision here.  The code which sets the headers
> > is currently in HttpNetworkTransaction::DoSendRequest().  In order to
> > construct them, it consults a number of member fields/methods:
> > * ShouldApplyProxyAuth
> > * ShouldApplyServerAuth
> > * auth_controllers_
> > * using_ssl
> > * proxy_info_.is_http()
> > * session_->network_delegate
> > I'm guessing we don't want to pass all of this in.
> >
> 
> Although ugly, I think we do :-(  It's better than the string, no?
> 
> Also, many of these may land on the request_ already?  Not sure.

Is there actually something to be gained from the ugliness?

http://codereview.chromium.org/3079002/diff/15002/31001
File net/http/http_basic_stream.cc (right):

http://codereview.chromium.org/3079002/diff/15002/31001#newcode26
net/http/http_basic_stream.cc:26: return parser_->SendRequest(
DCHECK that Init has been called?

http://codereview.chromium.org/3079002/diff/15002/31002
File net/http/http_basic_stream.h (right):

http://codereview.chromium.org/3079002/diff/15002/31002#newcode61
net/http/http_basic_stream.h:61: HttpRequestInfo request_info_;
Instead of storing the request_info, you could initialize the HttpStreamParser
in the InitializeStream method and change the HttpStreamParser to take the
request_info in the constructor.

http://codereview.chromium.org/3079002/diff/15002/31003
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/3079002/diff/15002/31003#newcode1062
net/http/http_network_transaction.cc:1062: return
http_stream_->SendRequest(request_headers_,
You haven't called Init on this stream yet.

http://codereview.chromium.org/3079002/diff/15002/31004
File net/http/http_proxy_client_socket.cc (right):

http://codereview.chromium.org/3079002/diff/15002/31004#newcode327
net/http/http_proxy_client_socket.cc:327:
http_stream_->InitializeStream(request_, &io_callback_);
Since this explicitly instantiats a HttpBasicStream, just pass NULL for the
callback.

http://codereview.chromium.org/3079002/diff/15002/31005
File net/http/http_stream.h (right):

http://codereview.chromium.org/3079002/diff/15002/31005#newcode30
net/http/http_stream.h:30: // Initialize stream.  Must be called before calling
SendRequest().
You should document the return values here.

http://codereview.chromium.org/3079002/diff/15002/31005#newcode31
net/http/http_stream.h:31: virtual int InitializeStream(const HttpRequestInfo&
request_info,
Now that this has an Init method, the NetLog should probably be passed in here
instead of the individual constructors.  We sometimes don't have the NetLog we
intend to use until we are ready to actually do something.

[rch (use chromium not google), 2010-07-27 21:31:06]

http://codereview.chromium.org/3079002/diff/15002/31001
File net/http/http_basic_stream.cc (right):

http://codereview.chromium.org/3079002/diff/15002/31001#newcode26
net/http/http_basic_stream.cc:26: return parser_->SendRequest(
On 2010/07/27 21:10:47, vandebo wrote:
> DCHECK that Init has been called?

Since request_info_ is an inline object, I don't see an obvious way to do this
without adding a boolean.  What do you think?  I could change it to be a
scoped_ptr, I guess, and check for NULL?

http://codereview.chromium.org/3079002/diff/15002/31002
File net/http/http_basic_stream.h (right):

http://codereview.chromium.org/3079002/diff/15002/31002#newcode61
net/http/http_basic_stream.h:61: HttpRequestInfo request_info_;
On 2010/07/27 21:10:47, vandebo wrote:
> Instead of storing the request_info, you could initialize the HttpStreamParser
> in the InitializeStream method and change the HttpStreamParser to take the
> request_info in the constructor.

I could definitely do that.  That would require that we save the
ClientSocketHandle from the constructor so that we can use it in
InitializeStream().  Does that strike you as better?  (We'd also have to move
the net_log, but you mention that you'd prefer that in any case.)

http://codereview.chromium.org/3079002/diff/15002/31003
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/3079002/diff/15002/31003#newcode1062
net/http/http_network_transaction.cc:1062: return
http_stream_->SendRequest(request_headers_,
On 2010/07/27 21:10:47, vandebo wrote:
> You haven't called Init on this stream yet.

Done.  Whoops, I noticed this myself just now.

http://codereview.chromium.org/3079002/diff/15002/31004
File net/http/http_proxy_client_socket.cc (right):

http://codereview.chromium.org/3079002/diff/15002/31004#newcode327
net/http/http_proxy_client_socket.cc:327:
http_stream_->InitializeStream(request_, &io_callback_);
On 2010/07/27 21:10:47, vandebo wrote:
> Since this explicitly instantiats a HttpBasicStream, just pass NULL for the
> callback.

Done.  I'll make the same change in http_network_transaction.cc

http://codereview.chromium.org/3079002/diff/15002/31005
File net/http/http_stream.h (right):

http://codereview.chromium.org/3079002/diff/15002/31005#newcode30
net/http/http_stream.h:30: // Initialize stream.  Must be called before calling
SendRequest().
On 2010/07/27 21:10:47, vandebo wrote:
> You should document the return values here.

Done.

http://codereview.chromium.org/3079002/diff/15002/31005#newcode31
net/http/http_stream.h:31: virtual int InitializeStream(const HttpRequestInfo&
request_info,
On 2010/07/27 21:10:47, vandebo wrote:
> Now that this has an Init method, the NetLog should probably be passed in here
> instead of the individual constructors.  We sometimes don't have the NetLog we
> intend to use until we are ready to actually do something.

I'm fine with that.  To do this would require that, in the HttpBasicStream, we
delay constructing the parser until Init.  This would mean we'd need to store
the ClientSocketHandle in a member field.  Sound good to you?

In the case of SpdyHttpStream, how would you handle the BoundNetLog member
field?  Would you make it a pointer, and just point it at whatever is passed in?
 Would you make it an inline object and clone it during Init()?  Would you make
it a smart pointer and point it to a copy of the Init() argument?

(I'm new enough to C++ that I'm never quite sure what the right idiom is.)

[vandebo (ex-Chrome), 2010-07-27 22:15:11]

http://codereview.chromium.org/3079002/diff/15002/31001
File net/http/http_basic_stream.cc (right):

http://codereview.chromium.org/3079002/diff/15002/31001#newcode26
net/http/http_basic_stream.cc:26: return parser_->SendRequest(
On 2010/07/27 21:31:06, Ryan Hamilton wrote:
> On 2010/07/27 21:10:47, vandebo wrote:
> > DCHECK that Init has been called?
> 
> Since request_info_ is an inline object, I don't see an obvious way to do this
> without adding a boolean.  What do you think?  I could change it to be a
> scoped_ptr, I guess, and check for NULL?

With HttpStreamParser construction moving to Init, you have a couple options.

http://codereview.chromium.org/3079002/diff/15002/31002
File net/http/http_basic_stream.h (right):

http://codereview.chromium.org/3079002/diff/15002/31002#newcode61
net/http/http_basic_stream.h:61: HttpRequestInfo request_info_;
On 2010/07/27 21:31:06, Ryan Hamilton wrote:
> On 2010/07/27 21:10:47, vandebo wrote:
> > Instead of storing the request_info, you could initialize the
HttpStreamParser
> > in the InitializeStream method and change the HttpStreamParser to take the
> > request_info in the constructor.
> 
> I could definitely do that.  That would require that we save the
> ClientSocketHandle from the constructor so that we can use it in
> InitializeStream().  Does that strike you as better?  (We'd also have to move
> the net_log, but you mention that you'd prefer that in any case.)

Didn't notice that, but with the NetLog moving anyway, yup sounds good.

http://codereview.chromium.org/3079002/diff/15002/31005
File net/http/http_stream.h (right):

http://codereview.chromium.org/3079002/diff/15002/31005#newcode31
net/http/http_stream.h:31: virtual int InitializeStream(const HttpRequestInfo&
request_info,
On 2010/07/27 21:31:06, Ryan Hamilton wrote:
> On 2010/07/27 21:10:47, vandebo wrote:
> > Now that this has an Init method, the NetLog should probably be passed in
here
> > instead of the individual constructors.  We sometimes don't have the NetLog
we
> > intend to use until we are ready to actually do something.
> 
> I'm fine with that.  To do this would require that, in the HttpBasicStream, we
> delay constructing the parser until Init.  This would mean we'd need to store
> the ClientSocketHandle in a member field.  Sound good to you?

Yup. Though clearing it after use would be appropriate in this case.
 
> In the case of SpdyHttpStream, how would you handle the BoundNetLog member
> field?  Would you make it a pointer, and just point it at whatever is passed
in?
>  Would you make it an inline object and clone it during Init()?  Would you
make
> it a smart pointer and point it to a copy of the Init() argument?

Looks like it's only used Init, so I would remove the field.  If I'm mistaken,
then I would make it an inline object and clone to it - The NetLog classes are
light weight so that's what we usually do.

[rch (use chromium not google), 2010-07-27 23:10:35]

http://codereview.chromium.org/3079002/diff/15002/31001
File net/http/http_basic_stream.cc (right):

http://codereview.chromium.org/3079002/diff/15002/31001#newcode26
net/http/http_basic_stream.cc:26: return parser_->SendRequest(
On 2010/07/27 22:15:11, vandebo wrote:
> On 2010/07/27 21:31:06, Ryan Hamilton wrote:
> > On 2010/07/27 21:10:47, vandebo wrote:
> > > DCHECK that Init has been called?
> > 
> > Since request_info_ is an inline object, I don't see an obvious way to do
this
> > without adding a boolean.  What do you think?  I could change it to be a
> > scoped_ptr, I guess, and check for NULL?
> 
> With HttpStreamParser construction moving to Init, you have a couple options.

Done.

http://codereview.chromium.org/3079002/diff/15002/31002
File net/http/http_basic_stream.h (right):

http://codereview.chromium.org/3079002/diff/15002/31002#newcode61
net/http/http_basic_stream.h:61: HttpRequestInfo request_info_;
On 2010/07/27 22:15:11, vandebo wrote:
> On 2010/07/27 21:31:06, Ryan Hamilton wrote:
> > On 2010/07/27 21:10:47, vandebo wrote:
> > > Instead of storing the request_info, you could initialize the
> HttpStreamParser
> > > in the InitializeStream method and change the HttpStreamParser to take the
> > > request_info in the constructor.
> > 
> > I could definitely do that.  That would require that we save the
> > ClientSocketHandle from the constructor so that we can use it in
> > InitializeStream().  Does that strike you as better?  (We'd also have to
move
> > the net_log, but you mention that you'd prefer that in any case.)
> 
> Didn't notice that, but with the NetLog moving anyway, yup sounds good.

Done.

http://codereview.chromium.org/3079002/diff/15002/31005
File net/http/http_stream.h (right):

http://codereview.chromium.org/3079002/diff/15002/31005#newcode31
net/http/http_stream.h:31: virtual int InitializeStream(const HttpRequestInfo&
request_info,
On 2010/07/27 22:15:11, vandebo wrote:
> On 2010/07/27 21:31:06, Ryan Hamilton wrote:
> > On 2010/07/27 21:10:47, vandebo wrote:
> > > Now that this has an Init method, the NetLog should probably be passed in
> here
> > > instead of the individual constructors.  We sometimes don't have the
NetLog
> we
> > > intend to use until we are ready to actually do something.
> > 
> > I'm fine with that.  To do this would require that, in the HttpBasicStream,
we
> > delay constructing the parser until Init.  This would mean we'd need to
store
> > the ClientSocketHandle in a member field.  Sound good to you?
> 
> Yup. Though clearing it after use would be appropriate in this case.
>  
> > In the case of SpdyHttpStream, how would you handle the BoundNetLog member
> > field?  Would you make it a pointer, and just point it at whatever is passed
> in?
> >  Would you make it an inline object and clone it during Init()?  Would you
> make
> > it a smart pointer and point it to a copy of the Init() argument?
> 
> Looks like it's only used Init, so I would remove the field.  If I'm mistaken,
> then I would make it an inline object and clone to it - The NetLog classes are
> light weight so that's what we usually do.
> 

Done.

[rch (use chromium not google), 2010-07-27 23:13:55]

On Tue, Jul 27, 2010 at 2:10 PM, <vandebo@chromium.org> wrote:

> On 2010/07/27 20:58:24, mbelshe wrote:
>
>  On Tue, Jul 27, 2010 at 1:50 PM, <mailto:rch@google.com> wrote:
>>
>
>  >
>> > http://codereview.chromium.org/3079002/diff/15002/31005
>> > File net/http/http_stream.h (right):
>> >
>> > http://codereview.chromium.org/3079002/diff/15002/31005#newcode39
>> > net/http/http_stream.h:39: virtual int SendRequest(const std::string&
>> > request_headers,
>> > On 2010/07/27 19:40:35, Mike Belshe wrote:
>> >
>> >> I think we should remove request_headers from here.  The logic in
>> >> HTN::SendRequest to construct them should be movable into
>> >>
>> > HttpBasicStream
>> >
>> >> without too much effort.
>> >>
>> >
>> > I totally agree with your vision here.  The code which sets the headers
>> > is currently in HttpNetworkTransaction::DoSendRequest().  In order to
>> > construct them, it consults a number of member fields/methods:
>> > * ShouldApplyProxyAuth
>> > * ShouldApplyServerAuth
>> > * auth_controllers_
>> > * using_ssl
>> > * proxy_info_.is_http()
>> > * session_->network_delegate
>> > I'm guessing we don't want to pass all of this in.
>> >
>>
>
>  Although ugly, I think we do :-(  It's better than the string, no?
>>
>
>  Also, many of these may land on the request_ already?  Not sure.
>>
>
> Is there actually something to be gained from the ugliness?
>

Perhaps we could compromise.  Since DoSendRequest is pretty BasicHttp
specific, how about we leave that code as is.  The next bug I'm going to
work on will remove the SPDY specific states from HttpNetworkTransaction.
 As part of that CL, we'll probably want to clean up this path anyway, and
that would be a good context to make sure we do it the right way.

Cool?

http://codereview.chromium.org/3079002/diff/15002/31001
> File net/http/http_basic_stream.cc (right):
>
> http://codereview.chromium.org/3079002/diff/15002/31001#newcode26
> net/http/http_basic_stream.cc:26: return parser_->SendRequest(
> DCHECK that Init has been called?
>
> http://codereview.chromium.org/3079002/diff/15002/31002
> File net/http/http_basic_stream.h (right):
>
> http://codereview.chromium.org/3079002/diff/15002/31002#newcode61
> net/http/http_basic_stream.h:61: HttpRequestInfo request_info_;
> Instead of storing the request_info, you could initialize the
> HttpStreamParser in the InitializeStream method and change the
> HttpStreamParser to take the request_info in the constructor.
>
> http://codereview.chromium.org/3079002/diff/15002/31003
> File net/http/http_network_transaction.cc (right):
>
> http://codereview.chromium.org/3079002/diff/15002/31003#newcode1062
> net/http/http_network_transaction.cc:1062: return
> http_stream_->SendRequest(request_headers_,
> You haven't called Init on this stream yet.
>
> http://codereview.chromium.org/3079002/diff/15002/31004
> File net/http/http_proxy_client_socket.cc (right):
>
> http://codereview.chromium.org/3079002/diff/15002/31004#newcode327
> net/http/http_proxy_client_socket.cc:327:
> http_stream_->InitializeStream(request_, &io_callback_);
> Since this explicitly instantiats a HttpBasicStream, just pass NULL for
> the callback.
>
>
> http://codereview.chromium.org/3079002/diff/15002/31005
> File net/http/http_stream.h (right):
>
> http://codereview.chromium.org/3079002/diff/15002/31005#newcode30
> net/http/http_stream.h:30: // Initialize stream.  Must be called before
> calling SendRequest().
> You should document the return values here.
>
> http://codereview.chromium.org/3079002/diff/15002/31005#newcode31
> net/http/http_stream.h:31: virtual int InitializeStream(const
> HttpRequestInfo& request_info,
> Now that this has an Init method, the NetLog should probably be passed
> in here instead of the individual constructors.  We sometimes don't have
> the NetLog we intend to use until we are ready to actually do something.
>
>
> http://codereview.chromium.org/3079002/show
>

[Mike Belshe, 2010-07-27 23:15:09]

LGTM

[vandebo (ex-Chrome), 2010-07-27 23:59:05]

http://codereview.chromium.org/3079002/diff/34002/11004
File net/http/http_basic_stream.h (right):

http://codereview.chromium.org/3079002/diff/34002/11004#newcode16
net/http/http_basic_stream.h:16: #include "net/http/http_request_info.h"
Can change this back to a declaration.

http://codereview.chromium.org/3079002/diff/34002/11004#newcode62
net/http/http_basic_stream.h:62: ClientSocketHandle* socket_;
nit: We do have ClientSocket's as well. So it's better not to name these
socket_, either socket_handle, or connection.

http://codereview.chromium.org/3079002/diff/34002/11004#newcode64
net/http/http_basic_stream.h:64: HttpRequestInfo request_info_;
By changing HttpStreamParser, we can get rid of this copy.

http://codereview.chromium.org/3079002/diff/34002/11005
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/3079002/diff/34002/11005#newcode1062
net/http/http_network_transaction.cc:1062: // TODO(rch): while HttpBasicStream's
InitializeStream method
I'm not sure this comment is actually needed anymore...

http://codereview.chromium.org/3079002/diff/34002/11006
File net/http/http_proxy_client_socket.cc (right):

http://codereview.chromium.org/3079002/diff/34002/11006#newcode324
net/http/http_proxy_client_socket.cc:324: // TODO(rch): while HttpBasicStream's
InitializeStream method
ditto.

http://codereview.chromium.org/3079002/diff/34002/11008
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/3079002/diff/34002/11008#newcode237
net/spdy/spdy_http_stream.cc:237: int SpdyHttpStream::SendRequest(const
std::string& headers_string,
For unused parameters, we have a convention of commenting them out: const
std::string& /* headers_string */,

http://codereview.chromium.org/3079002/diff/34002/11009
File net/spdy/spdy_http_stream.h (right):

http://codereview.chromium.org/3079002/diff/34002/11009#newcode30
net/spdy/spdy_http_stream.h:30: class SpdyHttpStream : public
SpdyStream::Delegate, HttpStream {
public HttpStream

[rch (use chromium not google), 2010-07-28 00:34:05]

Will see what the trybots have to say...

http://codereview.chromium.org/3079002/diff/34002/11004
File net/http/http_basic_stream.h (right):

http://codereview.chromium.org/3079002/diff/34002/11004#newcode16
net/http/http_basic_stream.h:16: #include "net/http/http_request_info.h"
On 2010/07/27 23:59:05, vandebo wrote:
> Can change this back to a declaration.

Done.

http://codereview.chromium.org/3079002/diff/34002/11004#newcode62
net/http/http_basic_stream.h:62: ClientSocketHandle* socket_;
On 2010/07/27 23:59:05, vandebo wrote:
> nit: We do have ClientSocket's as well. So it's better not to name these
> socket_, either socket_handle, or connection.

Done.

http://codereview.chromium.org/3079002/diff/34002/11004#newcode64
net/http/http_basic_stream.h:64: HttpRequestInfo request_info_;
On 2010/07/27 23:59:05, vandebo wrote:
> By changing HttpStreamParser, we can get rid of this copy.

Done.  Whoops, meant to do this earlier.

http://codereview.chromium.org/3079002/diff/34002/11005
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/3079002/diff/34002/11005#newcode1062
net/http/http_network_transaction.cc:1062: // TODO(rch): while HttpBasicStream's
InitializeStream method
On 2010/07/27 23:59:05, vandebo wrote:
> I'm not sure this comment is actually needed anymore...

Done.

http://codereview.chromium.org/3079002/diff/34002/11006
File net/http/http_proxy_client_socket.cc (right):

http://codereview.chromium.org/3079002/diff/34002/11006#newcode324
net/http/http_proxy_client_socket.cc:324: // TODO(rch): while HttpBasicStream's
InitializeStream method
On 2010/07/27 23:59:05, vandebo wrote:
> ditto.

Done.

http://codereview.chromium.org/3079002/diff/34002/11008
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/3079002/diff/34002/11008#newcode237
net/spdy/spdy_http_stream.cc:237: int SpdyHttpStream::SendRequest(const
std::string& headers_string,
On 2010/07/27 23:59:05, vandebo wrote:
> For unused parameters, we have a convention of commenting them out: const
> std::string& /* headers_string */,

Done.  Love it!

[vandebo (ex-Chrome), 2010-07-28 00:59:05]

http://codereview.chromium.org/3079002/diff/31013/12
File net/http/http_basic_stream.cc (right):

http://codereview.chromium.org/3079002/diff/31013/12#newcode17
net/http/http_basic_stream.cc:17: parser_.reset(new
HttpStreamParser(connection_, &request_info,
Changes to HttpStreamParser missing?

http://codereview.chromium.org/3079002/diff/31013/14
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/3079002/diff/31013/14#newcode1057
net/http/http_network_transaction.cc:1057: return
http_stream_->SendRequest(request_headers_,
nit: an argument or two more fill fit up here.

http://codereview.chromium.org/3079002/diff/31013/14#newcode1324
net/http/http_network_transaction.cc:1324: return
spdy_http_stream_->SendRequest(request_headers_,
ditto

http://codereview.chromium.org/3079002/diff/31013/16
File net/http/http_stream.h (right):

http://codereview.chromium.org/3079002/diff/31013/16#newcode34
net/http/http_stream.h:34: virtual int InitializeStream(const HttpRequestInfo&
request_info,
HttpBasicStream changes this, so it should be HttpRequestInfo* through and
through.

http://codereview.chromium.org/3079002/diff/31013/17
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/3079002/diff/31013/17#newcode153
net/spdy/spdy_http_stream.cc:153: int error =
spdy_session_->GetPushStream(request_info_.url, &stream_,
extra _ ?

[rch (use chromium not google), 2010-07-28 03:55:36]

Once more into the breach...  I ran the previous version through the trybots and
got failures on Windows and Linux, but success on Mac.  Mebe it was transient,
I'll go dig.

http://codereview.chromium.org/3079002/diff/31013/12
File net/http/http_basic_stream.cc (right):

http://codereview.chromium.org/3079002/diff/31013/12#newcode17
net/http/http_basic_stream.cc:17: parser_.reset(new
HttpStreamParser(connection_, &request_info,
On 2010/07/28 00:59:05, vandebo wrote:
> Changes to HttpStreamParser missing?

Done.  Argh, I didn't realize I needed to run gcl change again.

http://codereview.chromium.org/3079002/diff/31013/14
File net/http/http_network_transaction.cc (right):

http://codereview.chromium.org/3079002/diff/31013/14#newcode1057
net/http/http_network_transaction.cc:1057: return
http_stream_->SendRequest(request_headers_,
On 2010/07/28 00:59:05, vandebo wrote:
> nit: an argument or two more fill fit up here.

Done.

http://codereview.chromium.org/3079002/diff/31013/14#newcode1324
net/http/http_network_transaction.cc:1324: return
spdy_http_stream_->SendRequest(request_headers_,
On 2010/07/28 00:59:05, vandebo wrote:
> ditto

Done.

http://codereview.chromium.org/3079002/diff/31013/16
File net/http/http_stream.h (right):

http://codereview.chromium.org/3079002/diff/31013/16#newcode34
net/http/http_stream.h:34: virtual int InitializeStream(const HttpRequestInfo&
request_info,
On 2010/07/28 00:59:05, vandebo wrote:
> HttpBasicStream changes this, so it should be HttpRequestInfo* through and
> through.

Done.

http://codereview.chromium.org/3079002/diff/31013/17
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/3079002/diff/31013/17#newcode153
net/spdy/spdy_http_stream.cc:153: int error =
spdy_session_->GetPushStream(request_info_.url, &stream_,
On 2010/07/28 00:59:05, vandebo wrote:
> extra _ ?

Done.

http://codereview.chromium.org/3079002/diff/31013/17#newcode153
net/spdy/spdy_http_stream.cc:153: int error =
spdy_session_->GetPushStream(request_info_.url, &stream_,
On 2010/07/28 00:59:05, vandebo wrote:
> extra _ ?

Done.

[vandebo (ex-Chrome), 2010-07-28 23:30:39]

LGTM

http://codereview.chromium.org/3079002/diff/4005/5014
File net/http/http_basic_stream.h (right):

http://codereview.chromium.org/3079002/diff/4005/5014#newcode36
net/http/http_basic_stream.h:36: CompletionCallback* callback);
nit: callback is unused

http://codereview.chromium.org/3079002/diff/4005/5020
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/3079002/diff/4005/5020#newcode237
net/spdy/spdy_http_stream.cc:237: int SpdyHttpStream::SendRequest(const
std::string& /*headers_string*/,
My mistake, it's in the headers that we comment it out.

http://codereview.chromium.org/3079002/diff/4005/5021
File net/spdy/spdy_http_stream.h (right):

http://codereview.chromium.org/3079002/diff/4005/5021#newcode51
net/spdy/spdy_http_stream.h:51: virtual int SendRequest(const std::string&
headers,
Here.