Rename PrioritizedIOBuffer to FlipIOBuffer, refactor it into...

Rename PrioritizedIOBuffer to FlipIOBuffer, refactor it into
its own file, and have it carry a FlipStream pointer.

The PrioritizedIOBuffer was more generic, but after we queued IO,
we couldn't track which stream should be notified about the IO
completion.  Having it carry the FlipStream pointer will enable
IO tracking, but with a FlipStream pointer, it is really specific
to Flip, so I renamed to FlipIOBuffer.  I could have kept a generic
(void*) pointer (or used a template), but that seemed unnecessary
in this case.

This CL just changes the refactoring.  Will remove the
PrioritizedIOBuffer next.

BUG=none
TEST=none


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

[wtc, 2009-10-29 16:51:38]

LGTM.  Note in particular the potential overflow/wraparound
issue with the static int order_ member below.

http://codereview.chromium.org/341032/diff/2002/2005
File net/flip/flip_io_buffer.h (right):

http://codereview.chromium.org/341032/diff/2002/2005#newcode8
Line 8: #include "base/scoped_ptr.h"
This file is using scoped_refptr instead of
scoped_ptr, so perhaps it should include
"base/refcounted.h" instead of "base/scoped_ptr.h"?

("net/base/io_buffer.h" already includes
"base/refcounted.h".)

http://codereview.chromium.org/341032/diff/2002/2005#newcode25
Line 25: // |stream| is a pointer to the stream which owns this buffer.
Question: by "owns this buffer", do you mean the stream is
responsible for destroying this buffer?

http://codereview.chromium.org/341032/diff/2002/2005#newcode32
Line 32: FlipIOBuffer() : buffer_(0), priority_(0), stream_(NULL) {}
Nit: Initialize buffer_ with NULL instead of 0.  Actually,
since buffer_ is a scoped_refptr, we can rely on its
default constructor and don't need to initialize buffer_
explicitly.

http://codereview.chromium.org/341032/diff/2002/2005#newcode38
Line 38: int priority() { return priority_; }
Nit: both priority() and stream() can be 'const'.

http://codereview.chromium.org/341032/diff/2002/2005#newcode41
Line 41: // Comparison operator to support sorting.
It's confusing that a lower priority number has a higher
priority.

http://codereview.chromium.org/341032/diff/2002/2005#newcode45
Line 45: return position_ >= other.position_;
I'm not familiar with how operator< should be defined.
Are you sure we should use >= rather than > here?

http://codereview.chromium.org/341032/diff/2002/2005#newcode53
Line 53: static int order_;  // Maintains a FIFO order for equal priorities.
Is it possible for this global 'int' counter to overflow?
'int' can represent up to 2G.

If it wraps around and becomes a very negative number, will
that be problematic for operator< ?

Maybe order_ should be an unsigned type and should be
64-bit?

http://codereview.chromium.org/341032/diff/2002/2003
File net/net.gyp (right):

http://codereview.chromium.org/341032/diff/2002/2003#newcode289
Line 289: 'flip/flip_stream.cc',
These two come from the other CL I reviewed yesterday, right?

[Mike Belshe, 2009-10-29 19:35:36]

http://codereview.chromium.org/341032/diff/2002/2005
File net/flip/flip_io_buffer.h (right):

http://codereview.chromium.org/341032/diff/2002/2005#newcode8
Line 8: #include "base/scoped_ptr.h"
On 2009/10/29 16:51:39, wtc wrote:
> This file is using scoped_refptr instead of
> scoped_ptr, so perhaps it should include
> "base/refcounted.h" instead of "base/scoped_ptr.h"?
> 
> ("net/base/io_buffer.h" already includes
> "base/refcounted.h".)

Done.

http://codereview.chromium.org/341032/diff/2002/2005#newcode25
Line 25: // |stream| is a pointer to the stream which owns this buffer.
Hmm.  I reworded to "which is managing this buffer".

In some sense, it's just a pointer we carry with the buffer for tracking
purposes.

On 2009/10/29 16:51:39, wtc wrote:
> Question: by "owns this buffer", do you mean the stream is
> responsible for destroying this buffer?

http://codereview.chromium.org/341032/diff/2002/2005#newcode32
Line 32: FlipIOBuffer() : buffer_(0), priority_(0), stream_(NULL) {}
On 2009/10/29 16:51:39, wtc wrote:
> Nit: Initialize buffer_ with NULL instead of 0.  Actually,
> since buffer_ is a scoped_refptr, we can rely on its
> default constructor and don't need to initialize buffer_
> explicitly.

Done.

http://codereview.chromium.org/341032/diff/2002/2005#newcode38
Line 38: int priority() { return priority_; }
On 2009/10/29 16:51:39, wtc wrote:
> Nit: both priority() and stream() can be 'const'.

Done.

http://codereview.chromium.org/341032/diff/2002/2005#newcode41
Line 41: // Comparison operator to support sorting.
Well, that's the way it works :-)  Documented it though!

Our bug system is the same way - P0 is highest.

On 2009/10/29 16:51:39, wtc wrote:
> It's confusing that a lower priority number has a higher
> priority.

http://codereview.chromium.org/341032/diff/2002/2005#newcode45
Line 45: return position_ >= other.position_;
It doesn't matter.  Changed because it reads more clearly.
On 2009/10/29 16:51:39, wtc wrote:
> I'm not familiar with how operator< should be defined.
> Are you sure we should use >= rather than > here?

http://codereview.chromium.org/341032/diff/2002/2005#newcode53
Line 53: static int order_;  // Maintains a FIFO order for equal priorities.
On 2009/10/29 16:51:39, wtc wrote:
> Is it possible for this global 'int' counter to overflow?
> 'int' can represent up to 2G.
> 
> If it wraps around and becomes a very negative number, will
> that be problematic for operator< ?
> 
> Maybe order_ should be an unsigned type and should be
> 64-bit?

Done.

[wtc, 2009-10-30 01:00:53]

LGTM.

http://codereview.chromium.org/341032/diff/2002/2005
File net/flip/flip_io_buffer.h (right):

http://codereview.chromium.org/341032/diff/2002/2005#newcode45
Line 45: return position_ >= other.position_;
On 2009/10/29 19:35:36, Mike Belshe wrote:
> It doesn't matter.  Changed because it reads more clearly.

I'm worried that the original code will cause
  buffer_a < buffer_a
(i.e., comparing a buffer with itself) to return true,
which would be wrong.  The new code eliminates this
concern.