Cleanup the FlipDelegate API a bit in prep for fixing upload....

Cleanup the FlipDelegate API a bit in prep for fixing upload.

Document the API for FlipDelegate in flip_session.h.
Remove the method OnCancel, since it was not needed.
Add the method OnBodySent, which is not yet used but will
be.

FlipNetworkTransaction is a FlipDelegate, so the changes
there are just to reflect the new API.

BUG=none
TEST=none

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

[Mike Belshe, 2009-10-28 16:37:14]

Start with flip_session.h, I've documented the API for FlipDelegate.

I removed the method OnCancel, since it was not needed.
I added the method OnBodySent, which is not yet used but will be.

FlipNetworkTransaction is a FlipDelegate, so the changes there are just to
reflect the new API.

[wtc, 2009-10-28 19:04:34]

LGTM.

Your comment is a nice description of the CL, so I took the
liberty of adding it to the CL's description.  (Our project
has a lot of long commit messages, so this should be OK.)

I like your documentation of the FlipDelegate API a lot.
I suggested some changes to clarify it below.

http://codereview.chromium.org/339047/diff/1/3
File net/flip/flip_session.h (right):

http://codereview.chromium.org/339047/diff/1/3#newcode34
Line 34: // A callback interface for HTTP content retrieved from the Flip
stream.
It would be nice to document who calls the FlipDelegate
interface.  I think it's FlipSession, right?

http://codereview.chromium.org/339047/diff/1/3#newcode41
Line 41: // The delegate provides access to the HttpRequest for use by the flip
Typo: HttpRequest => HttpRequestInfo

http://codereview.chromium.org/339047/diff/1/3#newcode46
Line 46: // flip session.  If the delegate is not uploading content, this call
Question: who is uploading content?  The delegate or the
FlipSession?

Should we rename this method upload_data()?

http://codereview.chromium.org/339047/diff/1/3#newcode58
Line 58: virtual void OnBodySent(int status) = 0;
Do you want to rename this method OnUploadDataSent, since
you're talking about "upload data" rather than "body"
everywhere?

Since the int argument can be a byte count, it'd be nice
to name it 'result', which is more general.  'status'
suggests a succeed/fail status or an error status code.

http://codereview.chromium.org/339047/diff/1/3#newcode62
Line 62: // |status| contains an error code if a failure has occurred or OK
I'd also rename the argument 'result' even though it is truly
a status for this method.

http://codereview.chromium.org/339047/diff/1/3#newcode66
Line 66: // Called by the FlipSession when a response (e.g. a SYN_REPLY) has
been
It's important to point out that when this callback is called,
exactly what parts of a response have been received.  For
example, the response data hasn't been received; that's the
job of OnDataReceived.

http://codereview.chromium.org/339047/diff/1/3#newcode74
Line 74: // |buffer| contains the data received.
Document the lifetime of |buffer|.  Does the delegate need
to copy the data in |buffer| before onDataReceived?

http://codereview.chromium.org/339047/diff/1/3#newcode75
Line 75: // |bytes| is the number of bytes received or an error.
If |bytes| is 0, does it mean end of stream/response body?

[Mike Belshe, 2009-10-28 19:30:53]

http://codereview.chromium.org/339047/diff/1/3
File net/flip/flip_session.h (right):

http://codereview.chromium.org/339047/diff/1/3#newcode34
Line 34: // A callback interface for HTTP content retrieved from the Flip
stream.
On 2009/10/28 19:04:34, wtc wrote:
> It would be nice to document who calls the FlipDelegate
> interface.  I think it's FlipSession, right?

Done.

http://codereview.chromium.org/339047/diff/1/3#newcode41
Line 41: // The delegate provides access to the HttpRequest for use by the flip
On 2009/10/28 19:04:34, wtc wrote:
> Typo: HttpRequest => HttpRequestInfo

Done.

http://codereview.chromium.org/339047/diff/1/3#newcode46
Line 46: // flip session.  If the delegate is not uploading content, this call
It is a bit TBD; basically the delegate (we need the delegate to participate at
least for status so you can see what percentage has been uploaded)

But more work may be needed here.
On 2009/10/28 19:04:34, wtc wrote:
> Question: who is uploading content?  The delegate or the
> FlipSession?
> 
> Should we rename this method upload_data()?

http://codereview.chromium.org/339047/diff/1/3#newcode58
Line 58: virtual void OnBodySent(int status) = 0;
On 2009/10/28 19:04:34, wtc wrote:
> Do you want to rename this method OnUploadDataSent, since
> you're talking about "upload data" rather than "body"
> everywhere?
> 
> Since the int argument can be a byte count, it'd be nice
> to name it 'result', which is more general.  'status'
> suggests a succeed/fail status or an error status code.

Done.

http://codereview.chromium.org/339047/diff/1/3#newcode62
Line 62: // |status| contains an error code if a failure has occurred or OK
On 2009/10/28 19:04:34, wtc wrote:
> I'd also rename the argument 'result' even though it is truly
> a status for this method.

Done.

http://codereview.chromium.org/339047/diff/1/3#newcode66
Line 66: // Called by the FlipSession when a response (e.g. a SYN_REPLY) has
been
On 2009/10/28 19:04:34, wtc wrote:
> It's important to point out that when this callback is called,
> exactly what parts of a response have been received.  For
> example, the response data hasn't been received; that's the
> job of OnDataReceived.

Done.

http://codereview.chromium.org/339047/diff/1/3#newcode74
Line 74: // |buffer| contains the data received.
On 2009/10/28 19:04:34, wtc wrote:
> Document the lifetime of |buffer|.  Does the delegate need
> to copy the data in |buffer| before onDataReceived?

Done.

http://codereview.chromium.org/339047/diff/1/3#newcode75
Line 75: // |bytes| is the number of bytes received or an error.
On 2009/10/28 19:04:34, wtc wrote:
> If |bytes| is 0, does it mean end of stream/response body?

Done.