Add chunked uploads support to SPDY

Add chunked uploads support to SPDY

As part of this, I had to move the chunked encoding part from UploadData::Element::SetChunk
to HttpStreamParser::DoSendBody as SPDY doesn't have this encoded format and UploadData
needs to serve both.

BUG=none
TEST=net_unittests (2 new tests added)

Committed and rolled back: http://src.chromium.org/viewvc/chrome?view=rev&revision=76892

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

[Satish, 2011-01-26 22:36:33]

http://codereview.chromium.org/6292013/diff/1/net/base/upload_data_stream.cc
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/6292013/diff/1/net/base/upload_data_stream.cc#...
net/base/upload_data_stream.cc:72: if (bytes_copied)
This check is required because accessing "d[0]" below when d is empty will throw
an exception

http://codereview.chromium.org/6292013/diff/1/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/6292013/diff/1/net/http/http_stream_parser.cc#...
net/http/http_stream_parser.cc:276: chunk_data_length_ : result);
We use "chunk_data_length_" here because UploadDataStream only has the data and
not the chunk header+footer. For e.g. If the chunk data was 100 bytes long we'll
be sending 106 bytes (2 byte len, 2 crlfs, data) over the wire and result will
be 106 here.. but we should be consuming only 100 bytes from UploadDataStream

[willchan no longer on Chromium, 2011-01-27 18:47:48]

My instinct here is that UploadDataStream isn't designed correctly for our
needs. I think the interface should be changed.

http://codereview.chromium.org/6292013/diff/1/net/base/upload_data_stream.cc
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/6292013/diff/1/net/base/upload_data_stream.cc#...
net/base/upload_data_stream.cc:72: if (bytes_copied)
On 2011/01/26 22:36:33, Satish wrote:
> This check is required because accessing "d[0]" below when d is empty will
throw
> an exception

I wonder if it'd be simpler to do:
if (!bytes_copied)
  break;

Not sure what you think here.

http://codereview.chromium.org/6292013/diff/1/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/6292013/diff/1/net/http/http_stream_parser.cc#...
net/http/http_stream_parser.cc:276: chunk_data_length_ : result);
On 2011/01/26 22:36:33, Satish wrote:
> We use "chunk_data_length_" here because UploadDataStream only has the data
and
> not the chunk header+footer. For e.g. If the chunk data was 100 bytes long
we'll
> be sending 106 bytes (2 byte len, 2 crlfs, data) over the wire and result will
> be 106 here.. but we should be consuming only 100 bytes from UploadDataStream

This is buggy. ClientSocket::Write() may not write all bytes. It might only
partially write.

I think the correct way to do this is to only call MarkConsumedAndFillBuffer()
when you've successfully written the full chunk.

Going further, it seems to be that we should stop calling this
MarkConsumedAndFillBuffer() and redo the functionality. Perhaps it'd be better
to have something like int int GetNextBuffer(scoped_refptr<IOBufferWithSize>*
buf, CompletionCallback* callback);

Then it'd return ERR_IO_PENDING if the buffer is not yet available. Otherwise,
it'd return the number of bytes. 0 should mean EOF.

http://codereview.chromium.org/6292013/diff/1/net/spdy/spdy_http_stream.cc
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/6292013/diff/1/net/spdy/spdy_http_stream.cc#ne...
net/spdy/spdy_http_stream.cc:271: bool SpdyHttpStream::OnSendBodyComplete(int*
status) {
You're reusing the same parameter for input and output. You probably shouldn't
be doing this. Why don't you just change the return value?

[Satish, 2011-01-27 20:42:09]

http://codereview.chromium.org/6292013/diff/1/net/base/upload_data_stream.cc
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/6292013/diff/1/net/base/upload_data_stream.cc#...
net/base/upload_data_stream.cc:72: if (bytes_copied)
On 2011/01/27 18:47:49, willchan wrote:
> On 2011/01/26 22:36:33, Satish wrote:
> > This check is required because accessing "d[0]" below when d is empty will
> throw
> > an exception
> 
> I wonder if it'd be simpler to do:
> if (!bytes_copied)
>   break;
> 
> Not sure what you think here.

Sure could do that.

http://codereview.chromium.org/6292013/diff/1/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/6292013/diff/1/net/http/http_stream_parser.cc#...
net/http/http_stream_parser.cc:276: chunk_data_length_ : result);
> This is buggy. ClientSocket::Write() may not write all bytes. It might only
> partially write.
> 
> I think the correct way to do this is to only call MarkConsumedAndFillBuffer()
> when you've successfully written the full chunk.

Ok. I can stash the number of bytes given to Write() in a member var, decrement
it by the value received by OnIOComplete() and call DoLoop() only when it
reaches zero. Is that ok or do you have other suggestions?

> Going further, it seems to be that we should stop calling this
> MarkConsumedAndFillBuffer() and redo the functionality. Perhaps it'd be better
> to have something like int int GetNextBuffer(scoped_refptr<IOBufferWithSize>*
> buf, CompletionCallback* callback);
> 
> Then it'd return ERR_IO_PENDING if the buffer is not yet available. Otherwise,
> it'd return the number of bytes. 0 should mean EOF.

Sounds good. This seems like a general change in UploadDataStream and not
related to SPDY. Can I do it in a subsequent CL? This also requires that
UploadDataStream will now implement ChunkCallback and receive the call from
UploadData, and in turn invoke the CompletionCallback you mention above. That
would be a self contained change on its own.

