Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8673008:
base::Bind: Implement CancelableCallback to replace CancelableTaske. (Closed)
|
Created:
9 years, 1 month ago by James Hawkins Modified:
9 years, 1 month ago Reviewers:
willchan no longer on Chromium, groby-ooo-7-16, awong, csilv, darin (slow to review), akalin CC:
chromium-reviews, Paweł Hajdan Jr., brettw-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionbase::Bind: Implement CancelableCallback to replace CancelableTask.
Designed by groby,binji,csilv,jhawkins.
BUG=96749
TEST=CancelableCallbackTest.*
R=ajwong@chromium.org,darin@chromium.org
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111493
Patch Set 1 #
Total comments: 24
Patch Set 2 : Review fixes. #Patch Set 3 : AppCacheGroup conversion. #Patch Set 4 : Out-of-line destructor. #Patch Set 5 : Test build fix. #
Total comments: 29
Patch Set 6 : More reviewer fixes. #
Total comments: 20
Patch Set 7 : More fixes. #Patch Set 8 : Moar. #
Total comments: 13
Patch Set 9 : Moardor. #Patch Set 10 : Test build fix. #
Messages
Total messages: 26 (0 generated)
http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.cc File base/cancelable_callback.cc (right): http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.cc#new... base/cancelable_callback.cc:28: } nit: } // namespace base http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.h File base/cancelable_callback.h (right): http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.h#newc... base/cancelable_callback.h:6: #define BASE_CANCELABLE_CALLBACK_H_ Can we get a comment explaining usage, same as for the other callback functions/classes? http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.h#newc... base/cancelable_callback.h:22: CancelableCallback(const base::Closure callback); 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?) http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback_unitte... File base/cancelable_callback_unittest.cc (right): http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback_unitte... base/cancelable_callback_unittest.cc:56: TEST(CancelableCallbackTest, Destruction) { CallbackCanceledOnDestruction would probably describe this better. http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback_unitte... base/cancelable_callback_unittest.cc:71: } Can we add an example that uses PostTask? (Yes, it's fairly straightforward to use. Still, it'd be nice)
http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.cc File base/cancelable_callback.cc (right): http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.cc#new... base/cancelable_callback.cc:13: callback_(callback) { check for !callback.is_null? http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.h File base/cancelable_callback.h (right): http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.h#newc... base/cancelable_callback.h:22: CancelableCallback(const base::Closure callback); const base::Closure& callback?
[ +willchan, akalin ] Thanks for taking a shot at this! Added some high level comments. I have lower level comments, but want to make sure the API stablized out first. http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.h File base/cancelable_callback.h (right): http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.h#newc... base/cancelable_callback.h:19: class CancelableCallback { 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? http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.h#newc... base/cancelable_callback.h:22: CancelableCallback(const base::Closure callback); explicit and const& http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.h#newc... base/cancelable_callback.h:25: void Cancel(); 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_.
Another thing that would be helpful is to link to some examples of where this is used? On Tue, Nov 22, 2011 at 6:30 PM, <ajwong@chromium.org> wrote: > [ +willchan, akalin ] > > Thanks for taking a shot at this! > > Added some high level comments. > > I have lower level comments, but want to make sure the API stablized out > first. > > > > http://codereview.chromium.**org/8673008/diff/1/base/** > cancelable_callback.h<http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.h> > File base/cancelable_callback.h (right): > > http://codereview.chromium.**org/8673008/diff/1/base/** > cancelable_callback.h#**newcode19<http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.h#newcode19> > base/cancelable_callback.h:19: class CancelableCallback { > 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? > > > http://codereview.chromium.**org/8673008/diff/1/base/** > cancelable_callback.h#**newcode22<http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.h#newcode22> > base/cancelable_callback.h:22: CancelableCallback(const base::Closure > callback); > explicit and const& > > http://codereview.chromium.**org/8673008/diff/1/base/** > cancelable_callback.h#**newcode25<http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.h#newcode25> > base/cancelable_callback.h:25: void Cancel(); > 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_. > > http://codereview.chromium.**org/8673008/<http://codereview.chromium.org/8673... >
http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.cc File base/cancelable_callback.cc (right): http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.cc#new... base/cancelable_callback.cc:13: callback_(callback) { On 2011/11/23 02:21:44, binji wrote: > check for !callback.is_null? Done. http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.h File base/cancelable_callback.h (right): http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.h#newc... base/cancelable_callback.h:6: #define BASE_CANCELABLE_CALLBACK_H_ On 2011/11/23 02:14:12, groby wrote: > Can we get a comment explaining usage, same as for the other callback > functions/classes? I could not find a succinct example to write for usage. I'm open to suggestions. http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.h#newc... base/cancelable_callback.h:19: class CancelableCallback { On 2011/11/23 02:30:21, awong wrote: > 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? 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. http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.h#newc... base/cancelable_callback.h:22: CancelableCallback(const base::Closure callback); On 2011/11/23 02:21:44, binji wrote: > const base::Closure& callback? Done. http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.h#newc... base/cancelable_callback.h:22: CancelableCallback(const base::Closure callback); On 2011/11/23 02:30:21, awong wrote: > explicit and const& Done. http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.h#newc... base/cancelable_callback.h:22: CancelableCallback(const base::Closure callback); On 2011/11/23 02:14:12, groby wrote: > 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?) Done. http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.h#newc... base/cancelable_callback.h:25: void Cancel(); On 2011/11/23 02:30:21, awong wrote: > 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(); > > ? > Foo is never run in the above code. I added a unit test that emulates what you've written. > 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_. We could do this, but what is the advantage? It seems the current code is simpler than what this would be. http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback_unitte... File base/cancelable_callback_unittest.cc (right): http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback_unitte... base/cancelable_callback_unittest.cc:56: TEST(CancelableCallbackTest, Destruction) { On 2011/11/23 02:14:12, groby wrote: > CallbackCanceledOnDestruction would probably describe this better. Done. http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback_unitte... base/cancelable_callback_unittest.cc:71: } On 2011/11/23 02:14:12, groby wrote: > Can we add an example that uses PostTask? (Yes, it's fairly straightforward to > use. Still, it'd be nice) Done.
On 2011/11/23 02:37:39, awong wrote: > Another thing that would be helpful is to link to some examples of where > this is used? > I converted AppCacheGroup; let me know if that's enough of an example.
LGTM http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback_uni... File base/cancelable_callback_unittest.cc (right): http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback_uni... base/cancelable_callback_unittest.cc:50: cancelable.Cancel(); I suspect it would be interesting for people to know what happens if the second cancel _isn't_ there.
http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback_uni... File base/cancelable_callback_unittest.cc (right): http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback_uni... base/cancelable_callback_unittest.cc:50: cancelable.Cancel(); On 2011/11/23 19:18:57, groby wrote: > I suspect it would be interesting for people to know what happens if the second > cancel _isn't_ there. How so?
http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback.cc File base/cancelable_callback.cc (right): http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback.cc#... base/cancelable_callback.cc:5: #include "base/cancelable_callback.h" add includes for base/bind.h and base/compiler_specific.h. http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback.h File base/cancelable_callback.h (right): http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback.h#n... base/cancelable_callback.h:8: Consider adding some overview documentation here about what this class is for, with an example snippit, similar to what is currently in callback.h. http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback.h#n... base/cancelable_callback.h:13: #include "base/memory/singleton.h" i dont think bind.h, bind_helpers.h, compiler_specific.h, and singleton.h are used in this header. http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback.h#n... base/cancelable_callback.h:19: class CancelableCallback { i think you may need to add BASE_EXPORT (along with include for base/base_export.h) http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback.h#n... base/cancelable_callback.h:23: // |callback| must not be null. Would it be better to say "must not be a null callback"? Not a big deal, just a thought. http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback_uni... File base/cancelable_callback_unittest.cc (right): http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback_uni... base/cancelable_callback_unittest.cc:5: #include "base/cancelable_callback.h" add includes for base/bind.h, base/bind_helpers.h
http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback_uni... File base/cancelable_callback_unittest.cc (right): http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback_uni... base/cancelable_callback_unittest.cc:50: cancelable.Cancel(); Will callback2/3 be running without it? (It won't, if I read the code correctly - but it'd be nice to point that out) The second Cancel() in this test seems to imply that it still will be run. On 2011/11/23 19:37:10, James Hawkins wrote: > On 2011/11/23 19:18:57, groby wrote: > > I suspect it would be interesting for people to know what happens if the > second > > cancel _isn't_ there. > > How so?
http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.h File base/cancelable_callback.h (right): http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.h#newc... base/cancelable_callback.h:6: #define BASE_CANCELABLE_CALLBACK_H_ On 2011/11/23 03:59:08, James Hawkins wrote: > On 2011/11/23 02:14:12, groby wrote: > > Can we get a comment explaining usage, same as for the other callback > > functions/classes? > > I could not find a succinct example to write for usage. I'm open to > suggestions. 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). http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.h#newc... base/cancelable_callback.h:19: class CancelableCallback { On 2011/11/23 03:59:08, James Hawkins wrote: > On 2011/11/23 02:30:21, awong wrote: > > 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? > > 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. 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) http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.h#newc... base/cancelable_callback.h:25: void Cancel(); On 2011/11/23 03:59:08, James Hawkins wrote: > On 2011/11/23 02:30:21, awong wrote: > > 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(); > > > > ? > > 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.) > > Foo is never run in the above code. I added a unit test that emulates what > you've written. > > > 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_. > > We could do this, but what is the advantage? It seems the current code is > simpler than what this would be. http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback.cc File base/cancelable_callback.cc (right): http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback.cc#... base/cancelable_callback.cc:26: callback_.Reset(); We should have a unittest assert that the wrapped callback (specifically, it's bound arguments) are dropped when we Cancel(), or Reset() the CancelableCallback. Being able to depend on this contract seems important. http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback.h File base/cancelable_callback.h (right): http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback.h#n... base/cancelable_callback.h:32: // be null. Should state what happens to outstanding callbacks, and to any previously wrapped callbacks. http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback.h#n... base/cancelable_callback.h:35: // Returns the wrapped callback. Returns a callback that can disabled by calling Cancel(). http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback_uni... File base/cancelable_callback_unittest.cc (right): http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback_uni... base/cancelable_callback_unittest.cc:12: class Counter { Why use a class instead of a bare function like: void Increment(int* count) { (*count)++; } ? http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback_uni... base/cancelable_callback_unittest.cc:23: TEST(CancelableCallbackTest, Cancel) { For things related to bind/callback, I've been trying to use a convention where we add a comment above each test which lists in bullets the specific assertions the test tries to cover. Can we do something similar for these tests? For instance, the properties we're asserting here are: - Callback can be run multiple times. - After Cancel(), Run() completes, but has no effect. http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback_uni... base/cancelable_callback_unittest.cc:63: TEST(CancelableCallbackTest, NArity) { What's NArity mean in this context? If you're testing that you can support callbacks that have bound arguments, I would actually remove this test since it's double-covering the point of bind_unittests. http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback_uni... base/cancelable_callback_unittest.cc:110: base::Unretained(&counter), 3)); Also EXPECT_TRUE(cancelable.is_null())?
http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.h File base/cancelable_callback.h (right): http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.h#newc... base/cancelable_callback.h:19: class CancelableCallback { On 2011/11/23 19:58:23, awong wrote: > On 2011/11/23 03:59:08, James Hawkins wrote: > > On 2011/11/23 02:30:21, awong wrote: > > > 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? > > > > 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. > > 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? > 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. > 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) This would be fine. http://codereview.chromium.org/8673008/diff/1/base/cancelable_callback.h#newc... base/cancelable_callback.h:25: void Cancel(); On 2011/11/23 19:58:23, awong wrote: > On 2011/11/23 03:59:08, James Hawkins wrote: > > On 2011/11/23 02:30:21, awong wrote: > > > 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(); > > > > > > ? > > > > > 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.) > > > > > > Foo is never run in the above code. I added a unit test that emulates what > > you've written. > > > > > 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_. > > > > We could do this, but what is the advantage? It seems the current code is > > simpler than what this would be. > Can you comment what needs to be changed in the unittest I added? The new format you wrote isn't clear to me. http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback.cc File base/cancelable_callback.cc (right): http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback.cc#... base/cancelable_callback.cc:5: #include "base/cancelable_callback.h" On 2011/11/23 19:37:30, csilv wrote: > add includes for base/bind.h and base/compiler_specific.h. Done. http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback.cc#... base/cancelable_callback.cc:26: callback_.Reset(); On 2011/11/23 19:58:23, awong wrote: > We should have a unittest assert that the wrapped callback (specifically, it's > bound arguments) are dropped when we Cancel(), or Reset() the > CancelableCallback. > > Being able to depend on this contract seems important. Done. http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback.h File base/cancelable_callback.h (right): http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback.h#n... base/cancelable_callback.h:13: #include "base/memory/singleton.h" On 2011/11/23 19:37:30, csilv wrote: > i dont think bind.h, bind_helpers.h, compiler_specific.h, and singleton.h are > used in this header. Done. http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback.h#n... base/cancelable_callback.h:19: class CancelableCallback { On 2011/11/23 19:37:30, csilv wrote: > i think you may need to add BASE_EXPORT (along with include for > base/base_export.h) Done. http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback.h#n... base/cancelable_callback.h:23: // |callback| must not be null. On 2011/11/23 19:37:30, csilv wrote: > Would it be better to say "must not be a null callback"? Not a big deal, just a > thought. "|callback| must not be a null callback" sounds weird, and there's precedence for using "must not be null" (note the lower-case null.) http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback.h#n... base/cancelable_callback.h:32: // be null. On 2011/11/23 19:58:23, awong wrote: > Should state what happens to outstanding callbacks, and to any previously > wrapped callbacks. Done. http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback.h#n... base/cancelable_callback.h:35: // Returns the wrapped callback. On 2011/11/23 19:58:23, awong wrote: > Returns a callback that can disabled by calling Cancel(). Done. http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback_uni... File base/cancelable_callback_unittest.cc (right): http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback_uni... base/cancelable_callback_unittest.cc:5: #include "base/cancelable_callback.h" On 2011/11/23 19:37:30, csilv wrote: > add includes for base/bind.h, base/bind_helpers.h Done. http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback_uni... base/cancelable_callback_unittest.cc:12: class Counter { On 2011/11/23 19:58:23, awong wrote: > Why use a class instead of a bare function like: > > void Increment(int* count) { (*count)++; } > > ? This is an evolved version of a class I copied from a similar test. I'll simplify it to your suggestion. http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback_uni... base/cancelable_callback_unittest.cc:23: TEST(CancelableCallbackTest, Cancel) { On 2011/11/23 19:58:23, awong wrote: > For things related to bind/callback, I've been trying to use a convention where > we add a comment above each test which lists in bullets the specific assertions > the test tries to cover. Can we do something similar for these tests? > > For instance, the properties we're asserting here are: > - Callback can be run multiple times. > - After Cancel(), Run() completes, but has no effect. Done. http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback_uni... base/cancelable_callback_unittest.cc:50: cancelable.Cancel(); On 2011/11/23 19:43:14, groby wrote: > Will callback2/3 be running without it? (It won't, if I read the code correctly > - but it'd be nice to point that out) > > The second Cancel() in this test seems to imply that it still will be run. > > On 2011/11/23 19:37:10, James Hawkins wrote: > > On 2011/11/23 19:18:57, groby wrote: > > > I suspect it would be interesting for people to know what happens if the > > second > > > cancel _isn't_ there. > > > > How so? > I've rearranged this, per-offline. http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback_uni... base/cancelable_callback_unittest.cc:63: TEST(CancelableCallbackTest, NArity) { On 2011/11/23 19:58:23, awong wrote: > What's NArity mean in this context? > > If you're testing that you can support callbacks that have bound arguments, I > would actually remove this test since it's double-covering the point of > bind_unittests. Done. http://codereview.chromium.org/8673008/diff/8001/base/cancelable_callback_uni... base/cancelable_callback_unittest.cc:110: base::Unretained(&counter), 3)); On 2011/11/23 19:58:23, awong wrote: > Also EXPECT_TRUE(cancelable.is_null())? |cancelable| is not null at this point. I've added more EXPECTs/ASSERTs in this test to elucidate the details.
lgtm
commit nits. http://codereview.chromium.org/8673008/diff/15001/base/cancelable_callback.cc File base/cancelable_callback.cc (right): http://codereview.chromium.org/8673008/diff/15001/base/cancelable_callback.cc... base/cancelable_callback.cc:60: } // namespace bind http://codereview.chromium.org/8673008/diff/15001/base/cancelable_callback.h File base/cancelable_callback.h (right): http://codereview.chromium.org/8673008/diff/15001/base/cancelable_callback.h#... base/cancelable_callback.h:37: // }; This use case looks exactly like what you'd use a WeakPtrFactory for. Can we modify to show creating callbacks that we might cancel independent of one another? Also, the end of this comment should include a disucssion of the threading restrictions and lifetime guarantees of the target callback. http://codereview.chromium.org/8673008/diff/15001/base/cancelable_callback.h#... base/cancelable_callback.h:50: // Wrapper used to cancel a callback. nit: The file comment already covers this. Remove this comment? http://codereview.chromium.org/8673008/diff/15001/base/cancelable_callback.h#... base/cancelable_callback.h:60: // Cancels the stored callback. This should also specify that it's going to drop references to the wrapped callback. Both the Cancel() and Reset() should have strongly published semantics on when the resources in the wrapped callback are derefed. http://codereview.chromium.org/8673008/diff/15001/base/cancelable_callback.h#... base/cancelable_callback.h:64: // be null. Outstanding and any previously wrapped callbacks are reset. This this be "are reset" or "are canceled?" http://codereview.chromium.org/8673008/diff/15001/base/cancelable_callback.h#... base/cancelable_callback.h:70: // Returns true if the wrapped callback is null. This isn't true is it? It's if the CancelableCallback itself has been Reset w/o a callback? All constructors/mutators assert the wrapped callback is not null. http://codereview.chromium.org/8673008/diff/15001/base/cancelable_callback.h#... base/cancelable_callback.h:74: // Closure that runs the stored callback. Comment is too close to function name. remove? http://codereview.chromium.org/8673008/diff/15001/base/cancelable_callback.h#... base/cancelable_callback.h:78: // |weak_factory_|. nit: How about InitializeForwarder instead of BindForwarder? http://codereview.chromium.org/8673008/diff/15001/base/cancelable_callback_un... File base/cancelable_callback_unittest.cc (right): http://codereview.chromium.org/8673008/diff/15001/base/cancelable_callback_un... base/cancelable_callback_unittest.cc:25: // * Callback can be run multiple times. Formatting Nit: I've been using a "-" instead of a "*" for these and starting with a single, short, summary sentence for the test. Example: http://www.google.com/codesearch#OAMlx_jo-ck/src/base/bind_unittest.cc&exact_... http://codereview.chromium.org/8673008/diff/15001/base/cancelable_callback_un... base/cancelable_callback_unittest.cc:62: cancelable.Cancel(); Add comment saying this Cancel() should have no effect, but should still be callable.
Will, Darin, This is going to need a base OWNERS review. I think it's pretty close. Any chance either of you can take a crack at it?
I was going to let you do it first :) Do you think it's ready? I don't anticipate having major issues. On Wed, Nov 23, 2011 at 3:41 PM, <ajwong@chromium.org> wrote: > Will, Darin, > > This is going to need a base OWNERS review. I think it's pretty close. > Any > chance either of you can take a crack at it? > > http://codereview.chromium.**org/8673008/<http://codereview.chromium.org/8673... >
Wow...fast response. Just wanted it on the radar for today. :) All that's left are comment nits. I expect the next patch should be good. Will ping again then. -Albert On Wed, Nov 23, 2011 at 3:42 PM, William Chan (陈智昌) <willchan@chromium.org>wrote: > I was going to let you do it first :) Do you think it's ready? I don't > anticipate having major issues. > > > On Wed, Nov 23, 2011 at 3:41 PM, <ajwong@chromium.org> wrote: > >> Will, Darin, >> >> This is going to need a base OWNERS review. I think it's pretty close. >> Any >> chance either of you can take a crack at it? >> >> http://codereview.chromium.**org/8673008/<http://codereview.chromium.org/8673... >> > >
http://codereview.chromium.org/8673008/diff/15001/base/cancelable_callback.cc File base/cancelable_callback.cc (right): http://codereview.chromium.org/8673008/diff/15001/base/cancelable_callback.cc... base/cancelable_callback.cc:60: } On 2011/11/23 23:41:07, awong wrote: > // namespace bind Done. http://codereview.chromium.org/8673008/diff/15001/base/cancelable_callback.h File base/cancelable_callback.h (right): http://codereview.chromium.org/8673008/diff/15001/base/cancelable_callback.h#... base/cancelable_callback.h:50: // Wrapper used to cancel a callback. On 2011/11/23 23:41:07, awong wrote: > nit: The file comment already covers this. Remove this comment? Done. http://codereview.chromium.org/8673008/diff/15001/base/cancelable_callback.h#... base/cancelable_callback.h:60: // Cancels the stored callback. On 2011/11/23 23:41:07, awong wrote: > This should also specify that it's going to drop references to the wrapped > callback. > Done. > Both the Cancel() and Reset() should have strongly published semantics on when > the resources in the wrapped callback are derefed. Strongly published as in I need to add more comments? http://codereview.chromium.org/8673008/diff/15001/base/cancelable_callback.h#... base/cancelable_callback.h:64: // be null. Outstanding and any previously wrapped callbacks are reset. On 2011/11/23 23:41:07, awong wrote: > This this be "are reset" or "are canceled?" Done. http://codereview.chromium.org/8673008/diff/15001/base/cancelable_callback.h#... base/cancelable_callback.h:70: // Returns true if the wrapped callback is null. On 2011/11/23 23:41:07, awong wrote: > This isn't true is it? It's if the CancelableCallback itself has been Reset w/o > a callback? > In the current implementation it is the former (which happens to be the case for the latter as well). > All constructors/mutators assert the wrapped callback is not null. Are you asking me to add these checks? http://codereview.chromium.org/8673008/diff/15001/base/cancelable_callback.h#... base/cancelable_callback.h:74: // Closure that runs the stored callback. On 2011/11/23 23:41:07, awong wrote: > Comment is too close to function name. remove? Done. http://codereview.chromium.org/8673008/diff/15001/base/cancelable_callback.h#... base/cancelable_callback.h:78: // |weak_factory_|. On 2011/11/23 23:41:07, awong wrote: > nit: How about InitializeForwarder instead of BindForwarder? Done. http://codereview.chromium.org/8673008/diff/15001/base/cancelable_callback_un... File base/cancelable_callback_unittest.cc (right): http://codereview.chromium.org/8673008/diff/15001/base/cancelable_callback_un... base/cancelable_callback_unittest.cc:25: // * Callback can be run multiple times. On 2011/11/23 23:41:07, awong wrote: > Formatting Nit: I've been using a "-" instead of a "*" for these and starting > with a single, short, summary sentence for the test. > > Example: > > http://www.google.com/codesearch#OAMlx_jo-ck/src/base/bind_unittest.cc&exact_... Done. http://codereview.chromium.org/8673008/diff/15001/base/cancelable_callback_un... base/cancelable_callback_unittest.cc:62: cancelable.Cancel(); On 2011/11/23 23:41:07, awong wrote: > Add comment saying this Cancel() should have no effect, but should still be > callable. Done.
http://codereview.chromium.org/8673008/diff/15001/base/cancelable_callback.h File base/cancelable_callback.h (right): http://codereview.chromium.org/8673008/diff/15001/base/cancelable_callback.h#... base/cancelable_callback.h:37: // }; On 2011/11/23 23:41:07, awong wrote: > This use case looks exactly like what you'd use a WeakPtrFactory for. > > Can we modify to show creating callbacks that we might cancel independent of one > another? > > Also, the end of this comment should include a disucssion of the threading > restrictions and lifetime guarantees of the target callback. Done.
LGTM Will: this.Pass();
If awong is fine with this, I am too. http://codereview.chromium.org/8673008/diff/14005/base/cancelable_callback.cc File base/cancelable_callback.cc (right): http://codereview.chromium.org/8673008/diff/14005/base/cancelable_callback.cc... base/cancelable_callback.cc:20: DCHECK_EQ(false, callback.is_null()); Nit: Is there a reason you prefer DCHECK_EQ (false, x) to DCHECK(!x)? Seems unnecessarily verbose and makes the error message be a bit different. http://codereview.chromium.org/8673008/diff/14005/base/cancelable_callback.h File base/cancelable_callback.h (right): http://codereview.chromium.org/8673008/diff/14005/base/cancelable_callback.h#... base/cancelable_callback.h:12: // CancellableCallback objects must be created on, posted to, cancelled on, and Cancelable http://codereview.chromium.org/8673008/diff/14005/base/cancelable_callback.h#... base/cancelable_callback.h:46: class BASE_EXPORT CancelableCallback { Should we inherit from NonThreadSafe here? http://codereview.chromium.org/8673008/diff/14005/base/cancelable_callback_un... File base/cancelable_callback_unittest.cc (right): http://codereview.chromium.org/8673008/diff/14005/base/cancelable_callback_un... base/cancelable_callback_unittest.cc:7: #include "base/cancelable_callback.h" goes first http://codereview.chromium.org/8673008/diff/14005/base/cancelable_callback_un... base/cancelable_callback_unittest.cc:50: base::Bind(Increment, base::Unretained(&count))); I always forget, doesn't Increment need & before it?
LGTM
http://codereview.chromium.org/8673008/diff/14005/base/cancelable_callback_un... File base/cancelable_callback_unittest.cc (right): http://codereview.chromium.org/8673008/diff/14005/base/cancelable_callback_un... base/cancelable_callback_unittest.cc:50: base::Bind(Increment, base::Unretained(&count))); On 2011/11/24 00:44:30, willchan wrote: > I always forget, doesn't Increment need & before it? I had thought it did, but I have again learned that I don't understand the type promotion rules from functions to function pointers. Let's add it in for good measure though.
http://codereview.chromium.org/8673008/diff/14005/base/cancelable_callback.cc File base/cancelable_callback.cc (right): http://codereview.chromium.org/8673008/diff/14005/base/cancelable_callback.cc... base/cancelable_callback.cc:20: DCHECK_EQ(false, callback.is_null()); On 2011/11/24 00:44:30, willchan wrote: > Nit: Is there a reason you prefer DCHECK_EQ (false, x) to DCHECK(!x)? Seems > unnecessarily verbose and makes the error message be a bit different. It's more explicit; however, I don't care to push it anymore so I'll change it. http://codereview.chromium.org/8673008/diff/14005/base/cancelable_callback.h File base/cancelable_callback.h (right): http://codereview.chromium.org/8673008/diff/14005/base/cancelable_callback.h#... base/cancelable_callback.h:12: // CancellableCallback objects must be created on, posted to, cancelled on, and On 2011/11/24 00:44:30, willchan wrote: > Cancelable Done. http://codereview.chromium.org/8673008/diff/14005/base/cancelable_callback.h#... base/cancelable_callback.h:46: class BASE_EXPORT CancelableCallback { On 2011/11/24 00:44:30, willchan wrote: > Should we inherit from NonThreadSafe here? I'll make that change in a followup CL since it may be (only slightly) invasive. http://codereview.chromium.org/8673008/diff/14005/base/cancelable_callback_un... File base/cancelable_callback_unittest.cc (right): http://codereview.chromium.org/8673008/diff/14005/base/cancelable_callback_un... base/cancelable_callback_unittest.cc:7: #include "base/cancelable_callback.h" On 2011/11/24 00:44:30, willchan wrote: > goes first I read (on either c-style or chromium-dev) that this only applies if there is not a corresponding implementation file for myclass.h. Same thing as my previous reply though: I'll just make the change. http://codereview.chromium.org/8673008/diff/14005/base/cancelable_callback_un... base/cancelable_callback_unittest.cc:50: base::Bind(Increment, base::Unretained(&count))); On 2011/11/24 01:00:03, awong wrote: > On 2011/11/24 00:44:30, willchan wrote: > > I always forget, doesn't Increment need & before it? > > I had thought it did, but I have again learned that I don't understand the type > promotion rules from functions to function pointers. > > Let's add it in for good measure though. Hmm, I was really sure I've seen this fail to compile before...though I suppose I don't understand the circumstances either. This change works for me on Linux, so we'll see if it sticks.
http://codereview.chromium.org/8673008/diff/14005/base/cancelable_callback_un... File base/cancelable_callback_unittest.cc (right): http://codereview.chromium.org/8673008/diff/14005/base/cancelable_callback_un... base/cancelable_callback_unittest.cc:7: #include "base/cancelable_callback.h" On 2011/11/24 02:47:48, James Hawkins wrote: > On 2011/11/24 00:44:30, willchan wrote: > > goes first > > I read (on either c-style or chromium-dev) that this only applies if there is > not a corresponding implementation file for myclass.h. Same thing as my > previous reply though: I'll just make the change. Will has caught me on this previously, but it doesn't stick in my head. :-/ Style guide got modified somewhere in the last year or two. It used to be as you had it originally. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_... http://codereview.chromium.org/8673008/diff/14005/base/cancelable_callback_un... base/cancelable_callback_unittest.cc:50: base::Bind(Increment, base::Unretained(&count))); On 2011/11/24 02:47:48, James Hawkins wrote: > On 2011/11/24 01:00:03, awong wrote: > > On 2011/11/24 00:44:30, willchan wrote: > > > I always forget, doesn't Increment need & before it? > > > > I had thought it did, but I have again learned that I don't understand the > type > > promotion rules from functions to function pointers. > > > > Let's add it in for good measure though. > > Hmm, I was really sure I've seen this fail to compile before...though I suppose > I don't understand the circumstances either. This change works for me on Linux, > so we'll see if it sticks. I *know* I went through and fixed a bunch that failed to compile. But jam found a case of the auto-promotion happening. Function type and array type auto-promotion. The bane of my life. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
