Disk cache: First pass to make it possible to have...

Disk cache: First pass to make it possible to have
multiple instances of BackendImpl. We need multiple
objects to be able to support media files on the cache.

After this change, histograms will be the only thing that
get messed up by multiple disk caches.


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

[Alpha Left Google, 2009-03-25 20:28:53]

Thanks for the quick updates! Everything looks except for the use of
s_trace_buffer in net/disk_cache/trace.cc.

[Alpha Left Google, 2009-03-25 20:29:37]

Oh, I forgot to publish my comments.

http://codereview.chromium.org/49027/diff/1/19
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/49027/diff/1/19#newcode178
Line 178: cache->set_type(net::TEMP_MEDIA_CACHE);
Excellent! You already did what I want to do. :)

http://codereview.chromium.org/49027/diff/1/13
File net/disk_cache/trace.cc (right):

http://codereview.chromium.org/49027/diff/1/13#newcode58
Line 58: DCHECK(s_trace_buffer);
Having multiple instance of TraceObject to delete the global s_trace_buffer will
be a problem. If s_trace_buffer has to be shared by all instances we can make it
a singleton, contain it in TraceObject or do an if statement here.

http://codereview.chromium.org/49027/diff/1/13#newcode64
Line 64: DCHECK(s_trace_buffer);
We may hit the case when s_trace_buffer is destroyed by one BackendImpl instance
and others are trying to use it. Can we put an if statement here or use a
singleton?

http://codereview.chromium.org/49027/diff/1/13#newcode85
Line 85: DCHECK(s_trace_buffer);
Same question here for use of s_trace_buffer.

http://codereview.chromium.org/49027/diff/1/8
File net/disk_cache/trace.h (right):

http://codereview.chromium.org/49027/diff/1/8#newcode26
Line 26: friend class base::RefCounted<TraceObject>;
Can you add some comment here explaining the use of this class has to be
scoped_refptr<TraceObject>?

[rvargas (doing something else), 2009-03-25 21:56:12]

Thanks. The reason for making the TraceObject refcounted was to be able to share
it among all the callers and still be able to destroy it when not needed
anymore.

I think it works now.

On 2009/03/25 20:29:37, Alpha wrote:
> Oh, I forgot to publish my comments.
> 
> http://codereview.chromium.org/49027/diff/1/19
> File chrome/browser/net/chrome_url_request_context.cc (right):
> 
> http://codereview.chromium.org/49027/diff/1/19#newcode178
> Line 178: cache->set_type(net::TEMP_MEDIA_CACHE);
> Excellent! You already did what I want to do. :)
> 
> http://codereview.chromium.org/49027/diff/1/13
> File net/disk_cache/trace.cc (right):
> 
> http://codereview.chromium.org/49027/diff/1/13#newcode58
> Line 58: DCHECK(s_trace_buffer);
> Having multiple instance of TraceObject to delete the global s_trace_buffer
will
> be a problem. If s_trace_buffer has to be shared by all instances we can make
it
> a singleton, contain it in TraceObject or do an if statement here.
> 
> http://codereview.chromium.org/49027/diff/1/13#newcode64
> Line 64: DCHECK(s_trace_buffer);
> We may hit the case when s_trace_buffer is destroyed by one BackendImpl
instance
> and others are trying to use it. Can we put an if statement here or use a
> singleton?
> 
> http://codereview.chromium.org/49027/diff/1/13#newcode85
> Line 85: DCHECK(s_trace_buffer);
> Same question here for use of s_trace_buffer.
> 
> http://codereview.chromium.org/49027/diff/1/8
> File net/disk_cache/trace.h (right):
> 
> http://codereview.chromium.org/49027/diff/1/8#newcode26
> Line 26: friend class base::RefCounted<TraceObject>;
> Can you add some comment here explaining the use of this class has to be
> scoped_refptr<TraceObject>?

[Alpha Left Google, 2009-03-25 22:30:08]

Thanks. LGTM.