http://codereview.chromium.org/6292013/diff/1/net/spdy/spdy_http_stream.cc
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/6292013/diff/1/net/spdy/spdy_http_stream.cc#ne...
net/spdy/spdy_http_stream.cc:271: bool SpdyHttpStream::OnSendBodyComplete(int*
status) {
On 2011/01/27 18:47:49, willchan wrote:
> You're reusing the same parameter for input and output. You probably shouldn't
> be doing this. Why don't you just change the return value?

The function needs the status code as input and return 2 values - network status
code as well as EOF flag. So we either need 1 IN parameter (status), 1 OUT
parameter (eof) and a return value (write status), or 1 IN/OUT parameter and a
return value as I have done here. I'm fine either way, so I can change it to the
first if you prefer so.

[willchan no longer on Chromium, 2011-01-28 00:56:17]

http://codereview.chromium.org/6292013/diff/1/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/6292013/diff/1/net/http/http_stream_parser.cc#...
net/http/http_stream_parser.cc:276: chunk_data_length_ : result);
On 2011/01/27 20:42:09, Satish wrote:
> > This is buggy. ClientSocket::Write() may not write all bytes. It might only
> > partially write.
> > 
> > I think the correct way to do this is to only call
MarkConsumedAndFillBuffer()
> > when you've successfully written the full chunk.
> 
> Ok. I can stash the number of bytes given to Write() in a member var,
decrement
> it by the value received by OnIOComplete() and call DoLoop() only when it
> reaches zero. Is that ok or do you have other suggestions?

I'm not sure I follow. Write() with 10 bytes may have a return value of 3. In
order to complete it, you need to call Write() again, with the buffer index
incremented by 3, and the number of bytes to write changed to 7. When
OnIOComplete(result) is called, there's no way to know which state you're in. It
could be a Read() that is completing. So you have to call DoLoop() to enter the
state machine so you know that it's the return value for Write().

I think you need to expand the number of states in the state machine. And the
state machine will be easier to understand if you implement my suggestion below
wrt UploadDataStream::MarkConsumedAndFillBuffer() vs
UploadDataStream::GetNextBuffer().

I think you need states like.
STATE_GET_BODY,
STATE_GET_BODY_COMPLETE,
STATE_SEND_BODY,
STATE_SEND_BODY_COMPLETE

STATE_GET_BODY calls request_body_->GetNextBuffer(). Pass the return value to
STATE_GET_BODY_COMPLETE
In STATE_GET_BODY_COMPLETE, if the result is positive, go straight to
STATE_SEND_BODY. If the result is negative, it must be ERR_IO_PENDING, mark the
next state as STATE_SEND_BODY, and then bail out of the state machine. We'll
continue when it comes back.
In STATE_SEND_BODY_COMPLETE, if we're sent the whole chunk, go to
STATE_GET_BODY. Otherwise, go to back to STATE_SEND_BODY.

> 
> > Going further, it seems to be that we should stop calling this
> > MarkConsumedAndFillBuffer() and redo the functionality. Perhaps it'd be
better
> > to have something like int int
GetNextBuffer(scoped_refptr<IOBufferWithSize>*
> > buf, CompletionCallback* callback);
> > 
> > Then it'd return ERR_IO_PENDING if the buffer is not yet available.
Otherwise,
> > it'd return the number of bytes. 0 should mean EOF.
> 
> Sounds good. This seems like a general change in UploadDataStream and not
> related to SPDY. Can I do it in a subsequent CL? This also requires that
> UploadDataStream will now implement ChunkCallback and receive the call from
> UploadData, and in turn invoke the CompletionCallback you mention above. That
> would be a self contained change on its own.

