base::Bind: Convert disk_cache_based_ssl_host_info.

base::Bind: Convert disk_cache_based_ssl_host_info.

BUG=none
TEST=none

R=csilv@chromium.org

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

[James Hawkins, 2011-12-05 04:23:07]

The x_sync bot redness is unrelated to this change (verified on the tryserver
waterfall).

[csilv, 2011-12-06 00:56:08]

http://codereview.chromium.org/8794003/diff/3001/net/base/test_completion_cal...
File net/base/test_completion_callback.h (right):

http://codereview.chromium.org/8794003/diff/3001/net/base/test_completion_cal...
net/base/test_completion_callback.h:43: };
This is a pretty cool approach, although when we remove
TestOldCompletionCallback you'll probably get rid of the base-class, thus
removing the benefit of adding it.  No change required, just an observation.

http://codereview.chromium.org/8794003/diff/3001/net/disk_cache/disk_cache.h
File net/disk_cache/disk_cache.h (right):

http://codereview.chromium.org/8794003/diff/3001/net/disk_cache/disk_cache.h#...
net/disk_cache/disk_cache.h:36: typedef net::OldCompletionCallback
OldCompletionCallback;
(comment-only)  I'm trying to decide if this is cool or lame.  I'm not sure. 
Leaning towards lame.

http://codereview.chromium.org/8794003/diff/3001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

http://codereview.chromium.org/8794003/diff/3001/net/http/http_cache.cc#newco...
net/http/http_cache.cc:158: backend_(backend) {}
I don't see where this constructor is used, can you clarify?  If this
constructor is not needed, then perhaps WorkItem doesn't need to support
OldCompletionCallback at all.

[James Hawkins, 2011-12-06 17:25:12]

http://codereview.chromium.org/8794003/diff/3001/net/base/test_completion_cal...
File net/base/test_completion_callback.h (right):

http://codereview.chromium.org/8794003/diff/3001/net/base/test_completion_cal...
net/base/test_completion_callback.h:43: };
On 2011/12/06 00:56:08, csilv wrote:
> This is a pretty cool approach, although when we remove
> TestOldCompletionCallback you'll probably get rid of the base-class, thus
> removing the benefit of adding it.  No change required, just an observation.

I don't think so; see the use of this base class to specialize a
CompletionCallback in this CL (http_cache_unittest.cc).

http://codereview.chromium.org/8794003/diff/3001/net/disk_cache/disk_cache.h
File net/disk_cache/disk_cache.h (right):

http://codereview.chromium.org/8794003/diff/3001/net/disk_cache/disk_cache.h#...
net/disk_cache/disk_cache.h:36: typedef net::OldCompletionCallback
OldCompletionCallback;
On 2011/12/06 00:56:08, csilv wrote:
> (comment-only)  I'm trying to decide if this is cool or lame.  I'm not sure. 
> Leaning towards lame.

It's lame, but removing it now would be a mess.

http://codereview.chromium.org/8794003/diff/3001/net/http/http_cache.cc
File net/http/http_cache.cc (right):

http://codereview.chromium.org/8794003/diff/3001/net/http/http_cache.cc#newco...
net/http/http_cache.cc:158: backend_(backend) {}
On 2011/12/06 00:56:08, csilv wrote:
> I don't see where this constructor is used, can you clarify?  If this
> constructor is not needed, then perhaps WorkItem doesn't need to support
> OldCompletionCallback at all.

Done.

[csilv, 2011-12-06 18:29:17]

lgtm

[awong, 2011-12-08 22:35:09]

doesn't fix the asan issue, but here's some comments.

http://codereview.chromium.org/8794003/diff/7001/net/base/test_completion_cal...
File net/base/test_completion_callback.h (right):

http://codereview.chromium.org/8794003/diff/7001/net/base/test_completion_cal...
net/base/test_completion_callback.h:35: TestCompletionCallbackBase();
Declare an out-of-line destructor.

http://codereview.chromium.org/8794003/diff/7001/net/base/test_completion_cal...
net/base/test_completion_callback.h:48: TestOldCompletionCallback() {};
Should out-of-line these constructors/destructors.

http://codereview.chromium.org/8794003/diff/7001/net/base/test_completion_cal...
net/base/test_completion_callback.h:62: ~TestCompletionCallback();
virtual.

http://codereview.chromium.org/8794003/diff/7001/net/disk_cache/mem_entry_imp...
File net/disk_cache/mem_entry_impl.cc (right):

http://codereview.chromium.org/8794003/diff/7001/net/disk_cache/mem_entry_imp...
net/disk_cache/mem_entry_impl.cc:190: const net::CompletionCallback&
completion_callback) {
Should just call the other one with NULL since there's no completion.

http://codereview.chromium.org/8794003/diff/7001/net/disk_cache/mem_entry_imp...
net/disk_cache/mem_entry_impl.cc:229: const net::CompletionCallback&
completion_callback, bool truncate) {
Same comment as earlier.

http://codereview.chromium.org/8794003/diff/7001/net/http/disk_cache_based_ss...
File net/http/disk_cache_based_ssl_host_info.cc (left):

http://codereview.chromium.org/8794003/diff/7001/net/http/disk_cache_based_ss...
net/http/disk_cache_based_ssl_host_info.cc:96: if (!IsCallbackPending())
This is strange...previously did we just leak?

http://codereview.chromium.org/8794003/diff/7001/net/http/disk_cache_based_ss...
File net/http/disk_cache_based_ssl_host_info.cc (right):

http://codereview.chromium.org/8794003/diff/7001/net/http/disk_cache_based_ss...
net/http/disk_cache_based_ssl_host_info.cc:16:
DiskCacheBasedSSLHostInfo::CallbackImpl::CallbackImpl(
Can we delete CallbackImpl now?

http://codereview.chromium.org/8794003/diff/7001/net/http/mock_http_cache.cc
File net/http/mock_http_cache.cc (right):

http://codereview.chromium.org/8794003/diff/7001/net/http/mock_http_cache.cc#...
net/http/mock_http_cache.cc:14: class OldCallbackRunner : public Task {
Don't need this either I don't hitnk.

Bind(&OldCompletionCallback::Run<int>, base::Unretained(old_cb))

http://codereview.chromium.org/8794003/diff/7001/net/http/mock_http_cache.cc#...
net/http/mock_http_cache.cc:30: const net::CompletionCallback& callback, int
result) {
Don't need htis?

http://codereview.chromium.org/8794003/diff/7001/net/http/mock_http_cache.cc#...
net/http/mock_http_cache.cc:656: FROM_HERE,
base::Bind(&CompletionCallbackRunner, callback, result));
I think you can just do Bind(callback, result) and lose
CompletionCalblackRunner.