Update the FLIP session to use the FlipIOBuffer....

Update the FLIP session to use the FlipIOBuffer.
Also removed some of the testing hacks for URL rewriting
that are no longer needed.

This change removes the 'batching' of frames written
to the socket.  The reason for doing this is because
I'm going to need to start notifying to the upper layer
as progress is made on the writes (e.g. upload notifications).
When the frames are batched (potentially from different 
transactions), it becomes very difficult to know when each
write completes.  I don't think that batching is necessary, 
as writes will accummulate in the socket buffer, so this
should be a better approach.

BUG=none
TEST=flip_session_unittest.cc


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

[wtc, 2009-11-03 17:55:41]

LGTM.  Please see my comments below.  Some of the issues
could be serious.

You may want to describe your change to
FlipSession::WriteSocket (you're no longer accumulating
four MSS's of data and sending them in batch).

http://codereview.chromium.org/342088/diff/4001/5002
File net/flip/flip_session.cc (right):

http://codereview.chromium.org/342088/diff/4001/5002#newcode116
Line 116: std::string hack_url(info->url.PathForRequest());
This variable doesn't need to be named "hack_url" any
more because you removed the ifdef for the hack.

I'd just inline the variable in line 119:
  (*headers)["url"] = info->url.PathForRequest();

http://codereview.chromium.org/342088/diff/4001/5002#newcode353
Line 353: DCHECK(static_cast<size_t>(result) == in_flight_write_.size());
Nit: there is a DCHECK_EQ macro for this (cpplint would
suggest this).

http://codereview.chromium.org/342088/diff/4001/5002#newcode415
Line 415: // Loop sending frames until we've sent everything or until the write
We may need to exit the loop if the write fails.  See my
comment at line 441 below.

http://codereview.chromium.org/342088/diff/4001/5002#newcode426
Line 426: scoped_array<flip::FlipFrame> compressed_frame(
BUG: scoped_array's destructor uses the delete [] operator
to delete the memory, but it treats the pointer as FlipFrame*.

On the other hand, FlipFramer::CompressFrame allocates the
returned FlipFrame using the new char[] operator.  So there
is a type mismatch between allocation and deallocation.

Perhaps we should define a constructor and destructor for
FlipFrame, so that we can use scoped_ptr<flip::FlipFrame>
here instead?

http://codereview.chromium.org/342088/diff/4001/5002#newcode428
Line 428: size_t size = compressed_frame.get()->length() +
sizeof(flip::FlipFrame);
You may be able to omit the .get() method call here.

http://codereview.chromium.org/342088/diff/4001/5002#newcode441
Line 441: if (rv == net::ERR_IO_PENDING)
We should also do something if |rv| is a real error.
It seems that we should also break out of the while loop
if |rv| is a real error.

http://codereview.chromium.org/342088/diff/4001/5002#newcode444
Line 444: // We sent the frame successfully.
This comment is not accurate because |rv| may be a real
error.

http://codereview.chromium.org/342088/diff/4001/5001
File net/flip/flip_session_unittest.cc (right):

http://codereview.chromium.org/342088/diff/4001/5001#newcode10
Line 10: #include "net/flip/flip_io_buffer.h"
Nit: list flip_io_buffer.h before flip_session.h.  Also they
should be listed along with the other three headers.

Note: the style guide vaguely recommends that the unit
test file for a class should include the class's .h file
first, so perhaps we should just move flip_session.h to the
top of the file:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

[Mike Belshe, 2009-11-03 21:57:49]

http://codereview.chromium.org/342088/diff/4001/5002
File net/flip/flip_session.cc (right):

http://codereview.chromium.org/342088/diff/4001/5002#newcode116
Line 116: std::string hack_url(info->url.PathForRequest());
On 2009/11/03 17:55:41, wtc wrote:
> This variable doesn't need to be named "hack_url" any
> more because you removed the ifdef for the hack.
> 
> I'd just inline the variable in line 119:
>   (*headers)["url"] = info->url.PathForRequest();

Done.

http://codereview.chromium.org/342088/diff/4001/5002#newcode353
Line 353: DCHECK(static_cast<size_t>(result) == in_flight_write_.size());
On 2009/11/03 17:55:41, wtc wrote:
> Nit: there is a DCHECK_EQ macro for this (cpplint would
> suggest this).

Done.

http://codereview.chromium.org/342088/diff/4001/5002#newcode426
Line 426: scoped_array<flip::FlipFrame> compressed_frame(
You're right.  I need to fix this.  This particular problem isn't new to this
CL, and I suspect it works because we have no destructors. To fix right, I need
to do a significant overhaul of flip_protocol.h, which I will do in my next CL
tomorrow.

On 2009/11/03 17:55:41, wtc wrote:
> BUG: scoped_array's destructor uses the delete [] operator
> to delete the memory, but it treats the pointer as FlipFrame*.
> 
> On the other hand, FlipFramer::CompressFrame allocates the
> returned FlipFrame using the new char[] operator.  So there
> is a type mismatch between allocation and deallocation.
> 
> Perhaps we should define a constructor and destructor for
> FlipFrame, so that we can use scoped_ptr<flip::FlipFrame>
> here instead?
> 
>

http://codereview.chromium.org/342088/diff/4001/5002#newcode428
Line 428: size_t size = compressed_frame.get()->length() +
sizeof(flip::FlipFrame);
I need to get rid of the scoped_array first.

On 2009/11/03 17:55:41, wtc wrote:
> You may be able to omit the .get() method call here.

http://codereview.chromium.org/342088/diff/4001/5002#newcode441
Line 441: if (rv == net::ERR_IO_PENDING)
I'm not sure that we should?  We still need to pass the error to
OnWriteComplete.  I guess the only thing we could do would be to terminate the
entire connection.  I added a note with a TODO.  This is basically a duplicate
of the TODO I already have for dealing with write errors.

On 2009/11/03 17:55:41, wtc wrote:
> We should also do something if |rv| is a real error.
> It seems that we should also break out of the while loop
> if |rv| is a real error.

http://codereview.chromium.org/342088/diff/4001/5001
File net/flip/flip_session_unittest.cc (right):

http://codereview.chromium.org/342088/diff/4001/5001#newcode10
Line 10: #include "net/flip/flip_io_buffer.h"
On 2009/11/03 17:55:41, wtc wrote:
> Nit: list flip_io_buffer.h before flip_session.h.  Also they
> should be listed along with the other three headers.
> 
> Note: the style guide vaguely recommends that the unit
> test file for a class should include the class's .h file
> first, so perhaps we should just move flip_session.h to the
> top of the file:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde...

Done.

[wtc, 2009-11-03 23:28:13]

LGTM.

http://codereview.chromium.org/342088/diff/6004/6006
File net/flip/flip_session.cc (right):

http://codereview.chromium.org/342088/diff/6004/6006#newcode415
Line 415: // returns ERR_IO_PENDING.
This comment needs updating to match the new code.
I think you can just say "until the write fails".

http://codereview.chromium.org/342088/diff/6004/6006#newcode429
Line 429: 
Nit: you added an extra blank line.

http://codereview.chromium.org/342088/diff/6004/6006#newcode448
Line 448: if (rv < 0)
I agree with this new code.  We may need to do something
to mark the connection as in an error state and cause the
connection to be closed.