http://codereview.chromium.org/6292013/diff/1/net/spdy/spdy_http_stream.cc
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/6292013/diff/1/net/spdy/spdy_http_stream.cc#ne...
net/spdy/spdy_http_stream.cc:271: bool SpdyHttpStream::OnSendBodyComplete(int*
status) {
On 2011/01/27 20:42:09, Satish wrote:
> On 2011/01/27 18:47:49, willchan wrote:
> > You're reusing the same parameter for input and output. You probably
shouldn't
> > be doing this. Why don't you just change the return value?
> 
> The function needs the status code as input and return 2 values - network
status
> code as well as EOF flag. So we either need 1 IN parameter (status), 1 OUT
> parameter (eof) and a return value (write status), or 1 IN/OUT parameter and a
> return value as I have done here. I'm fine either way, so I can change it to
the
> first if you prefer so.

OK, I see what's going on here. You should probably be changing
SpdyStream::DoSendBodyComplete() to go back to DoSendBody() if there is still
more data to be written. You should not be writting data here in
SpdyHttpStream::OnSendBodyComplete().

[Satish, 2011-01-28 17:37:51]

http://codereview.chromium.org/6292013/diff/7002/net/base/upload_data_stream.cc
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/6292013/diff/7002/net/base/upload_data_stream....
net/base/upload_data_stream.cc:72: if (bytes_copied)
I left this as is because we need to set 'advance_to_next_element' to true below
and doing a break here means we have to duplicate that here as well.

[Satish, 2011-01-28 17:41:54]

New patch uploaded, please take a look.

Changing UploadDataStream to implement a GetNextBuffer() style interface  is a
more sweeping change and requires modifications to http stream, spdy stream as
well as chrome-frame which has its own variation of UploadDataStream. I think it
will be cleaner to implement that change in a separate CL after this change goes
in.

Because of the way the latest patch is implemented, a big chunk of this will
stay the same even with the above suggested new interface.

[willchan no longer on Chromium, 2011-02-01 23:35:59]

http://codereview.chromium.org/6292013/diff/7002/net/base/upload_data_stream.cc
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/6292013/diff/7002/net/base/upload_data_stream....
net/base/upload_data_stream.cc:72: if (bytes_copied)
On 2011/01/28 17:37:51, Satish wrote:
> I left this as is because we need to set 'advance_to_next_element' to true
below
> and doing a break here means we have to duplicate that here as well.

Well, maybe buf_len_ += bytes_copied should be included in the if condition
then.

http://codereview.chromium.org/6292013/diff/7002/net/base/upload_data_stream.h
File net/base/upload_data_stream.h (right):

http://codereview.chromium.org/6292013/diff/7002/net/base/upload_data_stream....
net/base/upload_data_stream.h:29: size_t GetMaxBufferSize() const { return
kBufSize; }
It's annoying we have to expose this. We should just be returning a
IOBufferWithSize for the consumer to take. Or write to a buffer that they
provide.

Please leave a TODO to fix this, so people will know not to use this.

http://codereview.chromium.org/6292013/diff/7002/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/6292013/diff/7002/net/http/http_stream_parser....
net/http/http_stream_parser.cc:276: int HttpStreamParser::DoSendBody(int result)
{
Please add a bunch of assertions to validate this function works correctly. I'll
give some examples below.

I also feel like we have a lot of unnecessary
request_body_->MarkConsumedAndFillBuffer(0) calls. A stricter UploadDataStream
implementation should probably disallow this. Is there a way to rework this code
so we don't have to call it with 0?

http://codereview.chromium.org/6292013/diff/7002/net/http/http_stream_parser....
net/http/http_stream_parser.cc:280: memcpy(chunk_buf_->data(),
chunk_buf_->data() + result, chunk_length_);
You need to use memmove instead of memcpy because you have overlapping memory
regions.

Rather than having to do copies, how about just incrementing an index pointer to
keep track of where you are in the buffer?

http://codereview.chromium.org/6292013/diff/7002/net/http/http_stream_parser....
net/http/http_stream_parser.cc:300: // Encode and send the buffer as 1 chunk.
Why don't you use a single sprintf() call? Something like:
sprintf("%X\r\n%*s\r\n", buf_len, buf_len, request_body_->buf()->data())? It's
annoying to have to read the destination pointers each time to make sure you
have the indices calculated correctly.

http://codereview.chromium.org/6292013/diff/7002/net/spdy/spdy_http_stream.cc
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/6292013/diff/7002/net/spdy/spdy_http_stream.cc...
net/spdy/spdy_http_stream.cc:273: return
stream_->WriteStreamData(request_body_stream_->buf(), buf_len,
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
You need to align your parameters on the second line.

http://codereview.chromium.org/6292013/diff/7002/net/spdy/spdy_http_stream.cc...
net/spdy/spdy_http_stream.cc:284:
request_body_stream_->MarkConsumedAndFillBuffer(*status);
I'm confused how this works. OK == 0. You set *status to OK a few lines before.
How does this ever make progress?

At this point, we've gone through enough of the design. You need to add tests to
show this stuff works.

http://codereview.chromium.org/6292013/diff/7002/net/spdy/spdy_proxy_client_s...
File net/spdy/spdy_proxy_client_socket.h (right):

http://codereview.chromium.org/6292013/diff/7002/net/spdy/spdy_proxy_client_s...
net/spdy/spdy_proxy_client_socket.h:98: virtual void
set_chunk_callback(ChunkCallback* callback) {}
If you're ignoring the parameter, please document it properly. See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
which says:
"""
// Comment out unused named parameters in definitions.
void Circle::Rotate(double /*radians*/) {}
"""

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

http://codereview.chromium.org/6292013/diff/7002/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:635: DCHECK_GE(result,
static_cast<int>(spdy::SpdyFrame::size()));
Why is it a DCHECK_GE now?

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

http://codereview.chromium.org/6292013/diff/7002/net/spdy/spdy_stream.h#newco...
net/spdy/spdy_stream.h:58: virtual bool OnSendBodyComplete(int* status) = 0;
Change this to be more consistent with the rest of the network stack code. If it
needs to return a net error code, return it using the return value. Use a bool*
output parameter for the eof status. Please update the comments too.

http://codereview.chromium.org/6292013/diff/7002/net/spdy/spdy_stream.h#newco...
net/spdy/spdy_stream.h:79: virtual void set_chunk_callback(ChunkCallback*
callback) = 0;
Please comment this.

[Satish, 2011-02-22 14:25:43]

Latest patch uploaded, I will add tests once the current questions get cleared
below. Please excuse this late reply as I was away for the past couple of weeks.

http://codereview.chromium.org/6292013/diff/7002/net/base/upload_data_stream.cc
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/6292013/diff/7002/net/base/upload_data_stream....
net/base/upload_data_stream.cc:72: if (bytes_copied)
On 2011/02/01 23:35:59, willchan wrote:
> On 2011/01/28 17:37:51, Satish wrote:
> > I left this as is because we need to set 'advance_to_next_element' to true
> below
> > and doing a break here means we have to duplicate that here as well.
> 
> Well, maybe buf_len_ += bytes_copied should be included in the if condition
> then.

Done.

http://codereview.chromium.org/6292013/diff/7002/net/base/upload_data_stream.h
File net/base/upload_data_stream.h (right):

http://codereview.chromium.org/6292013/diff/7002/net/base/upload_data_stream....
net/base/upload_data_stream.h:29: size_t GetMaxBufferSize() const { return
kBufSize; }
> It's annoying we have to expose this. We should just be returning a
> IOBufferWithSize for the consumer to take. Or write to a buffer that they
> provide.
> 
> Please leave a TODO to fix this, so people will know not to use this.

We don't write to a buffer provided by the caller, but the caller indeed copies
the data to their own IOBuffer. To do that the caller needs to know how large
the IOBuffer should be allocated for, hence this method. The alternative is to
pass this constant as a parameter at creation time which looks uglier because
the consumer and creator of this object are different classes. Since this is
primarily used to avoid unwanted allocations and just use the same IOBuffer in
http_stream_parser, I'm not sure why you feel this is ugly. A caller who sends
the IOBuffer as a whole doesn't care about this method at all.

http://codereview.chromium.org/6292013/diff/7002/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/6292013/diff/7002/net/http/http_stream_parser....
net/http/http_stream_parser.cc:276: int HttpStreamParser::DoSendBody(int result)
{
> I also feel like we have a lot of unnecessary
> request_body_->MarkConsumedAndFillBuffer(0) calls. A stricter UploadDataStream
> implementation should probably disallow this. Is there a way to rework this
code
> so we don't have to call it with 0?

In the newly added code MarkConsumedAndFillBuffer() is never called with zero.
Can you clarify which code flow you thought results in such calls?

http://codereview.chromium.org/6292013/diff/7002/net/http/http_stream_parser....
net/http/http_stream_parser.cc:280: memcpy(chunk_buf_->data(),
chunk_buf_->data() + result, chunk_length_);
On 2011/02/01 23:35:59, willchan wrote:
> You need to use memmove instead of memcpy because you have overlapping memory
> regions.

Done

> Rather than having to do copies, how about just incrementing an index pointer
to
> keep track of where you are in the buffer?

The net::Socket::Write call in the next line only takes an IOBuffer and writes
the whole buffer out, so there is no easy way to use an index here.. unless I
change that to use a DrainableIOBuffer or something similar which is a larger
change.

http://codereview.chromium.org/6292013/diff/7002/net/http/http_stream_parser....
net/http/http_stream_parser.cc:300: // Encode and send the buffer as 1 chunk.
On 2011/02/01 23:35:59, willchan wrote:
> Why don't you use a single sprintf() call? Something like:
> sprintf("%X\r\n%*s\r\n", buf_len, buf_len, request_body_->buf()->data())? It's
> annoying to have to read the destination pointers each time to make sure you
> have the indices calculated correctly.

MSDN says that for %s 'characters are printed up to the first null character or
until the precision value is reached' - so if there was a zero byte in the
buffer only so many characters will be printed. Hence I'm doing the memcpy
myself

http://codereview.chromium.org/6292013/diff/7002/net/spdy/spdy_http_stream.cc
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/6292013/diff/7002/net/spdy/spdy_http_stream.cc...
net/spdy/spdy_http_stream.cc:273: return
stream_->WriteStreamData(request_body_stream_->buf(), buf_len,
On 2011/02/01 23:35:59, willchan wrote:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
> You need to align your parameters on the second line.

Done.

http://codereview.chromium.org/6292013/diff/7002/net/spdy/spdy_http_stream.cc...
net/spdy/spdy_http_stream.cc:284:
request_body_stream_->MarkConsumedAndFillBuffer(*status);
On 2011/02/01 23:35:59, willchan wrote:
> I'm confused how this works. OK == 0. You set *status to OK a few lines
before.
> How does this ever make progress?
> 
> At this point, we've gone through enough of the design. You need to add tests
to
> show this stuff works.

This was a last minute copy/paste error, fixed now.

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

http://codereview.chromium.org/6292013/diff/7002/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:635: DCHECK_GE(result,
static_cast<int>(spdy::SpdyFrame::size()));
On 2011/02/01 23:35:59, willchan wrote:
> Why is it a DCHECK_GE now?

Per the SPDY spec the last chunk is indicated with a SPDY frame with no data, so
'result' here would be teh same as SpdyFrame::size(). Hence changing GT to GE

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

http://codereview.chromium.org/6292013/diff/7002/net/spdy/spdy_stream.h#newco...
net/spdy/spdy_stream.h:58: virtual bool OnSendBodyComplete(int* status) = 0;
On 2011/02/01 23:35:59, willchan wrote:
> Change this to be more consistent with the rest of the network stack code. If
it
> needs to return a net error code, return it using the return value. Use a
bool*
> output parameter for the eof status. Please update the comments too.

Done.

[willchan no longer on Chromium, 2011-02-24 19:03:39]

Still looking, but wanted to send comments asap since I'm already way behind.
Sorry for the delay, right now's pretty busy.

http://codereview.chromium.org/6292013/diff/7002/net/base/upload_data_stream.h
File net/base/upload_data_stream.h (right):

http://codereview.chromium.org/6292013/diff/7002/net/base/upload_data_stream....
net/base/upload_data_stream.h:29: size_t GetMaxBufferSize() const { return
kBufSize; }
On 2011/02/22 14:25:44, Satish wrote:
> > It's annoying we have to expose this. We should just be returning a
> > IOBufferWithSize for the consumer to take. Or write to a buffer that they
> > provide.
> > 
> > Please leave a TODO to fix this, so people will know not to use this.
> 
> We don't write to a buffer provided by the caller, but the caller indeed
copies
> the data to their own IOBuffer. To do that the caller needs to know how large
> the IOBuffer should be allocated for, hence this method. The alternative is to
> pass this constant as a parameter at creation time which looks uglier because
> the consumer and creator of this object are different classes. Since this is
> primarily used to avoid unwanted allocations and just use the same IOBuffer in
> http_stream_parser, I'm not sure why you feel this is ugly. A caller who sends
> the IOBuffer as a whole doesn't care about this method at all.

I was thinking that instead of an interface like MarkConsumedAndFillBuffer(),
we'd have something like a int Read(num_bytes, buf, callback) member function.
The user provides a IOBuffer with the appropriate max size specified by
"num_bytes". The return value indicates the size that was populated by
UploadDataStream.

A MarkConsumedAndFillBuffer() member function dictates that UploadDataStream
owns the IOBuffer, which is just lent out to the caller to read temporarily.
This means that when MarkConsumedAndFillBuffer() is called, UploadDataStream has
to do a memmove. Ew, why?

Users should provide their own buffer. They can either use new buffers and keep
calling Read() to populate the new buffers, or they can finishing using their
first buffer and then pass that same buffer to Read(). Why does UploadDataStream
dictate the Read() size? Shouldn't callers be in control of how much data they
want to read at a time?

http://codereview.chromium.org/6292013/diff/7002/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/6292013/diff/7002/net/http/http_stream_parser....
net/http/http_stream_parser.cc:276: int HttpStreamParser::DoSendBody(int result)
{
On 2011/02/22 14:25:44, Satish wrote:
> > I also feel like we have a lot of unnecessary
> > request_body_->MarkConsumedAndFillBuffer(0) calls. A stricter
UploadDataStream
> > implementation should probably disallow this. Is there a way to rework this
> code
> > so we don't have to call it with 0?
> 
> In the newly added code MarkConsumedAndFillBuffer() is never called with zero.
> Can you clarify which code flow you thought results in such calls?

I don't recall the control flow =/ I looked at don't see it, so maybe I was just
wrong before? I hope I didn't miss it on the re-read.

http://codereview.chromium.org/6292013/diff/7002/net/http/http_stream_parser....
net/http/http_stream_parser.cc:280: memcpy(chunk_buf_->data(),
chunk_buf_->data() + result, chunk_length_);
On 2011/02/22 14:25:44, Satish wrote:
> On 2011/02/01 23:35:59, willchan wrote:
> > You need to use memmove instead of memcpy because you have overlapping
memory
> > regions.
> 
> Done
> 
> > Rather than having to do copies, how about just incrementing an index
pointer
> to
> > keep track of where you are in the buffer?
> 
> The net::Socket::Write call in the next line only takes an IOBuffer and writes
> the whole buffer out, so there is no easy way to use an index here.. unless I
> change that to use a DrainableIOBuffer or something similar which is a larger
> change.

DrainableIOBuffer is the right solution here. Of course, UploadDataStream is
creating the IOBuffer, rather than using a user-provided buffer. UGH.

http://codereview.chromium.org/6292013/diff/7002/net/http/http_stream_parser....
net/http/http_stream_parser.cc:300: // Encode and send the buffer as 1 chunk.
On 2011/02/22 14:25:44, Satish wrote:
> On 2011/02/01 23:35:59, willchan wrote:
> > Why don't you use a single sprintf() call? Something like:
> > sprintf("%X\r\n%*s\r\n", buf_len, buf_len, request_body_->buf()->data())?
It's
> > annoying to have to read the destination pointers each time to make sure you
> > have the indices calculated correctly.
> 
> MSDN says that for %s 'characters are printed up to the first null character
or
> until the precision value is reached' - so if there was a zero byte in the
> buffer only so many characters will be printed. Hence I'm doing the memcpy
> myself

Doh, I forgot about the NUL. You're absolutely correct.

My style preference would be for you to chunk_ptr += length, after each memcpy,
but I don't feel strongly. What you have is ok too.

[willchan no longer on Chromium, 2011-02-24 19:41:34]

http://codereview.chromium.org/6292013/diff/20001/net/base/upload_data_stream.cc
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/6292013/diff/20001/net/base/upload_data_stream...
net/base/upload_data_stream.cc:72: if (bytes_copied) {
Please add a comment for why we need this condition. I re-read the earlier
review comments and they make sense, but this code is not immediately obvious
that it's because d may be empty.

http://codereview.chromium.org/6292013/diff/20001/net/http/http_stream_parser.cc
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/6292013/diff/20001/net/http/http_stream_parser...
net/http/http_stream_parser.cc:281: memmove(chunk_buf_->data(),
chunk_buf_->data() + result, chunk_length_);
Please add a regression test here, since last time it was memcpy() which was
wrong.

http://codereview.chromium.org/6292013/diff/20001/net/http/http_stream_parser...
net/http/http_stream_parser.cc:297: chunk_length_ = 5;
Please do something like:
static const char kLastChunk[] = "0\r\n\r\n";

Then you can do chunk_length_ = strlen(kLastChunk) (note: compiler will reduce
this to a compile-time constant) and memcpy with strlen(kLastChunk) too. I
prefer this to magic constants like 5.

http://codereview.chromium.org/6292013/diff/20001/net/http/http_stream_parser.h
File net/http/http_stream_parser.h (right):

http://codereview.chromium.org/6292013/diff/20001/net/http/http_stream_parser...
net/http/http_stream_parser.h:192: // Stores an encoded chunk for chunked
uploads.
Please add a note that this should be improved not to rely on copies.

http://codereview.chromium.org/6292013/diff/20001/net/spdy/spdy_http_stream.cc
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/6292013/diff/20001/net/spdy/spdy_http_stream.c...
net/spdy/spdy_http_stream.cc:283: return OK;
You should probably add a
if (status != 0) {
  NOTREACHED();
  return ERR_UNEXPECTED;
}

http://codereview.chromium.org/6292013/diff/20001/net/spdy/spdy_http_stream.c...
net/spdy/spdy_http_stream.cc:287: *eof = false;
Er, does this work for the non-chunked case? When will *eof = true be set in the
non-chunked case? Do we have a test covering this? If not, please add one.

[Satish, 2011-03-02 14:42:23]

I have addressed all the review comments and added a couple of unit tests for
the new code. Please take a look at the latest patch.

[willchan no longer on Chromium, 2011-03-02 18:44:40]

http://codereview.chromium.org/6292013/diff/20001/net/spdy/spdy_http_stream.cc
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/6292013/diff/20001/net/spdy/spdy_http_stream.c...
net/spdy/spdy_http_stream.cc:287: *eof = false;
On 2011/02/24 19:41:34, willchan wrote:
> Er, does this work for the non-chunked case? When will *eof = true be set in
the
> non-chunked case? Do we have a test covering this? If not, please add one.

Was there a test? Or did you add one?

http://codereview.chromium.org/6292013/diff/28001/net/base/upload_data_stream.h
File net/base/upload_data_stream.h (right):

http://codereview.chromium.org/6292013/diff/28001/net/base/upload_data_stream...
net/base/upload_data_stream.h:30: // TODO: We should ideally have
UploadDataStream expose a Read() method which
Put a username on this TODO, either yours or mine. Doesn't mean you have to fix
it, it means someone can ask you for an explanation of what's going on here. I
have plenty of TODO(willchan)s that I probably won't be getting around to.

http://codereview.chromium.org/6292013/diff/28001/net/spdy/spdy_framer.cc
File net/spdy/spdy_framer.cc (right):

http://codereview.chromium.org/6292013/diff/28001/net/spdy/spdy_framer.cc#new...
net/spdy/spdy_framer.cc:796: } else if (current_data_frame.flags() &
DATA_FLAG_FIN) {
Someone else will have to verify this change is ok, because I'm not super
familiar with spdy_framer.cc.

http://codereview.chromium.org/6292013/diff/28001/net/spdy/spdy_http_stream_u...
File net/spdy/spdy_http_stream_unittest.cc (right):

http://codereview.chromium.org/6292013/diff/28001/net/spdy/spdy_http_stream_u...
net/spdy/spdy_http_stream_unittest.cc:125: host_port_pair));
host_port_pair must align with reads a la
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
which says:
"If the arguments do not all fit on one line, they should be broken up onto
multiple lines, with each subsequent line aligned with the first argument. Do
not add spaces after the open paren or before the close paren:"

reads is the first argument in the InitSession() function call, so parameters on
new lines must align with it.

http://codereview.chromium.org/6292013/diff/28001/net/spdy/spdy_http_stream_u...
net/spdy/spdy_http_stream_unittest.cc:138: scoped_ptr<SpdyHttpStream>
http_stream(
How about just instantiating this on the stack? Is there any reason to use a
scoped_ptr?

http://codereview.chromium.org/6292013/diff/28001/net/spdy/spdy_http_stream_u...
net/spdy/spdy_http_stream_unittest.cc:159:
EXPECT_TRUE(!http_session_->spdy_session_pool()->HasSession(pair));
This should be EXPECT_FALSE instead of EXPECT_TRUE(!

[Satish, 2011-03-03 13:50:09]

I have uploaded a new patch with the following major changes:

- SPDY doesn't require a zero-length data frame to mark the end of chunked
stream, instead the last valid data frame should have the FIN flag set. This
requires the application layer to indicate the last chunk when giving chunks to
send, so I have changed the URLFetcher/URLRequest/UploadData/UploadDataStream
interfaces by adding a is_last_chunk flag to AppendChunk.

- Due to the above change, spdy_http_stream.cc changes are now much simpler.
However SpdyHttpStream::OnSendBody() needs to know when the last chunk is being
sent out and UploadDataStream::eof() only returns true AFTER the data was sent
out. So I have added a new method UploadDataStream::peek_end_of_chunks() to
check if the stream is chunked and if so has the last chunk been read yet (not
sent, just read and returned). Using this the SpdyHttpStream::OnSendBody()
function now decides when to set the FIN flag in the SPDY frame.

- Changes to spdy_framer.cc have been rolled back

http://codereview.chromium.org/6292013/diff/20001/net/spdy/spdy_http_stream.cc
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/6292013/diff/20001/net/spdy/spdy_http_stream.c...
net/spdy/spdy_http_stream.cc:287: *eof = false;
On 2011/03/02 18:44:40, willchan wrote:
> On 2011/02/24 19:41:34, willchan wrote:
> > Er, does this work for the non-chunked case? When will *eof = true be set in
> the
> > non-chunked case? Do we have a test covering this? If not, please add one.
> 
> Was there a test? Or did you add one?

I added 2 unit tests and they cover this code path as well. This code has also
been greatly simplified in the latest patch.

http://codereview.chromium.org/6292013/diff/28001/net/base/upload_data_stream.h
File net/base/upload_data_stream.h (right):

http://codereview.chromium.org/6292013/diff/28001/net/base/upload_data_stream...
net/base/upload_data_stream.h:30: // TODO: We should ideally have
UploadDataStream expose a Read() method which
On 2011/03/02 18:44:40, willchan wrote:
> Put a username on this TODO, either yours or mine. Doesn't mean you have to
fix
> it, it means someone can ask you for an explanation of what's going on here. I
> have plenty of TODO(willchan)s that I probably won't be getting around to.

Done.

http://codereview.chromium.org/6292013/diff/28001/net/spdy/spdy_framer.cc
File net/spdy/spdy_framer.cc (right):

http://codereview.chromium.org/6292013/diff/28001/net/spdy/spdy_framer.cc#new...
net/spdy/spdy_framer.cc:796: } else if (current_data_frame.flags() &
DATA_FLAG_FIN) {
On 2011/03/02 18:44:40, willchan wrote:
> Someone else will have to verify this change is ok, because I'm not super
> familiar with spdy_framer.cc.

I reverted this change as mentioned above.

http://codereview.chromium.org/6292013/diff/28001/net/spdy/spdy_http_stream_u...
File net/spdy/spdy_http_stream_unittest.cc (right):

http://codereview.chromium.org/6292013/diff/28001/net/spdy/spdy_http_stream_u...
net/spdy/spdy_http_stream_unittest.cc:125: host_port_pair));
On 2011/03/02 18:44:40, willchan wrote:
> host_port_pair must align with reads

Done.

http://codereview.chromium.org/6292013/diff/28001/net/spdy/spdy_http_stream_u...
net/spdy/spdy_http_stream_unittest.cc:138: scoped_ptr<SpdyHttpStream>
http_stream(
On 2011/03/02 18:44:40, willchan wrote:
> How about just instantiating this on the stack? Is there any reason to use a
> scoped_ptr?

Done.

http://codereview.chromium.org/6292013/diff/28001/net/spdy/spdy_http_stream_u...
net/spdy/spdy_http_stream_unittest.cc:159:
EXPECT_TRUE(!http_session_->spdy_session_pool()->HasSession(pair));
On 2011/03/02 18:44:40, willchan wrote:
> This should be EXPECT_FALSE instead of EXPECT_TRUE(!

Done.

[willchan no longer on Chromium, 2011-03-03 19:47:47]

http://codereview.chromium.org/6292013/diff/39001/net/base/upload_data.cc
File net/base/upload_data.cc (right):

http://codereview.chromium.org/6292013/diff/39001/net/base/upload_data.cc#new...
net/base/upload_data.cc:31: void UploadData::Element::SetToChunk(const char*
bytes, int bytes_len,
http://dev.chromium.org/developers/coding-style#Code_Formatting says:

"""
For function declarations and definitions, put each argument on a separate line
unless everything fits on one line. You should only break this rule when the
arguments have some logical association like "int x, int y" or "char* str, int
str_len" where putting a few arguments together makes semantic sense.  Do NOT
collapse function arguments together just because you have "too many":
"""

http://codereview.chromium.org/6292013/diff/39001/net/base/upload_data.cc#new...
net/base/upload_data.cc:145: void UploadData::AppendChunk(const char* bytes, int
bytes_len,
http://dev.chromium.org/developers/coding-style#Code_Formatting says:

"""
For function declarations and definitions, put each argument on a separate line
unless everything fits on one line. You should only break this rule when the
arguments have some logical association like "int x, int y" or "char* str, int
str_len" where putting a few arguments together makes semantic sense.  Do NOT
collapse function arguments together just because you have "too many":
"""

http://codereview.chromium.org/6292013/diff/39001/net/base/upload_data_stream.cc
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/6292013/diff/39001/net/base/upload_data_stream...
net/base/upload_data_stream.cc:146: bool UploadDataStream::peek_end_of_chunks()
const {
This is not a trivial accessor. Also, this may be better named as
IsOnLastChunk().

http://codereview.chromium.org/6292013/diff/39001/net/base/upload_data_stream...
net/base/upload_data_stream.cc:147: std::vector<UploadData::Element>& elements =
*data_->elements();
Can you get away with a const reference?

http://codereview.chromium.org/6292013/diff/39001/net/base/upload_data_stream...
net/base/upload_data_stream.cc:148: if (!data_->is_chunked())
If this doesn't depend on |elements|, then how about moving it above?

But should this ever be called in the non-chunked control path? Perhaps we
should have a DCHECK(data_->is_chunked()); I don't understand what
"peek_end_of_chunks()" means for a non-chunked UploadDataStream. Rather than
forcing weird semantics on the function, I think it's best to disallow it being
called. Callers should know they're dealing with chunked UploadDataStreams, or
they should check is_chunked().

http://codereview.chromium.org/6292013/diff/39001/net/base/upload_data_stream...
net/base/upload_data_stream.cc:152: next_element_ >= elements.size() - 1 &&
I find |next_element_ > elements.size()| more readable since you don't need the
- 1.

http://codereview.chromium.org/6292013/diff/39001/net/spdy/spdy_http_stream.cc
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/6292013/diff/39001/net/spdy/spdy_http_stream.c...
net/spdy/spdy_http_stream.cc:288: int SpdyHttpStream::OnSendBodyComplete(int
status, bool* eof) {
You should initialize *eof to false. Your comment in spdy_stream.h leaves this
state ambiguous. It's probably best to always set *eof. If *eof was already
true, then how can the user tell if this code also set it to true?

http://codereview.chromium.org/6292013/diff/39001/net/spdy/spdy_http_stream_u...
File net/spdy/spdy_http_stream_unittest.cc (right):

http://codereview.chromium.org/6292013/diff/39001/net/spdy/spdy_http_stream_u...
net/spdy/spdy_http_stream_unittest.cc:101: TEST_F(SpdyHttpStreamTest,
SendChunkedPost) {
Please add a test where you have multiple chunks.

http://codereview.chromium.org/6292013/diff/39001/net/spdy/spdy_network_trans...
File net/spdy/spdy_network_transaction_unittest.cc (right):

http://codereview.chromium.org/6292013/diff/39001/net/spdy/spdy_network_trans...
net/spdy/spdy_network_transaction_unittest.cc:1545:
TEST_P(SpdyNetworkTransactionTest, ChunkedPost) {
You should also have a test with multiple chunks.

http://codereview.chromium.org/6292013/diff/39001/net/spdy/spdy_proxy_client_...
File net/spdy/spdy_proxy_client_socket.h (right):

http://codereview.chromium.org/6292013/diff/39001/net/spdy/spdy_proxy_client_...
net/spdy/spdy_proxy_client_socket.h:98: virtual void
set_chunk_callback(ChunkCallback* /*callback*/) {}
Shouldn't this also do NOTREACHED() and return ERR_UNEXPECTED?

http://codereview.chromium.org/6292013/diff/39001/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/6292013/diff/39001/net/url_request/url_request...
net/url_request/url_request.h:334: bool is_last_chunk);
http://dev.chromium.org/developers/coding-style#Code_Formatting says:

"""
For function declarations and definitions, put each argument on a separate line
unless everything fits on one line. You should only break this rule when the
arguments have some logical association like "int x, int y" or "char* str, int
str_len" where putting a few arguments together makes semantic sense.  Do NOT
collapse function arguments together just because you have "too many":
"""

[Satish, 2011-03-03 22:21:43]

Thanks for the quick comments. I have addressed all style issues and some
replies below.

About adding tests with multiple chunks - that is not possible right now because
UploadDataStream combines all the chunks into a single block if they are < 16k
and I can only test a single mock read/write with that combined block. 
I'll need some test-only code in UploadDataStream so that it doesn't combine
chunks like this. I'd like to submit this CL as is add this test code +
additional unit tests in the next CL. Is that ok?

http://codereview.chromium.org/6292013/diff/39001/net/base/upload_data_stream.cc
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/6292013/diff/39001/net/base/upload_data_stream...
net/base/upload_data_stream.cc:152: next_element_ >= elements.size() - 1 &&
On 2011/03/03 19:47:48, willchan wrote:
> I find |next_element_ > elements.size()| more readable since you don't need
the
> - 1.

The condition could only be changed as "next_element_ > elements.size() - 2",
not as "next_element_ > elements.size()". So leaving as is.

http://codereview.chromium.org/6292013/diff/39001/net/spdy/spdy_http_stream.cc
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/6292013/diff/39001/net/spdy/spdy_http_stream.c...
net/spdy/spdy_http_stream.cc:288: int SpdyHttpStream::OnSendBodyComplete(int
status, bool* eof) {
On 2011/03/03 19:47:48, willchan wrote:
> You should initialize *eof to false. Your comment in spdy_stream.h leaves this
> state ambiguous. It's probably best to always set *eof. If *eof was already
> true, then how can the user tell if this code also set it to true?

*eof is already initialized below in line 292, not to false but to the
stream->eof(). Or have I misunderstood?

[willchan no longer on Chromium, 2011-03-03 22:26:54]

Sorry, it's not clear to me why you can't then add chunks that are over 16k?

http://codereview.chromium.org/6292013/diff/39001/net/base/upload_data_stream.cc
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/6292013/diff/39001/net/base/upload_data_stream...
net/base/upload_data_stream.cc:152: next_element_ >= elements.size() - 1 &&
On 2011/03/03 22:21:44, Satish wrote:
> On 2011/03/03 19:47:48, willchan wrote:
> > I find |next_element_ > elements.size()| more readable since you don't need
> the
> > - 1.
> 
> The condition could only be changed as "next_element_ > elements.size() - 2",
> not as "next_element_ > elements.size()". So leaving as is.

Oops, missed that! Bleh, off by one =/

http://codereview.chromium.org/6292013/diff/39001/net/spdy/spdy_http_stream.cc
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/6292013/diff/39001/net/spdy/spdy_http_stream.c...
net/spdy/spdy_http_stream.cc:288: int SpdyHttpStream::OnSendBodyComplete(int
status, bool* eof) {
On 2011/03/03 22:21:44, Satish wrote:
> On 2011/03/03 19:47:48, willchan wrote:
> > You should initialize *eof to false. Your comment in spdy_stream.h leaves
this
> > state ambiguous. It's probably best to always set *eof. If *eof was already
> > true, then how can the user tell if this code also set it to true?
> 
> *eof is already initialized below in line 292, not to false but to the
> stream->eof(). Or have I misunderstood?

Oops, I'm blind.

[Satish, 2011-03-03 22:35:31]

> Sorry, it's not clear to me why you can't then add chunks that are over
16k?

UploadDataStream would chop the chunks in this case to 16k. My point is that
the test will be relying on the 16k magic number which is a private
implementation detail of UploadDataStream. I think the correct way is to let
chunks be returned in their original size when in unit test mode and let the
test check for the same exact chunks as they were added. If you agree, I'll
do this in the next CL.

[willchan no longer on Chromium, 2011-03-03 22:40:16]

On Thu, Mar 3, 2011 at 2:35 PM, Satish Sampath <satish@chromium.org> wrote:
>> Sorry, it's not clear to me why you can't then add chunks that are over
>> 16k?
> UploadDataStream would chop the chunks in this case to 16k. My point is that
> the test will be relying on the 16k magic number which is a private
> implementation detail of UploadDataStream. I think the correct way is to let
> chunks be returned in their original size when in unit test mode and let the
> test check for the same exact chunks as they were added. If you agree, I'll
> do this in the next CL.

I see, you make a very good point. I agree with you that the test
should not rely on the 16k magic number. But right now, you are asking
me to let this change go in without testing perhaps the most important
part of the changelist! If you want to land your change sooner, I
suggest you write a test that relies on the 16k magic number, and fix
it in a following changelist.

[Satish, 2011-03-03 23:32:07]

Makes sense. I couldn't use 16k in the tests because SPDY data frames have a
limit of kMaxSpdyFrameChunkSize in code which is < 16k. So I have added a
flag to UploadDataStream to indicate that chunks should not be merged and
set this flag from the unit tests. Latest patch uploaded with these changes
and updated tests.

[willchan no longer on Chromium, 2011-03-03 23:42:32]

Great! LGTM.

http://codereview.chromium.org/6292013/diff/37026/net/base/upload_data_stream.cc
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/6292013/diff/37026/net/base/upload_data_stream...
net/base/upload_data_stream.cc:156: next_element_ == elements.size() &&
What's going on here? Did we catch a bug? It's not immediately obvious what this
change is for.

http://codereview.chromium.org/6292013/diff/37026/net/base/upload_data_stream.h
File net/base/upload_data_stream.h (right):

http://codereview.chromium.org/6292013/diff/37026/net/base/upload_data_stream...
net/base/upload_data_stream.h:65: #if UNIT_TEST
most chromium code does defined(UNIT_TEST), not just #if UNIT_TEST. It's
probably best to be consistent.

Please add a TODO to remove this. And I expect you to do this one after you
submit this changelist, because this is kludgy. I think you should add stuff to
UploadData one by one.

[Satish, 2011-03-03 23:50:58]

Thanks!

http://codereview.chromium.org/6292013/diff/37026/net/base/upload_data_stream.cc
File net/base/upload_data_stream.cc (right):

http://codereview.chromium.org/6292013/diff/37026/net/base/upload_data_stream...
net/base/upload_data_stream.cc:156: next_element_ == elements.size() &&
On 2011/03/03 23:42:32, willchan wrote:
> What's going on here? Did we catch a bug? It's not immediately obvious what
this
> change is for.

Sorry I missed this. Yes I found a bug when testing with 16k sized chunks. This
method should return true only after the whole chunk data was copied over to the
IOBuffer. Since we no longer have the old zero-size chunk to indicate the end
and instead have the flag on the last valid chunk, next_element_ should have
gone past all chunks once the last chunk was read into the IOBuffer.

http://codereview.chromium.org/6292013/diff/37026/net/base/upload_data_stream.h
File net/base/upload_data_stream.h (right):

http://codereview.chromium.org/6292013/diff/37026/net/base/upload_data_stream...
net/base/upload_data_stream.h:65: #if UNIT_TEST
On 2011/03/03 23:42:32, willchan wrote:
> most chromium code does defined(UNIT_TEST), not just #if UNIT_TEST. It's
> probably best to be consistent.

Will do.

> Please add a TODO to remove this. And I expect you to do this one after you
> submit this changelist, because this is kludgy. I think you should add stuff
to
> UploadData one by one.

With the current mock read/write testing model, I can't find easily how to break
in the middle of an ongoing sequence and add more chunks to the stream. If you
have any suggestions please let me know, perhaps topic for a separate mail
thread.

[Satish, 2011-03-03 23:57:06]

http://codereview.chromium.org/6292013/diff/37026/net/spdy/spdy_proxy_client_...
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/6292013/diff/37026/net/spdy/spdy_proxy_client_...
net/spdy/spdy_proxy_client_socket.cc:502: NOTREACHED();
Looks like adding this was incorrect as SpdyStream is calling this method when
sending a request and detaching. SpdyStream doesn't know about the actual POST
data so it can't call this method only for chunked POST data. Is it ok if I
remove the check and leave the method empty?

[willchan no longer on Chromium, 2011-03-04 00:02:55]

http://codereview.chromium.org/6292013/diff/37026/net/spdy/spdy_proxy_client_...
File net/spdy/spdy_proxy_client_socket.cc (right):

http://codereview.chromium.org/6292013/diff/37026/net/spdy/spdy_proxy_client_...
net/spdy/spdy_proxy_client_socket.cc:502: NOTREACHED();
On 2011/03/03 23:57:06, Satish wrote:
> Looks like adding this was incorrect as SpdyStream is calling this method when
> sending a request and detaching. SpdyStream doesn't know about the actual POST
> data so it can't call this method only for chunked POST data. Is it ok if I
> remove the check and leave the method empty?

OK