[SPDY] Remove SpdyStream::Delegate::OnSendBody()'s return value

[SPDY] Remove SpdyStream::Delegate::OnSendBody()'s return value

Add comment explaining semantics of OnSendBody().
Audit SpdyStream::Delegate implementations to either
comply with those semantics or CHECK-fail.

Strengthen some related checks in SpdyHttpStream.

Fix typo in spdy_stream_spdy3_unittest.cc.

BUG=242288
R=rch@chromium.org

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

[akalin, 2013-05-22 00:27:11]

+rch for review

[akalin, 2013-05-22 00:29:09]

Incidentally, I remember you mentioning a possible latent bug with flow control
and SpdyHttpStream::OnSendBody() if it needs to read more data from the upload
stream. But I think there's no bug; if the HTTP stream has to read from the
upload stream, and that read finishes asynchronously, then that call to
QueueStreamData() is dropped, but when the SpdyStream is unstalled, it'll call
OnSendBody() again, which will call QueueStreamData() again since the read is
queued.

Anyway, QueueStreamData() will be made 'reliable' in an upcoming CL.

[Ryan Hamilton, 2013-05-22 03:17:30]

https://chromiumcodereview.appspot.com/15555003/diff/2001/net/spdy/spdy_http_...
File net/spdy/spdy_http_stream.cc (right):

https://chromiumcodereview.appspot.com/15555003/diff/2001/net/spdy/spdy_http_...
net/spdy/spdy_http_stream.cc:465: CHECK_GE(request_body_buf_->BytesRemaining(),
0);
Alternatively:
CHECK_GE(request_body_buf_->BytesRemaining(), eof ? 0 : 1);

Why CHECK not DCHECK?  Is this to get crash reports to help debug?

https://chromiumcodereview.appspot.com/15555003/diff/2001/net/spdy/spdy_proxy...
File net/spdy/spdy_proxy_client_socket.cc (right):

https://chromiumcodereview.appspot.com/15555003/diff/2001/net/spdy/spdy_proxy...
net/spdy/spdy_proxy_client_socket.cc:474: CHECK(false);
CHECK(false)?  This is pretty hardcore.  Are you sure you want to crash the
user's browser in this case?

https://chromiumcodereview.appspot.com/15555003/diff/2001/net/spdy/spdy_proxy...
net/spdy/spdy_proxy_client_socket.cc:481: CHECK(false);
ditto

https://chromiumcodereview.appspot.com/15555003/diff/2001/net/spdy/spdy_strea...
File net/spdy/spdy_stream.cc (right):

https://chromiumcodereview.appspot.com/15555003/diff/2001/net/spdy/spdy_strea...
net/spdy/spdy_stream.cc:756: result = ERR_IO_PENDING;
Yeah, this looks weird to me.  I think it fits our pattern more consistently by
having DoSendBody return ERR_IO_PENDING.

https://chromiumcodereview.appspot.com/15555003/diff/2001/net/spdy/spdy_stream.h
File net/spdy/spdy_stream.h (right):

https://chromiumcodereview.appspot.com/15555003/diff/2001/net/spdy/spdy_strea...
net/spdy/spdy_stream.h:66: // to be read asynchronously). Must return
ERR_IO_PENDING.
Looks like it's now void

https://chromiumcodereview.appspot.com/15555003/diff/2001/net/spdy/spdy_strea...
net/spdy/spdy_stream.h:71: // TODO(akalin): Remove the return value.
I think you removed the return value, didn't you?

https://chromiumcodereview.appspot.com/15555003/diff/2001/net/spdy/spdy_strea...
net/spdy/spdy_stream.h:372: void DoSendBody();
I don't think this is a good idea.  The pattern we use for state machines is
that each step returns the result from that step... even if it's always OK or
always ERR_IO_PENDING.

https://chromiumcodereview.appspot.com/15555003/diff/2001/net/spdy/spdy_webso...
File net/spdy/spdy_websocket_stream.cc (right):

https://chromiumcodereview.appspot.com/15555003/diff/2001/net/spdy/spdy_webso...
net/spdy/spdy_websocket_stream.cc:90: CHECK(false);
You sure like checks :>

