base::Bind: Implement a 1-arity CancelableCallback and use this to implement

base::Bind: Implement a 1-arity CancelableCallback and use this to implement
net::CancelableCompletionClosure.

BUG=none
TEST=none

R=ajwong@chromium.org

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

[James Hawkins, 2011-11-29 23:55:44]

+michaeln for webkit/appcache.

[awong, 2011-11-30 00:09:19]

General API direction looks fine, but I have some comments and thoughts.

http://codereview.chromium.org/8662047/diff/1/base/cancelable_callback.h
File base/cancelable_callback.h (right):

http://codereview.chromium.org/8662047/diff/1/base/cancelable_callback.h#newc...
base/cancelable_callback.h:54: class CancelableCallback<R(void)> {
the return value should always be void shouldn't it?  Otherwise, what is the
value of Run() on a canceled callback?

http://codereview.chromium.org/8662047/diff/1/base/cancelable_callback.h#newc...
base/cancelable_callback.h:60: :
ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)),
Can we pull the common code out into a non-templated class?

You might even be able to use
http://en.wikipedia.org/wiki/Curiously_recurring_template_pattern to avoid the
virtual I expect though I haven't thought this through completely.

The less we have inside the templates, the less bloat we get in object files,
the happier thakis is. :)

The holy grail, btw, would be to somehow avoid the Bind() call.  I'm not sure
that's possible...but maybe it is if we threw the old API out the window and
required users to call Bind() themselves on the wrapped cancellation object?

Maybe?

[James Hawkins, 2011-11-30 00:27:51]

http://codereview.chromium.org/8662047/diff/1/base/cancelable_callback.h
File base/cancelable_callback.h (right):

http://codereview.chromium.org/8662047/diff/1/base/cancelable_callback.h#newc...
base/cancelable_callback.h:54: class CancelableCallback<R(void)> {
On 2011/11/30 00:09:19, awong wrote:
> the return value should always be void shouldn't it?  Otherwise, what is the
> value of Run() on a canceled callback?

Done.

http://codereview.chromium.org/8662047/diff/1/base/cancelable_callback.h#newc...
base/cancelable_callback.h:60: :
ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)),
On 2011/11/30 00:09:19, awong wrote:
> Can we pull the common code out into a non-templated class?
> 

Per off-line there's not any code that doesn't touch the template
specialization.

> You might even be able to use
> http://en.wikipedia.org/wiki/Curiously_recurring_template_pattern to avoid the
> virtual I expect though I haven't thought this through completely.
> 
> The less we have inside the templates, the less bloat we get in object files,
> the happier thakis is. :)
> 
> The holy grail, btw, would be to somehow avoid the Bind() call.  I'm not sure
> that's possible...but maybe it is if we threw the old API out the window and
> required users to call Bind() themselves on the wrapped cancellation object?
> 
> Maybe?

What are you envisioning this would look like? It looks like it would be
complicated for the user.

[awong, 2011-11-30 00:35:06]

http://codereview.chromium.org/8662047/diff/1/base/cancelable_callback.h
File base/cancelable_callback.h (right):

http://codereview.chromium.org/8662047/diff/1/base/cancelable_callback.h#newc...
base/cancelable_callback.h:60: :
ALLOW_THIS_IN_INITIALIZER_LIST(weak_factory_(this)),
On 2011/11/30 00:27:51, James Hawkins wrote:
> On 2011/11/30 00:09:19, awong wrote:
> > Can we pull the common code out into a non-templated class?
> > 
> 
> Per off-line there's not any code that doesn't touch the template
> specialization.
> 
> > You might even be able to use
> > http://en.wikipedia.org/wiki/Curiously_recurring_template_pattern to avoid
the
> > virtual I expect though I haven't thought this through completely.
> > 
> > The less we have inside the templates, the less bloat we get in object
files,
> > the happier thakis is. :)
> > 
> > The holy grail, btw, would be to somehow avoid the Bind() call.  I'm not
sure
> > that's possible...but maybe it is if we threw the old API out the window and
> > required users to call Bind() themselves on the wrapped cancellation object?
> > 
> > Maybe?
> 
> What are you envisioning this would look like? It looks like it would be
> complicated for the user.

