base::Bind: Implement CancelableCallback to replace CancelableTaske.

base/cancelable_callback.h

+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_CANCELABLE_CALLBACK_H_
+#define BASE_CANCELABLE_CALLBACK_H_

[groby-ooo-7-16, 2011/11/23 02:14:12]

Can we get a comment explaining usage, same as for the other callback
functions/classes?

[James Hawkins, 2011/11/23 03:59:08]

I could not find a succinct example to write for usage.  I'm open to
suggestions.

[awong, 2011/11/23 19:58:23]

I think we should start with a syntax example showing creating a Cancelable
Callback that's stored as a class member variable, a PostTask,and then a Cancel
and then explain the sequence of events.

We should make sure to explain the threading semantics in that a
CancelableCallback() has to be created, used, posted to, and canceled all on the
same thread.

Lastly, we should specify the lifetime of the wrapped callback (more below in
later comment).

+#pragma once
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/callback.h"
+#include "base/compiler_specific.h"
+#include "base/memory/singleton.h"
+#include "base/memory/weak_ptr.h"
+
+namespace base {
+
+// Wrapper used to cancel a callback.
+class CancelableCallback {

[awong, 2011/11/23 02:30:21]

Part of me wants to make this subsume content/brownser/cancelable_request.h

I think this requires 2 things:
  1) templating on T
  2) adding a "notifications" interface for events like WillExecute, DidExecute,
and Canceled.

Would this be too hard to weave into this?

[James Hawkins, 2011/11/23 03:59:08]

I'm not sure, but looking at CancelableRequest, it would take a lot of work. I'm
more than willing to look into making that happen post-commit of this CL.

[awong, 2011/11/23 19:58:23]

Happy to have most of the work in another CL, but before committing this one,
I'd like to have some confidence that this API doesn't somehow paint us into a
corner for picking up the features of CancelableRequest. Any thoughts on that?

The alternative would be to need to do a unification pass if the APIs somehow
diverged (similar to how I need to go convert IgnoreReturn -> IgnoreResult)

[James Hawkins, 2011/11/23 22:12:52]

Hmm, not really. I think they're very similar as is and I don't expect anyone to
make major changes to CancelableRequest anytime soon.
This would be fine.

+ public:
+  // |callback| must not be null.
+  CancelableCallback(const base::Closure callback);

[groby-ooo-7-16, 2011/11/23 02:14:12]

We probably need an Assign() or Reset() method - existing usage has member
variables of type CancelableTask, and we assign callbacks to it as necessary.

This is DISALLOW_COPY_AND_ASSIGN, so that won't work, but I don't believe we can
simply construct all uses via ctor. (Unless we store CancellableCallbacks as
scoped_ptr. Is that the plan?)

[binji, 2011/11/23 02:21:44]

const base::Closure& callback?

[awong, 2011/11/23 02:30:21]

explicit and const&

[James Hawkins, 2011/11/23 03:59:08]

Done.

[James Hawkins, 2011/11/23 03:59:08]

Done.

[James Hawkins, 2011/11/23 03:59:08]

Done.

+
+  // Cancels the stored callback.
+  void Cancel();

[awong, 2011/11/23 02:30:21]

What happens if this class is canceled multiple times?

CancelableCallback cb(Bind(&Foo));

ml->PostTask(cb.callback());
cb.Cancel();
ml->PostTask(cb.callback());
ml->PostTask(cb.callback());
cb.Cancel();

?

Right now it behaves as a kind of "resetable factory."  I wonder if it'd be
better to make it so after the first Cancel(), all other calls to callback()
return a Bind(&DoNothing);  Then in Cancel(), you can immediately drop the
reference to callback_ and forwarder_.

[James Hawkins, 2011/11/23 03:59:08]

Foo is never run in the above code.  I added a unit test that emulates what
you've written.
We could do this, but what is the advantage?  It seems the current code is
simpler than what this would be.

[awong, 2011/11/23 19:58:23]

Sorry, I wasn't clear on the sequencing.  Assume you're on a message loop that
has 6 tasks enqueued with the operations as follows:

task 1:
  ml->PostTask(cb.callback());  // A
  cb.Cancel();  // i

task 2:
  // A executes in canceled state and does nothing due to i

task 3:
  ml->PostTask(cb.callback());  // B
  ml->PostTask(cb.callback());  // C

task 4:
  // A executes in canceled state and does nothing due to i
  
task 5:
  cb.Cancel();  // ii

task 6:
  // C executes in canceled state and does nothing due to i

Net result (ii) is ineffective.


And rereading your code, I think the implementation already does what's above. 
But it'd be nice to have a unittest actually enforce this.  (The alternate
behavior would be to have B and C contingent on (ii) being called which I think
is a bad semantic.)

[James Hawkins, 2011/11/23 22:12:52]

Can you comment what needs to be changed in the unittest I added?  The new
format you wrote isn't clear to me.

+
+  // Returns the wrapped callback.
+  const base::Closure& callback() const;
+
+ private:
+  // Closure that runs the stored callback.
+  void RunCallback();
+
+  // Used to ensure RunCallback() is not run when this object is destroyed.
+  base::WeakPtrFactory<CancelableCallback> weak_factory_;
+
+  // The wrapper closure.
+  base::Closure forwarder_;
+
+  // The stored closure that may be cancelled.
+  base::Closure callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(CancelableCallback);
+};
+
+}  // namespace base
+
+#endif  // BASE_CANCELABLE_CALLBACK_H_