[akalin, 2013-05-22 08:40:05]

Comments addressed, PTAL.

https://codereview.chromium.org/15555003/diff/2001/net/spdy/spdy_http_stream.cc
File net/spdy/spdy_http_stream.cc (right):

https://codereview.chromium.org/15555003/diff/2001/net/spdy/spdy_http_stream....
net/spdy/spdy_http_stream.cc:465: CHECK_GE(request_body_buf_->BytesRemaining(),
0);
On 2013/05/22 03:17:30, Ryan Hamilton wrote:
> Alternatively:
> CHECK_GE(request_body_buf_->BytesRemaining(), eof ? 0 : 1);

Hmm, I prefer the split-out CHECK, simply because one knows from examining a
crash dump what the value of eof is (from the line number).

> Why CHECK not DCHECK?  Is this to get crash reports to help debug?

Yeah, pretty much. If this happens in the wild, I'd like to know about it.

https://codereview.chromium.org/15555003/diff/2001/net/spdy/spdy_proxy_client...
File net/spdy/spdy_proxy_client_socket.cc (right):

https://codereview.chromium.org/15555003/diff/2001/net/spdy/spdy_proxy_client...
net/spdy/spdy_proxy_client_socket.cc:474: CHECK(false);
On 2013/05/22 03:17:30, Ryan Hamilton wrote:
> CHECK(false)?  This is pretty hardcore.  Are you sure you want to crash the
> user's browser in this case?

Yeah, I think it's for the best. If this were originally a CHECK(false), I think
we would have caught this bug much sooner!

I don't think I've ever regretted converting something to a CHECK(), as it
provides clear feedback on what goes wrong via canary/dev channel. The worst
thing that could happen is that it starts triggering on the stable channel, but
I think we're pretty good at catching things before they get that far.

https://codereview.chromium.org/15555003/diff/2001/net/spdy/spdy_proxy_client...
net/spdy/spdy_proxy_client_socket.cc:481: CHECK(false);
On 2013/05/22 03:17:30, Ryan Hamilton wrote:
> ditto

ditto.

https://codereview.chromium.org/15555003/diff/2001/net/spdy/spdy_stream.cc
File net/spdy/spdy_stream.cc (right):

https://codereview.chromium.org/15555003/diff/2001/net/spdy/spdy_stream.cc#ne...
net/spdy/spdy_stream.cc:756: result = ERR_IO_PENDING;
On 2013/05/22 03:17:30, Ryan Hamilton wrote:
> Yeah, this looks weird to me.  I think it fits our pattern more consistently
by
> having DoSendBody return ERR_IO_PENDING.

Done.

https://codereview.chromium.org/15555003/diff/2001/net/spdy/spdy_stream.h
File net/spdy/spdy_stream.h (right):

https://codereview.chromium.org/15555003/diff/2001/net/spdy/spdy_stream.h#new...
net/spdy/spdy_stream.h:66: // to be read asynchronously). Must return
ERR_IO_PENDING.
On 2013/05/22 03:17:30, Ryan Hamilton wrote:
> Looks like it's now void

Done.

https://codereview.chromium.org/15555003/diff/2001/net/spdy/spdy_stream.h#new...
net/spdy/spdy_stream.h:71: // TODO(akalin): Remove the return value.
On 2013/05/22 03:17:30, Ryan Hamilton wrote:
> I think you removed the return value, didn't you?

Done.

https://codereview.chromium.org/15555003/diff/2001/net/spdy/spdy_stream.h#new...
net/spdy/spdy_stream.h:372: void DoSendBody();
On 2013/05/22 03:17:30, Ryan Hamilton wrote:
> I don't think this is a good idea.  The pattern we use for state machines is
> that each step returns the result from that step... even if it's always OK or
> always ERR_IO_PENDING.

Done.

[Ryan Hamilton, 2013-05-22 18:41:36]

lgtm

[commit-bot: I haz the power, 2013-05-22 18:59:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/akalin@chromium.org/15555003/15001

[akalin, 2013-05-22 22:35:22]

Committed patchset #4 manually as r201622 (presubmit successful).