I'm not sure.  I just would really like to avoid having bind.h in header files. 
But it might not be possible with a cancellation API.

[awong, 2011-11-30 00:52:55]

http://codereview.chromium.org/8662047/diff/2002/base/cancelable_callback.h
File base/cancelable_callback.h (right):

http://codereview.chromium.org/8662047/diff/2002/base/cancelable_callback.h#n...
base/cancelable_callback.h:45: #include "base/base_export.h"
alphabetical ordering.

http://codereview.chromium.org/8662047/diff/2002/base/cancelable_callback.h#n...
base/cancelable_callback.h:99: void Run() {
Can we renamed this function "Forward"?

http://codereview.chromium.org/8662047/diff/2002/base/cancelable_callback.h#n...
base/cancelable_callback.h:168: void Run(typename
internal::CallbackParamTraits<A1>::ForwardType a1) const {
Forward?

http://codereview.chromium.org/8662047/diff/2002/chrome/browser/renderer_host...
File chrome/browser/renderer_host/offline_resource_handler.cc (left):

http://codereview.chromium.org/8662047/diff/2002/chrome/browser/renderer_host...
chrome/browser/renderer_host/offline_resource_handler.cc:89:
CHECK(appcache_completion_callback_);
Do we no longer need an equivalent check?  Maybe
CHECK(appcache_completion_callback_.IsCancelled()) and then reset?

http://codereview.chromium.org/8662047/diff/2002/chrome/browser/renderer_host...
File chrome/browser/renderer_host/offline_resource_handler.cc (right):

http://codereview.chromium.org/8662047/diff/2002/chrome/browser/renderer_host...
chrome/browser/renderer_host/offline_resource_handler.cc:91: << " this=" <<
this;
You need a return; still.

http://codereview.chromium.org/8662047/diff/2002/chrome/browser/renderer_host...
chrome/browser/renderer_host/offline_resource_handler.cc:112:
DCHECK(appcache_completion_callback_.IsCancelled());
I think you still need to retain the manual AddRef() and Relaese() calls.  As
is, I think we introduce a refcount cycle on this since
appcache_completion_callback_ takes a reference onto this.

[michaeln, 2011-11-30 01:18:21]

http://codereview.chromium.org/8662047/diff/1/chrome/browser/renderer_host/of...
File chrome/browser/renderer_host/offline_resource_handler.cc (left):

http://codereview.chromium.org/8662047/diff/1/chrome/browser/renderer_host/of...
chrome/browser/renderer_host/offline_resource_handler.cc:95: return;
assuming DLOG() is a noop in release builds, it'd be safer to leave this return
here

http://codereview.chromium.org/8662047/diff/1/chrome/browser/renderer_host/of...
chrome/browser/renderer_host/offline_resource_handler.cc:115: AddRef();  // 
Balanced with OnCanHandleOfflineComplete
just checking... does the base::Bind do the AddRef() for us now?

http://codereview.chromium.org/8662047/diff/1/chrome/browser/renderer_host/of...
File chrome/browser/renderer_host/offline_resource_handler.cc (right):

http://codereview.chromium.org/8662047/diff/1/chrome/browser/renderer_host/of...
chrome/browser/renderer_host/offline_resource_handler.cc:82: Release();  //
Balanced with OnWillStart
looks like another balancing call to Release() that should be removed if the
'callback' holds the ref for us

http://codereview.chromium.org/8662047/diff/1/webkit/appcache/appcache_service.h
File webkit/appcache/appcache_service.h (right):

http://codereview.chromium.org/8662047/diff/1/webkit/appcache/appcache_servic...
webkit/appcache/appcache_service.h:151: class AsyncHelper;
maybe rename the existing class to OldAsyncHelper and old_pending_helpers_ and
let the new go unprefixed

[James Hawkins, 2011-11-30 01:44:58]

http://codereview.chromium.org/8662047/diff/1/chrome/browser/renderer_host/of...
File chrome/browser/renderer_host/offline_resource_handler.cc (left):

http://codereview.chromium.org/8662047/diff/1/chrome/browser/renderer_host/of...
chrome/browser/renderer_host/offline_resource_handler.cc:95: return;
On 2011/11/30 01:18:21, michaeln wrote:
> assuming DLOG() is a noop in release builds, it'd be safer to leave this
return
> here

Done.

http://codereview.chromium.org/8662047/diff/1/chrome/browser/renderer_host/of...
chrome/browser/renderer_host/offline_resource_handler.cc:115: AddRef();  // 
Balanced with OnCanHandleOfflineComplete
On 2011/11/30 01:18:21, michaeln wrote:
> just checking... does the base::Bind do the AddRef() for us now?

Yes.

http://codereview.chromium.org/8662047/diff/1/chrome/browser/renderer_host/of...
File chrome/browser/renderer_host/offline_resource_handler.cc (right):

http://codereview.chromium.org/8662047/diff/1/chrome/browser/renderer_host/of...
chrome/browser/renderer_host/offline_resource_handler.cc:82: Release();  //
Balanced with OnWillStart
On 2011/11/30 01:18:21, michaeln wrote:
> looks like another balancing call to Release() that should be removed if the
> 'callback' holds the ref for us

Done.

http://codereview.chromium.org/8662047/diff/1/webkit/appcache/appcache_service.h
File webkit/appcache/appcache_service.h (right):

http://codereview.chromium.org/8662047/diff/1/webkit/appcache/appcache_servic...
webkit/appcache/appcache_service.h:151: class AsyncHelper;
On 2011/11/30 01:18:21, michaeln wrote:
> maybe rename the existing class to OldAsyncHelper and old_pending_helpers_ and
> let the new go unprefixed

There are tons of existing usages of AsyncHelper, so I'm going to slowly convert
everything to NewAsyncHelper then get rid of the old one. I have a CL up that
does this in one blow.

http://codereview.chromium.org/8662047/diff/2002/base/cancelable_callback.h
File base/cancelable_callback.h (right):

http://codereview.chromium.org/8662047/diff/2002/base/cancelable_callback.h#n...
base/cancelable_callback.h:45: #include "base/base_export.h"
On 2011/11/30 00:52:55, awong wrote:
> alphabetical ordering.

Done.

http://codereview.chromium.org/8662047/diff/2002/base/cancelable_callback.h#n...
base/cancelable_callback.h:99: void Run() {
On 2011/11/30 00:52:55, awong wrote:
> Can we renamed this function "Forward"?

Done.

http://codereview.chromium.org/8662047/diff/2002/base/cancelable_callback.h#n...
base/cancelable_callback.h:168: void Run(typename
internal::CallbackParamTraits<A1>::ForwardType a1) const {
On 2011/11/30 00:52:55, awong wrote:
> Forward?

Done.

http://codereview.chromium.org/8662047/diff/2002/chrome/browser/renderer_host...
File chrome/browser/renderer_host/offline_resource_handler.cc (left):

http://codereview.chromium.org/8662047/diff/2002/chrome/browser/renderer_host...
chrome/browser/renderer_host/offline_resource_handler.cc:89:
CHECK(appcache_completion_callback_);
On 2011/11/30 00:52:55, awong wrote:
> Do we no longer need an equivalent check?  Maybe
> CHECK(appcache_completion_callback_.IsCancelled()) and then reset?

IMO the check is superfluous (we're in the callback afterall.  I'll add the
Cancel though.

http://codereview.chromium.org/8662047/diff/2002/chrome/browser/renderer_host...
File chrome/browser/renderer_host/offline_resource_handler.cc (right):

http://codereview.chromium.org/8662047/diff/2002/chrome/browser/renderer_host...
chrome/browser/renderer_host/offline_resource_handler.cc:91: << " this=" <<
this;
On 2011/11/30 00:52:55, awong wrote:
> You need a return; still.

Done.

http://codereview.chromium.org/8662047/diff/2002/chrome/browser/renderer_host...
chrome/browser/renderer_host/offline_resource_handler.cc:112:
DCHECK(appcache_completion_callback_.IsCancelled());
On 2011/11/30 00:52:55, awong wrote:
> I think you still need to retain the manual AddRef() and Relaese() calls.  As
> is, I think we introduce a refcount cycle on this since
> appcache_completion_callback_ takes a reference onto this.

Per-offline, made the callback own |this|.

[awong, 2011-11-30 01:55:39]

http://codereview.chromium.org/8662047/diff/8001/base/cancelable_callback.h
File base/cancelable_callback.h (right):

http://codereview.chromium.org/8662047/diff/8001/base/cancelable_callback.h#n...
base/cancelable_callback.h:71: callback_.Reset();
Should we also call forwarder_.Reset() here?

Otherwise we're going to keep its BindState<> alive unnecessarily.

http://codereview.chromium.org/8662047/diff/8001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/offline_resource_handler.cc (right):

http://codereview.chromium.org/8662047/diff/8001/chrome/browser/renderer_host...
chrome/browser/renderer_host/offline_resource_handler.cc:88:
appcache_completion_callback_.Cancel();
Isn't Reset() more appropriate here?

http://codereview.chromium.org/8662047/diff/8001/chrome/browser/renderer_host...
chrome/browser/renderer_host/offline_resource_handler.cc:117:
appcache_completion_callback_.Reset(
We should insert a comment either here, or up near the delcaration of
appcache_completion_callback_ that explains how this owns the object, but that
the circular reference is always broken since either we call cancel on it
somehow, or  appcache_service_->CanHandleMainResourceOffline() invokes the
actual callback, which then Resets()s the reference.  Thus, the circular
reference is guarantee transient.

Except write it in something in something other than albert-ese.

[awong, 2011-11-30 01:56:22]

http://codereview.chromium.org/8662047/diff/8001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/offline_resource_handler.cc (right):

http://codereview.chromium.org/8662047/diff/8001/chrome/browser/renderer_host...
chrome/browser/renderer_host/offline_resource_handler.cc:88:
appcache_completion_callback_.Cancel();
On 2011/11/30 01:55:39, awong wrote:
> Isn't Reset() more appropriate here?

Gha...ignore this comment.  I thought I had deleted it.

[James Hawkins, 2011-11-30 03:06:03]

http://codereview.chromium.org/8662047/diff/8001/base/cancelable_callback.h
File base/cancelable_callback.h (right):

http://codereview.chromium.org/8662047/diff/8001/base/cancelable_callback.h#n...
base/cancelable_callback.h:71: callback_.Reset();
On 2011/11/30 01:55:39, awong wrote:
> Should we also call forwarder_.Reset() here?
> 
> Otherwise we're going to keep its BindState<> alive unnecessarily.

Done.

http://codereview.chromium.org/8662047/diff/8001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/offline_resource_handler.cc (right):

http://codereview.chromium.org/8662047/diff/8001/chrome/browser/renderer_host...
chrome/browser/renderer_host/offline_resource_handler.cc:117:
appcache_completion_callback_.Reset(
On 2011/11/30 01:55:39, awong wrote:
> We should insert a comment either here, or up near the delcaration of
> appcache_completion_callback_ that explains how this owns the object, but that
> the circular reference is always broken since either we call cancel on it
> somehow, or  appcache_service_->CanHandleMainResourceOffline() invokes the
> actual callback, which then Resets()s the reference.  Thus, the circular
> reference is guarantee transient.
> 
> Except write it in something in something other than albert-ese.

Done.

[michaeln, 2011-11-30 20:04:22]

lgtm

http://codereview.chromium.org/8662047/diff/14021/base/cancelable_callback.h
File base/cancelable_callback.h (right):

http://codereview.chromium.org/8662047/diff/14021/base/cancelable_callback.h#...
base/cancelable_callback.h:12: // Calling Cancel() brings the object back to its
natural, default-constructed
ah... i see

http://codereview.chromium.org/8662047/diff/14021/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/offline_resource_handler.cc (right):

http://codereview.chromium.org/8662047/diff/14021/chrome/browser/renderer_hos...
chrome/browser/renderer_host/offline_resource_handler.cc:88:
appcache_completion_callback_.Cancel();
The call to Cancel() here is odd, your doing this to break reference cycle i
suppose, a small comment here would be good. Also is there is a different method
that would also work (like Reset() or Clear())?

[James Hawkins, 2011-11-30 20:07:12]

http://codereview.chromium.org/8662047/diff/14021/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/offline_resource_handler.cc (right):

http://codereview.chromium.org/8662047/diff/14021/chrome/browser/renderer_hos...
chrome/browser/renderer_host/offline_resource_handler.cc:88:
appcache_completion_callback_.Cancel();
On 2011/11/30 20:04:22, michaeln wrote:
> The call to Cancel() here is odd, your doing this to break reference cycle i
> suppose, a small comment here would be good. Also is there is a different
method
> that would also work (like Reset() or Clear())?

I'll add a comment.

There is a Reset(callback), but that is for setting the held callback to a
different callback.

[awong, 2011-11-30 20:28:16]

LGTM w/ nits

http://codereview.chromium.org/8662047/diff/14021/base/cancelable_callback.h
File base/cancelable_callback.h (right):

http://codereview.chromium.org/8662047/diff/14021/base/cancelable_callback.h#...
base/cancelable_callback.h:13: // state, i.e., callback() will return a null
callback.
callback() -> CancellableCallback::callback()

http://codereview.chromium.org/8662047/diff/14021/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/offline_resource_handler.cc (right):

http://codereview.chromium.org/8662047/diff/14021/chrome/browser/renderer_hos...
chrome/browser/renderer_host/offline_resource_handler.cc:88:
appcache_completion_callback_.Cancel();
On 2011/11/30 20:07:12, James Hawkins wrote:
> On 2011/11/30 20:04:22, michaeln wrote:
> > The call to Cancel() here is odd, your doing this to break reference cycle i
> > suppose, a small comment here would be good. Also is there is a different
> method
> > that would also work (like Reset() or Clear())?
> 
> I'll add a comment.
> 
> There is a Reset(callback), but that is for setting the held callback to a
> different callback.

This again makes me want to figure out how to rename these functions so
"Reset()". Maybe "Replace" versus CancelAndReset()?  I don't know.  Anyways,
another CL.

http://codereview.chromium.org/8662047/diff/14021/chrome/browser/renderer_hos...
chrome/browser/renderer_host/offline_resource_handler.cc:119: // circular
reference; however,
however, -> however, either

[James Hawkins, 2011-11-30 20:31:05]

http://codereview.chromium.org/8662047/diff/14021/base/cancelable_callback.h
File base/cancelable_callback.h (right):

http://codereview.chromium.org/8662047/diff/14021/base/cancelable_callback.h#...
base/cancelable_callback.h:13: // state, i.e., callback() will return a null
callback.
On 2011/11/30 20:28:17, awong wrote:
> callback() -> CancellableCallback::callback()

Done.

http://codereview.chromium.org/8662047/diff/14021/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/offline_resource_handler.cc (right):

http://codereview.chromium.org/8662047/diff/14021/chrome/browser/renderer_hos...
chrome/browser/renderer_host/offline_resource_handler.cc:119: // circular
reference; however,
On 2011/11/30 20:28:17, awong wrote:
> however, -> however, either 

Done.