Extend the IOBuffer to the disk cache.

Extend the IOBuffer to the disk cache.

This is cleanup from bug 5325.


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

[darin (slow to review), 2009-02-06 23:06:19]

LG overall.  Just minor nits and naming suggestions.

I think it would be good to standardize on the name we use for variables of type
IOBuffer.  Since they have a data() member, using data as the name of an
IOBuffer results in the awkward data->data() in the code.  I think either buf or
buffer would be better choices for the name of an IOBuffer.

http://codereview.chromium.org/20134/diff/1/18
File net/disk_cache/backend_unittest.cc (right):

http://codereview.chromium.org/20134/diff/1/18#newcode214
Line 214: scoped_refptr<net::IOBuffer> data = new net::IOBuffer(kDataSize);
nit: buf or buffer might be better names than data for this variable

http://codereview.chromium.org/20134/diff/1/18#newcode345
Line 345: scoped_refptr<net::IOBuffer> data = new net::IOBuffer(kSize);
nit: s/data/buffer/ ?

http://codereview.chromium.org/20134/diff/1/18#newcode373
Line 373: scoped_refptr<net::IOBuffer> data = new net::IOBuffer(kSize);
ditto

http://codereview.chromium.org/20134/diff/1/18#newcode394
Line 394: scoped_refptr<net::IOBuffer> data = new net::IOBuffer(kSize);
ditto

http://codereview.chromium.org/20134/diff/1/18#newcode559
Line 559: scoped_refptr<net::IOBuffer> data = new net::IOBuffer(kSize);
ditto

http://codereview.chromium.org/20134/diff/1/15
File net/disk_cache/disk_cache.h (right):

http://codereview.chromium.org/20134/diff/1/15#newcode134
Line 134: // performed from the callback code.
there should probably be some comments added to explain that a reference to the
given IOBuffer will be retained in the asynchronous case.

http://codereview.chromium.org/20134/diff/1/15#newcode149
Line 149: // what we are writing here.
ditto

http://codereview.chromium.org/20134/diff/1/14
File net/disk_cache/stress_cache.cc (right):

http://codereview.chromium.org/20134/diff/1/14#newcode101
Line 101: scoped_refptr<net::IOBuffer> data = new net::IOBuffer(kDataLen);
nit: s/data/buffer/

http://codereview.chromium.org/20134/diff/1/4
File net/http/http_cache.cc (right):

http://codereview.chromium.org/20134/diff/1/4#newcode31
Line 31: class TempIOBuffer : public net::IOBuffer {
nit: how about naming this DependentIOBuffer?  maybe this class would be good to
declare in io_buffer.h since it is used in two places.  that seems better than
defining it twice.

http://codereview.chromium.org/20134/diff/1/4#newcode33
Line 33: TempIOBuffer(const char* buffer) :
net::IOBuffer(const_cast<char*>(buffer)) {}
nit: s/buffer/data/ ?

http://codereview.chromium.org/20134/diff/1/7
File net/tools/dump_cache/upgrade.cc (right):

http://codereview.chromium.org/20134/diff/1/7#newcode20
Line 20: class TempIOBuffer : public net::IOBuffer {
nit: DependentIOBuffer?

[rvargas (doing something else), 2009-02-07 01:49:26]

Thanks. Could you take another look?.

On 2009/02/06 23:06:19, darin wrote:
> LG overall.  Just minor nits and naming suggestions.
> 
> I think it would be good to standardize on the name we use for variables of
type
> IOBuffer.  Since they have a data() member, using data as the name of an
> IOBuffer results in the awkward data->data() in the code.  I think either buf
or
> buffer would be better choices for the name of an IOBuffer.
> 
> http://codereview.chromium.org/20134/diff/1/18
> File net/disk_cache/backend_unittest.cc (right):
> 
> http://codereview.chromium.org/20134/diff/1/18#newcode214
> Line 214: scoped_refptr<net::IOBuffer> data = new net::IOBuffer(kDataSize);
> nit: buf or buffer might be better names than data for this variable
> 
> http://codereview.chromium.org/20134/diff/1/18#newcode345
> Line 345: scoped_refptr<net::IOBuffer> data = new net::IOBuffer(kSize);
> nit: s/data/buffer/ ?
> 
> http://codereview.chromium.org/20134/diff/1/18#newcode373
> Line 373: scoped_refptr<net::IOBuffer> data = new net::IOBuffer(kSize);
> ditto
> 
> http://codereview.chromium.org/20134/diff/1/18#newcode394
> Line 394: scoped_refptr<net::IOBuffer> data = new net::IOBuffer(kSize);
> ditto
> 
> http://codereview.chromium.org/20134/diff/1/18#newcode559
> Line 559: scoped_refptr<net::IOBuffer> data = new net::IOBuffer(kSize);
> ditto
> 
> http://codereview.chromium.org/20134/diff/1/15
> File net/disk_cache/disk_cache.h (right):
> 
> http://codereview.chromium.org/20134/diff/1/15#newcode134
> Line 134: // performed from the callback code.
> there should probably be some comments added to explain that a reference to
the
> given IOBuffer will be retained in the asynchronous case.
> 
> http://codereview.chromium.org/20134/diff/1/15#newcode149
> Line 149: // what we are writing here.
> ditto
> 
> http://codereview.chromium.org/20134/diff/1/14
> File net/disk_cache/stress_cache.cc (right):
> 
> http://codereview.chromium.org/20134/diff/1/14#newcode101
> Line 101: scoped_refptr<net::IOBuffer> data = new net::IOBuffer(kDataLen);
> nit: s/data/buffer/
> 
> http://codereview.chromium.org/20134/diff/1/4
> File net/http/http_cache.cc (right):
> 
> http://codereview.chromium.org/20134/diff/1/4#newcode31
> Line 31: class TempIOBuffer : public net::IOBuffer {
> nit: how about naming this DependentIOBuffer?  maybe this class would be good
to
> declare in io_buffer.h since it is used in two places.  that seems better than
> defining it twice.
> 
> http://codereview.chromium.org/20134/diff/1/4#newcode33
> Line 33: TempIOBuffer(const char* buffer) :
> net::IOBuffer(const_cast<char*>(buffer)) {}
> nit: s/buffer/data/ ?
> 
> http://codereview.chromium.org/20134/diff/1/7
> File net/tools/dump_cache/upgrade.cc (right):
> 
> http://codereview.chromium.org/20134/diff/1/7#newcode20
> Line 20: class TempIOBuffer : public net::IOBuffer {
> nit: DependentIOBuffer?

[darin (slow to review), 2009-02-07 03:09:08]

http://codereview.chromium.org/20134/diff/1016/1019
File net/http/http_cache.cc (right):

http://codereview.chromium.org/20134/diff/1016/1019#newcode1105
Line 1105: scoped_refptr<FakeIOBuffer> data = new FakeIOBuffer(
looks like your changes to net/base/io_buffer.h are not included in this CL. 
are you sure that FakeIOBuffer is best?  that suggests that the IOBuffer is not
real, but it is real... what is different is how the memory is managed.  that's
why i suggested DependentIOBuffer since the name says that the IOBuffer is
dependent on something.  maybe WrappedIOBuffer would be more to your liking?

[rvargas (doing something else), 2009-02-07 03:36:27]

On 2009/02/07 03:09:08, darin wrote:
> http://codereview.chromium.org/20134/diff/1016/1019
> File net/http/http_cache.cc (right):
> 
> http://codereview.chromium.org/20134/diff/1016/1019#newcode1105
> Line 1105: scoped_refptr<FakeIOBuffer> data = new FakeIOBuffer(
> looks like your changes to net/base/io_buffer.h are not included in this CL. 

Uups... Sorry about that.

> are you sure that FakeIOBuffer is best?  that suggests that the IOBuffer is
not
> real, but it is real... what is different is how the memory is managed. 
that's
> why i suggested DependentIOBuffer since the name says that the IOBuffer is
> dependent on something.  maybe WrappedIOBuffer would be more to your liking?

Yeah, I'm not totally happy with Fake either. The thing is that it is real in
the sense that it has the interface, but it just violates the whole idea. That's
why I have the comment about not using it. I changed it to WrappedIOBuffer
although that doesn't imply anything wrong with it's use.

[darin (slow to review), 2009-02-10 04:40:26]

LGTM

http://codereview.chromium.org/20134/diff/1062/1080
File net/base/io_buffer.h (right):

http://codereview.chromium.org/20134/diff/1062/1080#newcode34
Line 34: // owns the underlying buffer. Please use this class only as a last
resort.
nit: s/owns/own/

maybe you could also more clearly state when it is OK to use this class.

[rvargas (doing something else), 2009-02-10 19:01:38]

On 2009/02/10 04:40:26, darin wrote:
> LGTM
> 
> http://codereview.chromium.org/20134/diff/1062/1080
> File net/base/io_buffer.h (right):
> 
> http://codereview.chromium.org/20134/diff/1062/1080#newcode34
> Line 34: // owns the underlying buffer. Please use this class only as a last
> resort.
> nit: s/owns/own/
> 
> maybe you could also more clearly state when it is OK to use this class.

Thanks. Added "A good example is the buffer for a synchronous operation, where
we can be sure that nobody is keeping an extra reference to this object so the
lifetime of the buffer can be completely managed by its intended owner."