Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22877038:
Add a CallbackRegistry class to base/ to manage callbacks (Closed)
|
Created:
7 years, 4 months ago by Cait (Slow) Modified:
7 years, 3 months ago CC:
chromium-reviews, erikwright+watch_chromium.org, danakj, Bernhard Bauer, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionThis is needed in order to reduce the boilerplate involved in converting Notifications into callbacks. It is intended to be the callback version of ObserverList.
Usage Assumptions:
-Publishers outlive listeners (or at least, we can refactor things such that this is the case).
Usage Requirements:
-Typesafe: no blobs and casts.
-Decentralized: listener registers with publisher directly, instead of through a central service.
-Flexible: A listener may register callbacks with several publishers.
-Ease-of-use: Listener does not need to explicitly hold a reference to a publisher to deregister from it.
-Minimizes the risks of use-after-free: detect dangerous states (e.g dead listener is still registered) and fail in predictable ways instead. Also make it explicitly difficult to end up in those states.
BUG=268984
TEST=base/callback_list_unittest.cc
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222559
Patch Set 1 #
Total comments: 15
Patch Set 2 : #
Total comments: 27
Patch Set 3 : More tests and some clean up #Patch Set 4 : #
Total comments: 14
Patch Set 5 : #
Total comments: 6
Patch Set 6 : #
Total comments: 8
Patch Set 7 : Remove the macro, template on details type #
Total comments: 5
Patch Set 8 : Use CallbackListBase #
Total comments: 34
Patch Set 9 : Move internal bits to base::internal, fix leaks #
Total comments: 45
Patch Set 10 : Comments pt 1. #Patch Set 11 : Better documentation of test invariants #
Total comments: 29
Patch Set 12 : Get rid of CallbackListWithDetails, use specialization instead #
Total comments: 68
Patch Set 13 : Much clean up and removal of unneeded bits #
Total comments: 28
Patch Set 14 : #
Total comments: 1
Patch Set 15 : List --> Registry, Closure --> Handle #
Total comments: 39
Patch Set 16 : #
Total comments: 21
Patch Set 17 : Move Subscription inside CallbackRegistry<T> #Patch Set 18 : ...and clean up #
Total comments: 21
Patch Set 19 : #
Total comments: 19
Patch Set 20 : #
Total comments: 5
Patch Set 21 : Rebase + nits #
Messages
Total messages: 91 (0 generated)
https://codereview.chromium.org/22877038/diff/1/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/1/base/callback_list.h#newcode1 base/callback_list.h:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. No (c), 2013. For all new files. https://codereview.chromium.org/22877038/diff/1/base/callback_list.h#newcode57 base/callback_list.h:57: // remove_foo_callback_.Run(); Grr. Not a fan of is_null. Drop it; I have a suggestion below. https://codereview.chromium.org/22877038/diff/1/base/callback_list.h#newcode70 base/callback_list.h:70: template <class SIG> no indent https://codereview.chromium.org/22877038/diff/1/base/callback_list.h#newcode110 base/callback_list.h:110: // the same list more than once. Guarantee in the comment that you can always safely run the returned closure... https://codereview.chromium.org/22877038/diff/1/base/callback_list.h#newcode115 base/callback_list.h:115: return base::Closure(); ... and rather than returning base::Closure(), return base::bind(&base::DoNothing). https://codereview.chromium.org/22877038/diff/1/base/callback_list_unittest.cc File base/callback_list_unittest.cc (right): https://codereview.chromium.org/22877038/diff/1/base/callback_list_unittest.c... base/callback_list_unittest.cc:34: Listener a(1), b(-1), c(1), d(-1), e(-1); d and e aren't used https://codereview.chromium.org/22877038/diff/1/base/callback_list_unittest.c... base/callback_list_unittest.cc:65: drop blank line
Note that in https://codereview.chromium.org/22859034/, you forgot to null-check the removal callback before calling it. That's why you want this fix; even you messed it up. What chance do we have? :)
On 2013/08/21 19:58:39, Avi wrote: > Note that in https://codereview.chromium.org/22859034/, you forgot to null-check > the removal callback before calling it. That's why you want this fix; even you > messed it up. What chance do we have? :) std::vector<base::Closure> is already everywhere: https://code.google.com/p/chromium/codesearch#search/&q=vector%3Cbase::Closur... so you can have fun retrofitting this in.
https://codereview.chromium.org/22877038/diff/1/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/1/base/callback_list.h#newcode1 base/callback_list.h:1: // Copyright (c) 2011 The Chromium Authors. All rights reserved. On 2013/08/21 19:56:18, Avi wrote: > No (c), 2013. > > For all new files. Done. https://codereview.chromium.org/22877038/diff/1/base/callback_list.h#newcode57 base/callback_list.h:57: // remove_foo_callback_.Run(); On 2013/08/21 19:56:18, Avi wrote: > Grr. Not a fan of is_null. Drop it; I have a suggestion below. Done -- there are a couple more uses of is_null(), but they are internal (e.g. if we are unable to delete a cb from the list, because we're iterating, then it gets set to a null cb until we can delete it). I suppose that could change too if we extract out the iterating logic as Pawel suggested. https://codereview.chromium.org/22877038/diff/1/base/callback_list.h#newcode70 base/callback_list.h:70: template <class SIG> On 2013/08/21 19:56:18, Avi wrote: > no indent Done. https://codereview.chromium.org/22877038/diff/1/base/callback_list.h#newcode110 base/callback_list.h:110: // the same list more than once. On 2013/08/21 19:56:18, Avi wrote: > Guarantee in the comment that you can always safely run the returned closure... Done. https://codereview.chromium.org/22877038/diff/1/base/callback_list.h#newcode115 base/callback_list.h:115: return base::Closure(); On 2013/08/21 19:56:18, Avi wrote: > ... and rather than returning base::Closure(), return > base::bind(&base::DoNothing). Done. https://codereview.chromium.org/22877038/diff/1/base/callback_list_unittest.cc File base/callback_list_unittest.cc (right): https://codereview.chromium.org/22877038/diff/1/base/callback_list_unittest.c... base/callback_list_unittest.cc:34: Listener a(1), b(-1), c(1), d(-1), e(-1); On 2013/08/21 19:56:18, Avi wrote: > d and e aren't used Done. https://codereview.chromium.org/22877038/diff/1/base/callback_list_unittest.c... base/callback_list_unittest.cc:65: On 2013/08/21 19:56:18, Avi wrote: > drop blank line Done.
Several of my comments concern code that is broken or otherwise wrong in ObserverList. I wouldn't mind if you fixed that code too ;) https://codereview.chromium.org/22877038/diff/1/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/1/base/callback_list.h#newcode57 base/callback_list.h:57: // remove_foo_callback_.Run(); I don't mind internal use of is_null. But when you hand back callbacks to the outside world, it's nice to guarantee that they're always runnable. https://codereview.chromium.org/22877038/diff/8001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/8001/base/callback_list.h#newcode6 base/callback_list.h:6: #define BASE_CALLBACK_LIST_H__ One trailing _. (Yes, I see that ObserverList still has two, but two hasn't been the style in yeeeears.) https://codereview.chromium.org/22877038/diff/8001/base/callback_list.h#newco... base/callback_list.h:38: // void NotifyFoo(Foo* foo) { NotifyFoo really belongs in the private section; it's usually the guts of MyWidget that are notifying callbacks, not random strangers on the outside. https://codereview.chromium.org/22877038/diff/8001/base/callback_list.h#newco... base/callback_list.h:50: // MyWidgetListener::MyWidgetListener(){ Space between ) {. https://codereview.chromium.org/22877038/diff/8001/base/callback_list.h#newco... base/callback_list.h:51: // remove_foo_callback_ = MyWidget::GetCurrent()->RegisterCallback( Only one space between _ =. https://codereview.chromium.org/22877038/diff/8001/base/callback_list.h#newco... base/callback_list.h:60: // // do something. Comments are complete sentences. Capitalizing "do" will do. https://codereview.chromium.org/22877038/diff/8001/base/callback_list.h#newco... base/callback_list.h:92: } This is a very strange iterator class, one that is different than the parallel one in ObserverList. You might want to note that GetNext might return null callbacks, and that the caller shouldn't rely on that to stop iteration. Perhaps we should make the iterator class private. Perhaps we should make ObserverList::Iterator private too (does anyone external actually use it?). https://codereview.chromium.org/22877038/diff/8001/base/callback_list.h#newco... base/callback_list.h:139: if (callbacks_[i].Equlas(cb)) Can you compile and run your unit test before uploading each patch set? It would catch typos like this :) https://codereview.chromium.org/22877038/diff/8001/base/callback_list.h#newco... base/callback_list.h:154: size_t size() const { return callbacks_.size(); } This is misleading, as it counts deleted items. I'd make it private or at least protected. I'd make the ObserverList one private too. https://codereview.chromium.org/22877038/diff/8001/base/callback_list.h#newco... base/callback_list.h:173: int notify_depth_; IMO this should be named "active_iterator_count_" or the like. Yes, it's misnamed in ObserverList too. https://codereview.chromium.org/22877038/diff/8001/base/callback_list.h#newco... base/callback_list.h:188: DCHECK_EQ(CallbackListBase<SIG>::size(), 0U); In DCHECKs the constant is the first parameter. Yes, broken in ObserverList too. https://codereview.chromium.org/22877038/diff/8001/base/callback_list.h#newco... base/callback_list.h:204: cb.Run(__VA_ARGS__ ) ; \ Are those two odd spaces required? Eew. https://codereview.chromium.org/22877038/diff/8001/base/callback_list.h#newco... base/callback_list.h:204: cb.Run(__VA_ARGS__ ) ; \ Variadic macros are new in C99. Does this work in all our compilers (especially concerned here about VS)? Does this work in C++ code? https://codereview.chromium.org/22877038/diff/8001/base/callback_list_unittes... File base/callback_list_unittest.cc (right): https://codereview.chromium.org/22877038/diff/8001/base/callback_list_unittes... base/callback_list_unittest.cc:78: Tests for base::Closures (Callback<void(void)>s) would be awesome, though it might depend on you writing more code to implement support.
https://codereview.chromium.org/22877038/diff/8001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/8001/base/callback_list.h#newcode6 base/callback_list.h:6: #define BASE_CALLBACK_LIST_H__ On 2013/08/22 16:25:42, Avi wrote: > One trailing _. (Yes, I see that ObserverList still has two, but two hasn't been > the style in yeeeears.) Done. https://codereview.chromium.org/22877038/diff/8001/base/callback_list.h#newco... base/callback_list.h:38: // void NotifyFoo(Foo* foo) { On 2013/08/22 16:25:42, Avi wrote: > NotifyFoo really belongs in the private section; it's usually the guts of > MyWidget that are notifying callbacks, not random strangers on the outside. Done. https://codereview.chromium.org/22877038/diff/8001/base/callback_list.h#newco... base/callback_list.h:50: // MyWidgetListener::MyWidgetListener(){ On 2013/08/22 16:25:42, Avi wrote: > Space between ) {. Done. https://codereview.chromium.org/22877038/diff/8001/base/callback_list.h#newco... base/callback_list.h:51: // remove_foo_callback_ = MyWidget::GetCurrent()->RegisterCallback( On 2013/08/22 16:25:42, Avi wrote: > Only one space between _ =. Done. https://codereview.chromium.org/22877038/diff/8001/base/callback_list.h#newco... base/callback_list.h:60: // // do something. On 2013/08/22 16:25:42, Avi wrote: > Comments are complete sentences. Capitalizing "do" will do. Done. https://codereview.chromium.org/22877038/diff/8001/base/callback_list.h#newco... base/callback_list.h:92: } On 2013/08/22 16:25:42, Avi wrote: > This is a very strange iterator class, one that is different than the parallel > one in ObserverList. You might want to note that GetNext might return null > callbacks, and that the caller shouldn't rely on that to stop iteration. Perhaps > we should make the iterator class private. Perhaps we should make > ObserverList::Iterator private too (does anyone external actually use it?). So I've cleaned up the iterator class a bit. Notably: it is now safe to assume that if it returns a null callback, then it is safe to stop iteration. Unfortunately, it can't be made private, because the macro needs to use it (same with ObserverList::Iterator, which is actually used a fair amount externally beyond that). https://codereview.chromium.org/22877038/diff/8001/base/callback_list.h#newco... base/callback_list.h:139: if (callbacks_[i].Equlas(cb)) On 2013/08/22 16:25:42, Avi wrote: > Can you compile and run your unit test before uploading each patch set? It would > catch typos like this :) Done. https://codereview.chromium.org/22877038/diff/8001/base/callback_list.h#newco... base/callback_list.h:154: size_t size() const { return callbacks_.size(); } On 2013/08/22 16:25:42, Avi wrote: > This is misleading, as it counts deleted items. I'd make it private or at least > protected. I'd make the ObserverList one private too. Done. (NB: Parallel observerList changes coming in a separate CL). https://codereview.chromium.org/22877038/diff/8001/base/callback_list.h#newco... base/callback_list.h:173: int notify_depth_; On 2013/08/22 16:25:42, Avi wrote: > IMO this should be named "active_iterator_count_" or the like. Yes, it's > misnamed in ObserverList too. Done. https://codereview.chromium.org/22877038/diff/8001/base/callback_list.h#newco... base/callback_list.h:188: DCHECK_EQ(CallbackListBase<SIG>::size(), 0U); On 2013/08/22 16:25:42, Avi wrote: > In DCHECKs the constant is the first parameter. Yes, broken in ObserverList too. Done. https://codereview.chromium.org/22877038/diff/8001/base/callback_list.h#newco... base/callback_list.h:204: cb.Run(__VA_ARGS__ ) ; \ On 2013/08/22 16:25:42, Avi wrote: > Are those two odd spaces required? Eew. Done. https://codereview.chromium.org/22877038/diff/8001/base/callback_list.h#newco... base/callback_list.h:204: cb.Run(__VA_ARGS__ ) ; \ On 2013/08/22 16:25:42, Avi wrote: > Variadic macros are new in C99. Does this work in all our compilers (especially > concerned here about VS)? Does this work in C++ code? Good question -- to the trybots! https://codereview.chromium.org/22877038/diff/8001/base/callback_list_unittes... File base/callback_list_unittest.cc (right): https://codereview.chromium.org/22877038/diff/8001/base/callback_list_unittes... base/callback_list_unittest.cc:78: On 2013/08/22 16:25:42, Avi wrote: > Tests for base::Closures (Callback<void(void)>s) would be awesome, though it > might depend on you writing more code to implement support. Converted the existing tests to use base::Closures. I'll add a couple more to make sure params get passed properly.
https://codereview.chromium.org/22877038/diff/8001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/8001/base/callback_list.h#newco... base/callback_list.h:92: } On 2013/08/23 16:33:44, Cait Phillips wrote: > Unfortunately, it can't be made private, because the macro needs to use it What!? Macros aren't class-scoped? Yeah... ok... sniff... https://codereview.chromium.org/22877038/diff/15001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/15001/base/callback_list.h#newc... base/callback_list.h:109: return base::Bind(&base::DoNothing); Now that is defensive programming. I'm actually laughing as I read this. OTOH, is that an idiotic enough behavior to warrant a DCHECK? https://codereview.chromium.org/22877038/diff/15001/base/callback_list_unitte... File base/callback_list_unittest.cc (right): https://codereview.chromium.org/22877038/diff/15001/base/callback_list_unitte... base/callback_list_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. New files get "Copyright 2013". https://codereview.chromium.org/22877038/diff/15001/base/callback_list_unitte... base/callback_list_unittest.cc:26: total ++; No space. https://codereview.chromium.org/22877038/diff/15001/base/callback_list_unitte... base/callback_list_unittest.cc:66: class Clearer : public Listener{ Space before { https://codereview.chromium.org/22877038/diff/15001/base/callback_list_unitte... base/callback_list_unittest.cc:158: // |remover_1| runs once (and rms itself), |remover_2| runs once (and removes s/rms/removes/ https://codereview.chromium.org/22877038/diff/15001/base/callback_list_unitte... base/callback_list_unittest.cc:181: TEST(CallbackListTest, AddCallbacksDuringIteration) { So we're only supporting the equivalent of NOTIFY_ALL model of ObserverList. Probably worth documenting. https://codereview.chromium.org/22877038/diff/15001/base/callback_list_unitte... base/callback_list_unittest.cc:249: } // namespace What happened to tests of callbacks with parameters? It would be nice to have at least one.
https://codereview.chromium.org/22877038/diff/15001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/15001/base/callback_list.h#newc... base/callback_list.h:109: return base::Bind(&base::DoNothing); On 2013/08/23 16:51:12, Avi wrote: > Now that is defensive programming. I'm actually laughing as I read this. > > OTOH, is that an idiotic enough behavior to warrant a DCHECK? Done. https://codereview.chromium.org/22877038/diff/15001/base/callback_list_unitte... File base/callback_list_unittest.cc (right): https://codereview.chromium.org/22877038/diff/15001/base/callback_list_unitte... base/callback_list_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/08/23 16:51:12, Avi wrote: > New files get "Copyright 2013". Done. https://codereview.chromium.org/22877038/diff/15001/base/callback_list_unitte... base/callback_list_unittest.cc:26: total ++; On 2013/08/23 16:51:12, Avi wrote: > No space. Done. https://codereview.chromium.org/22877038/diff/15001/base/callback_list_unitte... base/callback_list_unittest.cc:66: class Clearer : public Listener{ On 2013/08/23 16:51:12, Avi wrote: > Space before { Done. https://codereview.chromium.org/22877038/diff/15001/base/callback_list_unitte... base/callback_list_unittest.cc:158: // |remover_1| runs once (and rms itself), |remover_2| runs once (and removes On 2013/08/23 16:51:12, Avi wrote: > s/rms/removes/ Done. https://codereview.chromium.org/22877038/diff/15001/base/callback_list_unitte... base/callback_list_unittest.cc:181: TEST(CallbackListTest, AddCallbacksDuringIteration) { On 2013/08/23 16:51:12, Avi wrote: > So we're only supporting the equivalent of NOTIFY_ALL model of ObserverList. > Probably worth documenting. Actually, adding support for EXISTING_ONLY was pretty straight forward, and seemed worthwhile for feature-parity with ObserverList. https://codereview.chromium.org/22877038/diff/15001/base/callback_list_unitte... base/callback_list_unittest.cc:249: } // namespace On 2013/08/23 16:51:12, Avi wrote: > What happened to tests of callbacks with parameters? It would be nice to have at > least one. Done.
LGTM with my nits. And give it a good subject line; this is no longer just a WIP. :) https://codereview.chromium.org/22877038/diff/30001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/30001/base/callback_list.h#newc... base/callback_list.h:75: // This is the default type if non type is provided to the constructor. "if non type" what u mean? fix Observer List too https://codereview.chromium.org/22877038/diff/30001/base/callback_list.h#newc... base/callback_list.h:78: // Specifies that callbacks added while sending out notification are not "while sending out notifications" or "while sending out a notification" english plz fix Observer List too https://codereview.chromium.org/22877038/diff/30001/base/callback_list_unitte... File base/callback_list_unittest.cc (right): https://codereview.chromium.org/22877038/diff/30001/base/callback_list_unitte... base/callback_list_unittest.cc:19: typedef CallbackList<OneParamCallback> TestCallbackList_2; Perhaps TestCallbackListWithParam or something? _2 is odd.
Mark: PTAL -- thanks! https://codereview.chromium.org/22877038/diff/30001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/30001/base/callback_list.h#newc... base/callback_list.h:75: // This is the default type if non type is provided to the constructor. On 2013/08/23 18:36:14, Avi wrote: > "if non type" what u mean? > > fix Observer List too Done. https://codereview.chromium.org/22877038/diff/30001/base/callback_list.h#newc... base/callback_list.h:78: // Specifies that callbacks added while sending out notification are not On 2013/08/23 18:36:14, Avi wrote: > "while sending out notifications" or "while sending out a notification" > > english plz > > fix Observer List too Done. https://codereview.chromium.org/22877038/diff/30001/base/callback_list_unitte... File base/callback_list_unittest.cc (right): https://codereview.chromium.org/22877038/diff/30001/base/callback_list_unitte... base/callback_list_unittest.cc:19: typedef CallbackList<OneParamCallback> TestCallbackList_2; On 2013/08/23 18:36:14, Avi wrote: > Perhaps TestCallbackListWithParam or something? _2 is odd. Done.
This seems reasonable, but I delegate this to ajwong, master of all things callback.
I like where this is going, but have some concerns about the implementation and API design. I've added some comments, but it might be faster if we got together and chatted realtime on monday. You up for that? https://codereview.chromium.org/22877038/diff/35001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/35001/base/callback_list.h#newc... base/callback_list.h:40: // FOR_EACH_CALLBACK(OnFooCallback, callback_list_, foo); Is there a way to make this into something like callback_list_->Run(foo)? and avoid the macro? https://codereview.chromium.org/22877038/diff/35001/base/callback_list.h#newc... base/callback_list.h:69: template <class SIG> nit: The convention in base/callback.h is to use "Sig" not "SIG". Please make this consistent. Also, is this CallbackType? https://codereview.chromium.org/22877038/diff/35001/base/callback_list.h#newc... base/callback_list.h:87: Iterator(CallbackListBase<SIG>& list) Taking things by reference is disallowed by style guide. Can we do this via pointer? https://codereview.chromium.org/22877038/diff/35001/base/callback_list.h#newc... base/callback_list.h:212: if (check_empty) { If you're taking this as a template constant parameter, there's no reason for this to be a runtime check. You can specialize the destructor implementation separately. Though, is it possible to just define one single behavior? I'd be nicer to always assert the list is empty on destruction.
Note, BTW, that a lot of this behavior is intentionally mirroring what ObserverList does. If you have suggestions to clean up the API that this presents, we should probably clean up ObserverList in a parallel manner.
I set up some time to meet and discuss this tomorrow in realtime (feel free to reschedule if needed). In the meantime, I'll work on addressing your comments. Cheers, Cait
shoot sorry. I lost track of time over lunch...I'm free now if you still are. On Mon, Aug 26, 2013 at 12:03 PM, <caitkp@chromium.org> wrote: > I set up some time to meet and discuss this tomorrow in realtime (feel > free to > reschedule if needed). In the meantime, I'll work on addressing your > comments. > > Cheers, > Cait > > https://codereview.chromium.**org/22877038/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I was able to remove the macro and switch the code to be templated on the details type, but I've yet to figure out how to do the specialization without re-implementing the whole class. Also still need to add thread-checking. https://codereview.chromium.org/22877038/diff/35001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/35001/base/callback_list.h#newc... base/callback_list.h:40: // FOR_EACH_CALLBACK(OnFooCallback, callback_list_, foo); On 2013/08/24 04:11:07, awong wrote: > Is there a way to make this into something like > > callback_list_->Run(foo)? > > and avoid the macro? Done. https://codereview.chromium.org/22877038/diff/35001/base/callback_list.h#newc... base/callback_list.h:69: template <class SIG> On 2013/08/24 04:11:07, awong wrote: > nit: The convention in base/callback.h is to use "Sig" not "SIG". Please make > this consistent. > > Also, is this CallbackType? Done. https://codereview.chromium.org/22877038/diff/35001/base/callback_list.h#newc... base/callback_list.h:87: Iterator(CallbackListBase<SIG>& list) On 2013/08/24 04:11:07, awong wrote: > Taking things by reference is disallowed by style guide. Can we do this via > pointer? Done. https://codereview.chromium.org/22877038/diff/35001/base/callback_list.h#newc... base/callback_list.h:212: if (check_empty) { On 2013/08/24 04:11:07, awong wrote: > If you're taking this as a template constant parameter, there's no reason for > this to be a runtime check. You can specialize the destructor implementation > separately. > > Though, is it possible to just define one single behavior? I'd be nicer to > always assert the list is empty on destruction. Done.
https://codereview.chromium.org/22877038/diff/43001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/43001/base/callback_list.h#newc... base/callback_list.h:120: void RemoveCallback(const base::Callback<void(const Detail&)>& cb) { Does this and HasCallback need to be exposed as public? Seems like RegisterCallback returning a remove Closure makes this less important. https://codereview.chromium.org/22877038/diff/43001/base/callback_list.h#newc... base/callback_list.h:125: else I think style guide says you're supposed to use braces if you have an else clause. https://codereview.chromium.org/22877038/diff/43001/base/callback_list.h#newc... base/callback_list.h:232: class CallbackList<NoDetail> { Hmm...thinking back on the mess between scoped_ptr and scoped_array, it actually might not be possible. That actually might be a reason for a base class (that you'd want to stuff in the internal namespace). Can that be made to work? I'll look at it more tomorrow morning.
https://codereview.chromium.org/22877038/diff/43001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/43001/base/callback_list.h#newc... base/callback_list.h:125: else awong wrote: > I think style guide says you're supposed to use braces if you have an else > clause. Nope. It says that if one side has braces, the other side needs it too. But neither of these require braces, so they’re optional.
Cait: One brainstormy thought, perhaps you could pull these out into a base class that help CallbackBase pointers (not sure how you'd manage the lifetime yet...) and then cast back down to the right type in the subclass's Run(). The downcast is safe because the only way to add to the list is via the RegisterCallback<> call...again, not sure this works. Was just a thought I had this morning driving into work. https://codereview.chromium.org/22877038/diff/43001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/43001/base/callback_list.h#newc... base/callback_list.h:125: else On 2013/08/27 02:08:21, Mark Mentovai wrote: > awong wrote: > > I think style guide says you're supposed to use braces if you have an else > > clause. > > Nope. It says that if one side has braces, the other side needs it too. But > neither of these require braces, so they’re optional. Thanks for looking it up. Even so, in this case the lack of brace before the return is making my eyes slightly wild. :-/
On 2013/08/27 16:51:34, awong wrote: > Cait: > > One brainstormy thought, perhaps you could pull these out into a base class that > help CallbackBase pointers (not sure how you'd manage the lifetime yet...) and > then cast back down to the right type in the subclass's Run(). The downcast is > safe because the only way to add to the list is via the RegisterCallback<> > call...again, not sure this works. Was just a thought I had this morning driving > into work. So create a CallbackListBase (which manages a vector of CallbackBase ptrs), and then create 2 subclasses (one templated on Detail type, on for closures), which implement RegisterCallback(..) and Run(..) (and cast appropriately)? I'll give it a try. Seems like a CallbackListBase class is going to be needed whichever route we go. > > https://codereview.chromium.org/22877038/diff/43001/base/callback_list.h > File base/callback_list.h (right): > > https://codereview.chromium.org/22877038/diff/43001/base/callback_list.h#newc... > base/callback_list.h:125: else > On 2013/08/27 02:08:21, Mark Mentovai wrote: > > awong wrote: > > > I think style guide says you're supposed to use braces if you have an else > > > clause. > > > > Nope. It says that if one side has braces, the other side needs it too. But > > neither of these require braces, so they’re optional. > > Thanks for looking it up. Even so, in this case the lack of brace before the > return is making my eyes slightly wild. :-/
On Tue, Aug 27, 2013 at 10:12 AM, <caitkp@chromium.org> wrote: > On 2013/08/27 16:51:34, awong wrote: > >> Cait: >> > > One brainstormy thought, perhaps you could pull these out into a base >> class >> > that > >> help CallbackBase pointers (not sure how you'd manage the lifetime >> yet...) and >> then cast back down to the right type in the subclass's Run(). The >> downcast >> > is > >> safe because the only way to add to the list is via the RegisterCallback<> >> call...again, not sure this works. Was just a thought I had this morning >> > driving > >> into work. >> > > So create a CallbackListBase (which manages a vector of CallbackBase > ptrs), and > then create 2 subclasses (one templated on Detail type, on for closures), > which > implement RegisterCallback(..) and Run(..) (and cast appropriately)? > Yeah, something like that. Though again, I'm not sure how lifetime is going to work. If it does work, you can void templating the base class. Note with this method, you could just create 2 differently named classes (CallbackList, CallbackListWithDetails) where with different APIs and then CallbackList wouldn't need templates either. I'll give it a try. Seems like a CallbackListBase class is going to be > needed > whichever route we go. https://codereview.chromium.**org/22877038/diff/43001/base/** >> callback_list.h<https://codereview.chromium.org/22877038/diff/43001/base/callback_list.h> >> File base/callback_list.h (right): >> > > > https://codereview.chromium.**org/22877038/diff/43001/base/** > callback_list.h#newcode125<https://codereview.chromium.org/22877038/diff/43001/base/callback_list.h#newcode125> > >> base/callback_list.h:125: else >> On 2013/08/27 02:08:21, Mark Mentovai wrote: >> > awong wrote: >> > > I think style guide says you're supposed to use braces if you have an >> else >> > > clause. >> > >> > Nope. It says that if one side has braces, the other side needs it too. >> But >> > neither of these require braces, so they’re optional. >> > > Thanks for looking it up. Even so, in this case the lack of brace before >> the >> return is making my eyes slightly wild. :-/ >> > > > > https://codereview.chromium.**org/22877038/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hi Albert, PTAL - the latest patch uses base::internal::CallbackBase as suggested, which does remove a lot of the templating and allow better reuse of code. However, I'm not sure how to convert const base::Callback<void(const Detail&)>&'s into CallbackBase pointers w/o leaking things (see RegisterCallback). -Cait On 2013/08/27 18:24:10, awong wrote: > On Tue, Aug 27, 2013 at 10:12 AM, <mailto:caitkp@chromium.org> wrote: > > > On 2013/08/27 16:51:34, awong wrote: > > > >> Cait: > >> > > > > One brainstormy thought, perhaps you could pull these out into a base > >> class > >> > > that > > > >> help CallbackBase pointers (not sure how you'd manage the lifetime > >> yet...) and > >> then cast back down to the right type in the subclass's Run(). The > >> downcast > >> > > is > > > >> safe because the only way to add to the list is via the RegisterCallback<> > >> call...again, not sure this works. Was just a thought I had this morning > >> > > driving > > > >> into work. > >> > > > > So create a CallbackListBase (which manages a vector of CallbackBase > > ptrs), and > > then create 2 subclasses (one templated on Detail type, on for closures), > > which > > implement RegisterCallback(..) and Run(..) (and cast appropriately)? > > > > Yeah, something like that. Though again, I'm not sure how lifetime is > going to work. If it does work, you can void templating the base class. > > Note with this method, you could just create 2 differently named classes > (CallbackList, CallbackListWithDetails) where with different APIs and then > CallbackList wouldn't need templates either. > > I'll give it a try. Seems like a CallbackListBase class is going to be > > needed > > whichever route we go. > > https://codereview.chromium.**org/22877038/diff/43001/base/** > >> > callback_list.h<https://codereview.chromium.org/22877038/diff/43001/base/callback_list.h> > >> File base/callback_list.h (right): > >> > > > > > > https://codereview.chromium.**org/22877038/diff/43001/base/** > > > callback_list.h#newcode125<https://codereview.chromium.org/22877038/diff/43001/base/callback_list.h#newcode125> > > > >> base/callback_list.h:125: else > >> On 2013/08/27 02:08:21, Mark Mentovai wrote: > >> > awong wrote: > >> > > I think style guide says you're supposed to use braces if you have an > >> else > >> > > clause. > >> > > >> > Nope. It says that if one side has braces, the other side needs it too. > >> But > >> > neither of these require braces, so they’re optional. > >> > > > > Thanks for looking it up. Even so, in this case the lack of brace before > >> the > >> return is making my eyes slightly wild. :-/ > >> > > > > > > > > > https://codereview.chromium.**org/22877038/%3Chttps://codereview.chromium.org...> > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Looking pretty solid. I added a number of comments. Avi, do you have any thoughts on the API? Particularly curious for your thoughts on my comment regarding returning a "deregister" callback. Let me know when it's ready for another pass. https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:70: class CallbackListBase { The declaration will have to be public because CallbackListWithDetails() will need the full definition here. I'd just move it into the base::internal namespace and add a scary comment. https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:102: callbacks_[i]->Reset(); Call delete before setting to NULL? BTW, do we need to worry about re-entrancy? What if the Reset() causes this list to be notified again? Maybe it might be better to do CallbackBase* cb = callback_[i]; callback_[i] = NULL; cb->Reset(); delete cb; I'm not sure if that's sufficient to save us from much though. Thoughts? https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:114: protected: If we put this in the base::internal namespace, it might be okay to make all this stuff public. Alternately, I don't think anything other than NotificationType needs to be public so if we keep protected, maybe just move everything down? https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:130: if (list_.get() && --list_->active_iterator_count_ == 0) You should be able to do if (list_) here and elsewhere in the CL. https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:141: return index_ < max_index ? callbacks[index_++] : This should fit on one line. https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:159: callbacks_.erase(callbacks_.begin() + i); How frequently are lists going to be modified in place? I'm unsure where the tipping point for using a vector versus a list or a set is here. https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:166: base::Closure AddCallback(base::internal::CallbackBase* cb) { Document that this takes ownership of |cb| https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:177: bool HasCallback(base::internal::CallbackBase* cb) const { I don't think this is used. https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:199: spurious newline. https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:214: spurious newline. https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:216: CallbackListWithDetails() {} Do we need a default constructor? Few APIs are nicer :) https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:228: const base::Callback<void(const Detail&)>& cb) { Make this "const CallbackType&" as well? Alternately, don't bother with the typedef at all (up to you). https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:230: // TODO(caitkp): This is a leak. Can't the CallbackListBase<> just delete the pointer on Remove or destruct? https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:244: DISALLOW_COPY_AND_ASSIGN(CallbackListWithDetails); needs a private: https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:252: spurious newline https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:264: base::Closure RegisterCallback(const base::Closure& cb) { One more nit: maybe just call this Add()? That better parallels the internal CallbackList API. Lastly, I am again wondering about returning a "remove callback" here. Is it really that annoying for that client classes to keep a reference to the list around in a way that guarantees the list outlives them?
https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:230: // TODO(caitkp): This is a leak. On 2013/08/27 21:23:29, awong wrote: > Can't the CallbackListBase<> just delete the pointer on Remove or destruct? In the current impl, CallbackListBase keeps a vector of CallbackBase pointers, and CallbackBase has a protected dtor... https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:264: base::Closure RegisterCallback(const base::Closure& cb) { On 2013/08/27 21:23:29, awong wrote: > One more nit: maybe just call this Add()? That better parallels the internal > CallbackList API. > > Lastly, I am again wondering about returning a "remove callback" here. Is it > really that annoying for that client classes to keep a reference to the list > around in a way that guarantees the list outlives them? My thinking for the "remove callback" approach was that in the long run, it would result in less work for list owners and clients. Without it, each list owner will have to expose some way of adding and removing a callback (whereas in the previous approach, they'd only have to expose a way to add a callback, as removing would be done via the "remove callback"), and each client will need to hold a ref to the list (or the list owner), and a ref to the callback they added so it can be removed later. I've also encountered cases where things will occasionally disappear out from under you (see https://codereview.chromium.org/22799016/ for an example), and then the client ends up needing to hold (and check the validity of) a weak_ptr to the list owner. If the "remove callback" API is cumbersome and/or confusing though, we could certainly look at other approaches.
https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:230: // TODO(caitkp): This is a leak. On 2013/08/27 22:14:20, Cait Phillips wrote: > On 2013/08/27 21:23:29, awong wrote: > > Can't the CallbackListBase<> just delete the pointer on Remove or destruct? > In the current impl, CallbackListBase keeps a vector of CallbackBase pointers, > and CallbackBase has a protected dtor... Geez. What paranoid idiot made that protected...oh...that was me. :( I suggest we friend CallbackList from CallbackBase. The two are (or should be) in base::internal, and this class is tightly related enough that it makes sense to me. https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:264: base::Closure RegisterCallback(const base::Closure& cb) { On 2013/08/27 22:14:20, Cait Phillips wrote: > On 2013/08/27 21:23:29, awong wrote: > > One more nit: maybe just call this Add()? That better parallels the internal > > CallbackList API. > > > > Lastly, I am again wondering about returning a "remove callback" here. Is it > > really that annoying for that client classes to keep a reference to the list > > around in a way that guarantees the list outlives them? > > My thinking for the "remove callback" approach was that in the long run, it > would result in less work for list owners and clients. Without it, each list > owner will have to expose some way of adding and removing a callback (whereas in > the previous approach, they'd only have to expose a way to add a callback, as > removing would be done via the "remove callback"), and each client will need to > hold a ref to the list (or the list owner), and a ref to the callback they added > so it can be removed later. > > I've also encountered cases where things will occasionally disappear out from > under you (see https://codereview.chromium.org/22799016/ for an example), and > then the client ends up needing to hold (and check the validity of) a weak_ptr > to the list owner. > > If the "remove callback" API is cumbersome and/or confusing though, we could > certainly look at other approaches. My main concerns are: (1) Binding a WeakPtr<> allows people to get lazy regarding the lifetime of the CallbackList and their own object. (2) returning a Closure means yet another dynamic allocation. Neither of these are big issues, but I wish we could avoid them. Given that the implementation internally is using a "new CallbackType(cb)", it might not be possible to avoid a Callback based approach (what token would we give back to the client when they want to remove themselves?). However, do we really have to vend a Closure to a WeakPtr<>? Can we instead force classes using this to restructure their lifetime guarantees? Also, does the the previously discussed change to the destruction contract that asserts size() == 0 make this impossible?
The details of this are over my head. I added one comment before giving up due to low blood sugar; I'll try coming back to this later. Re returning a removal closure: I like it for purity's sake though it's not an easy fit into most existing users of callback lists. I keep thinking back to all the places where, in the destructor, a class calls a deregister of itself as an observer. Maybe we can make an RAII class that calls a callback on destruction to guarantee a run of the callback. https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:115: typedef std::vector<base::internal::CallbackBase*> ListType; Why CallbackBase and not Callback? It's not obvious to me (though to you, Albert, certainly), and I'd opine comment-worthy.
NotificationService has already allowed for observers to observe sources without any knowledge of their lifetimes. It would be good to discourage this behavior, but the reality of the situation is that it's there now, and it will either require a lot of refactoring to get rid of, or we'll have to work around it. That said, I agree that the CallbackList should not perpetuate this cycle of lifetime-obliviousness. So getting rid of the weak-ptr seems like a good plan. In that case, I guess we could either just return a handle of some sort for the client to use to remove its callback, or pass a closure bound to a base::Unretained list ptr? (the latter assumes that the list will outlive its clients). On 2013/08/28 15:38:57, Avi wrote: > The details of this are over my head. I added one comment before giving up due > to low blood sugar; I'll try coming back to this later. > > Re returning a removal closure: I like it for purity's sake though it's not an > easy fit into most existing users of callback lists. I keep thinking back to all > the places where, in the destructor, a class calls a deregister of itself as an > observer. Maybe we can make an RAII class that calls a callback on destruction > to guarantee a run of the callback. > > https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h > File base/callback_list.h (right): > > https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... > base/callback_list.h:115: typedef std::vector<base::internal::CallbackBase*> > ListType; > Why CallbackBase and not Callback? It's not obvious to me (though to you, > Albert, certainly), and I'd opine comment-worthy.
On Wed, Aug 28, 2013 at 10:16 AM, <caitkp@chromium.org> wrote: > NotificationService has already allowed for observers to observe sources > without > any knowledge of their lifetimes. It would be good to discourage this > behavior, > but the reality of the situation is that it's there now, and it will either > require a lot of refactoring to get rid of, or we'll have to work around > it. > Yeah. I totally know how that goes. How about lets' do it with Unretained(), and then as you hit issues, we can revisit making the returned callback more friendly? > That said, I agree that the CallbackList should not perpetuate this cycle > of > lifetime-obliviousness. So getting rid of the weak-ptr seems like a good > plan. > In that case, I guess we could either just return a handle of some sort > for the > client to use to remove its callback, or pass a closure bound to a > base::Unretained list ptr? (the latter assumes that the list will outlive > its > clients). > > > On 2013/08/28 15:38:57, Avi wrote: > >> The details of this are over my head. I added one comment before giving >> up due >> to low blood sugar; I'll try coming back to this later. >> > > Re returning a removal closure: I like it for purity's sake though it's >> not an >> easy fit into most existing users of callback lists. I keep thinking back >> to >> > all > >> the places where, in the destructor, a class calls a deregister of itself >> as >> > an > >> observer. Maybe we can make an RAII class that calls a callback on >> destruction >> to guarantee a run of the callback. >> > Avi, I've got a present for you: https://code.google.com/p/chromium/codesearch#chromium/src/base/bind_helpers.... :D https://codereview.chromium.**org/22877038/diff/53001/base/** >> callback_list.h<https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h> >> File base/callback_list.h (right): >> > > > https://codereview.chromium.**org/22877038/diff/53001/base/** > callback_list.h#newcode115<https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newcode115> > >> base/callback_list.h:115: typedef std::vector<base::internal::** >> CallbackBase*> >> ListType; >> Why CallbackBase and not Callback? It's not obvious to me (though to you, >> Albert, certainly), and I'd opine comment-worthy. >> > > > https://codereview.chromium.**org/22877038/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/08/28 17:33:37, awong wrote: > Avi, I've got a present for you: > https://code.google.com/p/chromium/codesearch#chromium/src/base/bind_helpers.... Sweet. We need to use that for all these returned closures. In fact, Cait, you should use ScopedClosureRunner in your sample code.
https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:70: class CallbackListBase { On 2013/08/27 21:23:29, awong wrote: > The declaration will have to be public because CallbackListWithDetails() will > need the full definition here. > > I'd just move it into the base::internal namespace and add a scary comment. Done. https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:114: protected: On 2013/08/27 21:23:29, awong wrote: > If we put this in the base::internal namespace, it might be okay to make all > this stuff public. Alternately, I don't think anything other than > NotificationType needs to be public so if we keep protected, maybe just move > everything down? Seems like we'd want to keep Clear() and might_have_callbacks() as part of the public API? (ObserverList exposes similar functions). https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:130: if (list_.get() && --list_->active_iterator_count_ == 0) On 2013/08/27 21:23:29, awong wrote: > You should be able to do > if (list_) > > here and elsewhere in the CL. > Done. https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:141: return index_ < max_index ? callbacks[index_++] : On 2013/08/27 21:23:29, awong wrote: > This should fit on one line. Done. https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:159: callbacks_.erase(callbacks_.begin() + i); On 2013/08/27 21:23:29, awong wrote: > How frequently are lists going to be modified in place? I'm unsure where the > tipping point for using a vector versus a list or a set is here. I was wondering that too... https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:166: base::Closure AddCallback(base::internal::CallbackBase* cb) { On 2013/08/27 21:23:29, awong wrote: > Document that this takes ownership of |cb| Done. https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:177: bool HasCallback(base::internal::CallbackBase* cb) const { On 2013/08/27 21:23:29, awong wrote: > I don't think this is used. Done. https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:199: On 2013/08/27 21:23:29, awong wrote: > spurious newline. Done. https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:214: On 2013/08/27 21:23:29, awong wrote: > spurious newline. Done. https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:216: CallbackListWithDetails() {} On 2013/08/27 21:23:29, awong wrote: > Do we need a default constructor? > > Few APIs are nicer :) Done. https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:228: const base::Callback<void(const Detail&)>& cb) { On 2013/08/27 21:23:29, awong wrote: > Make this "const CallbackType&" as well? Alternately, don't bother with the > typedef at all (up to you). Done. https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:244: DISALLOW_COPY_AND_ASSIGN(CallbackListWithDetails); On 2013/08/27 21:23:29, awong wrote: > needs a private: Done. https://codereview.chromium.org/22877038/diff/53001/base/callback_list.h#newc... base/callback_list.h:252: On 2013/08/27 21:23:29, awong wrote: > spurious newline Done.
https://codereview.chromium.org/22877038/diff/64001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/64001/base/callback_list.h#newc... base/callback_list.h:53: // remove_foo_callback_.reset(new base::ScopedClosureRunner( :( It would be nice if you could do what scoped_ptr does, and allow a reset() to a new value for ScopedClosureRunner.
Here's a more thorough review. I think I kinda like it (especially the thorough unittest coverage)! Please let me know when to take another pass. I also asked akalin@ to take a look since he's got a good overview of how all APIs fit together. https://codereview.chromium.org/22877038/diff/64001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/64001/base/callback_list.h#newc... base/callback_list.h:18: /////////////////////////////////////////////////////////////////////////////// nit: Can we remove the /////// borders? None of the other code in base seems to do this. Also, I would unindent the sections. (I'm basing my pattern off of base/files/file_path.h) https://codereview.chromium.org/22877038/diff/64001/base/callback_list.h#newc... base/callback_list.h:50: // nit: spurious newline. https://codereview.chromium.org/22877038/diff/64001/base/callback_list.h#newc... base/callback_list.h:53: // remove_foo_callback_.reset(new base::ScopedClosureRunner( On 2013/08/29 16:22:58, Avi wrote: > :( It would be nice if you could do what scoped_ptr does, and allow a reset() to > a new value for ScopedClosureRunner. Yeah...we shouldn't need to scoped_ptr<> a Scoper...Let's wait for avi's change to go in first. https://codereview.chromium.org/22877038/diff/64001/base/callback_list.h#newc... base/callback_list.h:75: class CallbackListWithDetails : public internal::CallbackListBase { Sorry to keep churning this, but it occurs to me we do composition here rather than inheritance, which would avoid the confusion with the virtual destructor. The tradeoff is making more methods in in CallbackListBase public, but since it's in base::internal, I'm actually okay with that. We'd also probably want to call CallbackListBase something like CallbackListImpl or CallbackListInternal or CallbackListFuzzyMonster or something. Do you mind switching over to that? https://codereview.chromium.org/22877038/diff/64001/base/callback_list.h#newc... base/callback_list.h:83: virtual ~CallbackListWithDetails() {} Kill off the virtual destructors? https://codereview.chromium.org/22877038/diff/64001/base/callback_list.h#newc... base/callback_list.h:116: virtual ~CallbackList() {} no virtual? https://codereview.chromium.org/22877038/diff/64001/base/callback_list_intern... File base/callback_list_internal.cc (right): https://codereview.chromium.org/22877038/diff/64001/base/callback_list_intern... base/callback_list_internal.cc:33: ListType& callbacks = list_->callbacks_; Normally I would object to the use of a reference type cause they're not really that common in chromium code, but I also see nothing banning them...hmm...up to you if you want to keep. https://codereview.chromium.org/22877038/diff/64001/base/callback_list_intern... base/callback_list_internal.cc:85: NOTREACHED() << "Callbacks can only be added once!"; This check doesn't do anything useful beyond a set anymore since we'r only comparing pointer values. If you're looking to compare callbacks_[i].Equals(cb) then that's a different story. https://codereview.chromium.org/22877038/diff/64001/base/callback_list_intern... File base/callback_list_internal.h (right): https://codereview.chromium.org/22877038/diff/64001/base/callback_list_intern... base/callback_list_internal.h:23: // DO NOT USE THIS CLASS DIRECTLY YOURSELF. Probably better to say "This class is meant as an implementation detail for CallbackList and CallbackListDetails. Do not use it directly." https://codereview.chromium.org/22877038/diff/64001/base/callback_list_intern... base/callback_list_internal.h:38: virtual ~CallbackListBase(); I don't think this needs to be virtual. No one should ever use this type... https://codereview.chromium.org/22877038/diff/64001/base/callback_list_intern... base/callback_list_internal.h:46: typedef std::vector<base::internal::CallbackBase*> ListType; It does seem that set or hash_set is a better choice here since the current code largely seems to be checking that a bunch of pointers are unique. Do you know if calling erase() in a hash_set or set invalidates iterators? If it does not invalidate, you might be able to simplify your iterator implementation. https://codereview.chromium.org/22877038/diff/64001/base/callback_list_intern... base/callback_list_internal.h:51: Iterator(CallbackListBase* list); explicit Also, this should jsut take a const base::WeakPtr<CallbackListBase>&. Then you don't need the GetWeakPtr() method in CallbackListBase. https://codereview.chromium.org/22877038/diff/64001/base/callback_list_unitte... File base/callback_list_unittest.cc (right): https://codereview.chromium.org/22877038/diff/64001/base/callback_list_unitte... base/callback_list_unittest.cc:18: typedef base::Callback<void(void)> TestCallback; TestCallback here is just a Closure. Why not use base::Closure? https://codereview.chromium.org/22877038/diff/64001/base/callback_list_unitte... base/callback_list_unittest.cc:29: virtual void OnEvent() { Naming the function with what their action is, rather than what they handle is easier to read. So void IncrementTotal() void MutiplyTotal(const int& x) or similar. https://codereview.chromium.org/22877038/diff/64001/base/callback_list_unitte... base/callback_list_unittest.cc:40: class Remover : public Listener { Is there a reason for the inheritance? If it'st just to pick up the total, I think it'd be better to avoid the implementation inheritance. https://codereview.chromium.org/22877038/diff/64001/base/callback_list_unitte... base/callback_list_unittest.cc:69: bool added; Should be added_ https://codereview.chromium.org/22877038/diff/64001/base/callback_list_unitte... base/callback_list_unittest.cc:87: bool added; nit: added -> added_ https://codereview.chromium.org/22877038/diff/64001/base/callback_list_unitte... base/callback_list_unittest.cc:107: TEST(CallbackListTest, BasicTest) { Can you add a comment listing the invariants being asserted by this test? Look at comments above the TEST_Fs in base/bind_unittests.cc for examples. https://codereview.chromium.org/22877038/diff/64001/base/callback_list_unitte... base/callback_list_unittest.cc:140: //remove_a.Run(); is this supposed to be commented out? https://codereview.chromium.org/22877038/diff/64001/base/callback_list_unitte... base/callback_list_unittest.cc:157: this newline seems inconsistent with how the previous test was written. remove?
2 more thoughts. https://codereview.chromium.org/22877038/diff/64001/base/callback_list_intern... File base/callback_list_internal.cc (right): https://codereview.chromium.org/22877038/diff/64001/base/callback_list_intern... base/callback_list_internal.cc:46: Compact(); Is this safe if there is an active iterator? https://codereview.chromium.org/22877038/diff/64001/base/callback_list_intern... base/callback_list_internal.cc:47: DCHECK_EQ(0U, size()); Was just thinking about this...in your original CL, sometimes you didn't actually care if the list was null or not. What about removing this dcheck, and instead adding a Size() or AssertNull() API that the person owning the callback list can assert on?
Addressed most of the comments. Better documentation of unit test invariants coming tomorrow a.m. https://codereview.chromium.org/22877038/diff/64001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/64001/base/callback_list.h#newc... base/callback_list.h:18: /////////////////////////////////////////////////////////////////////////////// On 2013/08/29 19:30:48, awong wrote: > nit: Can we remove the /////// borders? > > None of the other code in base seems to do this. > > Also, I would unindent the sections. (I'm basing my pattern off of > base/files/file_path.h) Done. https://codereview.chromium.org/22877038/diff/64001/base/callback_list.h#newc... base/callback_list.h:50: // On 2013/08/29 19:30:48, awong wrote: > nit: spurious newline. Done. https://codereview.chromium.org/22877038/diff/64001/base/callback_list.h#newc... base/callback_list.h:53: // remove_foo_callback_.reset(new base::ScopedClosureRunner( On 2013/08/29 19:30:48, awong wrote: > On 2013/08/29 16:22:58, Avi wrote: > > :( It would be nice if you could do what scoped_ptr does, and allow a reset() > to > > a new value for ScopedClosureRunner. > > Yeah...we shouldn't need to scoped_ptr<> a Scoper...Let's wait for avi's change > to go in first. Done. https://codereview.chromium.org/22877038/diff/64001/base/callback_list.h#newc... base/callback_list.h:75: class CallbackListWithDetails : public internal::CallbackListBase { On 2013/08/29 19:30:48, awong wrote: > Sorry to keep churning this, but it occurs to me we do composition here rather > than inheritance, which would avoid the confusion with the virtual destructor. > The tradeoff is making more methods in in CallbackListBase public, but since > it's in base::internal, I'm actually okay with that. > > We'd also probably want to call CallbackListBase something like CallbackListImpl > or CallbackListInternal or CallbackListFuzzyMonster or something. > > Do you mind switching over to that? Done -- where should the NotificationType enum live in this case? https://codereview.chromium.org/22877038/diff/64001/base/callback_list.h#newc... base/callback_list.h:83: virtual ~CallbackListWithDetails() {} On 2013/08/29 19:30:48, awong wrote: > Kill off the virtual destructors? Done. https://codereview.chromium.org/22877038/diff/64001/base/callback_list.h#newc... base/callback_list.h:116: virtual ~CallbackList() {} On 2013/08/29 19:30:48, awong wrote: > no virtual? Done. https://codereview.chromium.org/22877038/diff/64001/base/callback_list_intern... File base/callback_list_internal.cc (right): https://codereview.chromium.org/22877038/diff/64001/base/callback_list_intern... base/callback_list_internal.cc:33: ListType& callbacks = list_->callbacks_; On 2013/08/29 19:30:48, awong wrote: > Normally I would object to the use of a reference type cause they're not really > that common in chromium code, but I also see nothing banning them...hmm...up to > you if you want to keep. Done. https://codereview.chromium.org/22877038/diff/64001/base/callback_list_intern... base/callback_list_internal.cc:46: Compact(); On 2013/08/29 20:45:34, awong wrote: > Is this safe if there is an active iterator? Not really...but since we're destroying the list, all active iterators are going to be inactive very soon. It's not needed if we're not doing the size check in the dtor though, so I'll remove it. https://codereview.chromium.org/22877038/diff/64001/base/callback_list_intern... base/callback_list_internal.cc:47: DCHECK_EQ(0U, size()); On 2013/08/29 20:45:34, awong wrote: > Was just thinking about this...in your original CL, sometimes you didn't > actually care if the list was null or not. > > What about removing this dcheck, and instead adding a Size() or AssertNull() API > that the person owning the callback list can assert on? So I already sort of have this (with the might_have_callbacks() API), it doesn't compact the list prior to checking it, but provided there are no active iterators, the list should already be in a compacted state. Seems like the most important thing is to make sure that all the callback ptrs we allocated get freed upon destruction (hence the sanity-check call to Clear()). https://codereview.chromium.org/22877038/diff/64001/base/callback_list_intern... base/callback_list_internal.cc:85: NOTREACHED() << "Callbacks can only be added once!"; On 2013/08/29 19:30:48, awong wrote: > This check doesn't do anything useful beyond a set anymore since we'r only > comparing pointer values. > > If you're looking to compare callbacks_[i].Equals(cb) then that's a different > story. Done. https://codereview.chromium.org/22877038/diff/64001/base/callback_list_intern... File base/callback_list_internal.h (right): https://codereview.chromium.org/22877038/diff/64001/base/callback_list_intern... base/callback_list_internal.h:23: // DO NOT USE THIS CLASS DIRECTLY YOURSELF. On 2013/08/29 19:30:48, awong wrote: > Probably better to say > > "This class is meant as an implementation detail for CallbackList and > CallbackListDetails. Do not use it directly." Done. https://codereview.chromium.org/22877038/diff/64001/base/callback_list_intern... base/callback_list_internal.h:38: virtual ~CallbackListBase(); On 2013/08/29 19:30:48, awong wrote: > I don't think this needs to be virtual. No one should ever use this type... Done. https://codereview.chromium.org/22877038/diff/64001/base/callback_list_intern... base/callback_list_internal.h:46: typedef std::vector<base::internal::CallbackBase*> ListType; On 2013/08/29 19:30:48, awong wrote: > It does seem that set or hash_set is a better choice here since the current code > largely seems to be checking that a bunch of pointers are unique. > > Do you know if calling erase() in a hash_set or set invalidates iterators? If it > does not invalidate, you might be able to simplify your iterator implementation. I think the issue is for any type of collection, erase() will invalidate all iterators pointing to the element that was erased. So if I have two callbacks, cb1 and cb2, and cb1 erases cb2, then cb1 will run fine, but when I come back to run cb2, I'll have an iterator pointing to an element I just erased (and setting the element to NULL instead of erasing is a no-go since set/hash_set iterators are read-only). I agree that some sort of container that enforces uniqueness would be nice here, but that would prevent us from inserting NULL elements in as placeholders, and I've yet to find a better way of dealing with the (somewhat esoteric) case of callbacks removing other callbacks... https://codereview.chromium.org/22877038/diff/64001/base/callback_list_intern... base/callback_list_internal.h:51: Iterator(CallbackListBase* list); On 2013/08/29 19:30:48, awong wrote: > explicit > > Also, this should jsut take a const base::WeakPtr<CallbackListBase>&. > > Then you don't need the GetWeakPtr() method in CallbackListBase. I think GetWeakPtr is still needed, as CallbackList and CallbackListWithDetails will still need a way to get a weak_ptr when they instantiate an iterator. https://codereview.chromium.org/22877038/diff/64001/base/callback_list_unitte... File base/callback_list_unittest.cc (right): https://codereview.chromium.org/22877038/diff/64001/base/callback_list_unitte... base/callback_list_unittest.cc:18: typedef base::Callback<void(void)> TestCallback; On 2013/08/29 19:30:48, awong wrote: > TestCallback here is just a Closure. Why not use base::Closure? Done. https://codereview.chromium.org/22877038/diff/64001/base/callback_list_unitte... base/callback_list_unittest.cc:29: virtual void OnEvent() { On 2013/08/29 19:30:48, awong wrote: > Naming the function with what their action is, rather than what they handle is > easier to read. So > > void IncrementTotal() > void MutiplyTotal(const int& x) > > or similar. Done. https://codereview.chromium.org/22877038/diff/64001/base/callback_list_unitte... base/callback_list_unittest.cc:40: class Remover : public Listener { On 2013/08/29 19:30:48, awong wrote: > Is there a reason for the inheritance? > > If it'st just to pick up the total, I think it'd be better to avoid the > implementation inheritance. Done. https://codereview.chromium.org/22877038/diff/64001/base/callback_list_unitte... base/callback_list_unittest.cc:69: bool added; On 2013/08/29 19:30:48, awong wrote: > Should be added_ Done. https://codereview.chromium.org/22877038/diff/64001/base/callback_list_unitte... base/callback_list_unittest.cc:87: bool added; On 2013/08/29 19:30:48, awong wrote: > nit: added -> added_ Done. https://codereview.chromium.org/22877038/diff/64001/base/callback_list_unitte... base/callback_list_unittest.cc:140: //remove_a.Run(); On 2013/08/29 19:30:48, awong wrote: > is this supposed to be commented out? Done. https://codereview.chromium.org/22877038/diff/64001/base/callback_list_unitte... base/callback_list_unittest.cc:157: On 2013/08/29 19:30:48, awong wrote: > this newline seems inconsistent with how the previous test was written. remove? Done.
https://codereview.chromium.org/22877038/diff/79001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/79001/base/callback_list.h#newc... base/callback_list.h:50: // // TODO (caitkp): A less clunky approach here? How would this be less clunky? ScopedClosureRunner! Yay!
awong@, akalin@: gentle ping -- thanks!
On 2013/09/03 14:26:58, Cait Phillips wrote: > awong@, akalin@: gentle ping -- thanks! Albert: I've been playing around with this a bit lately and with a bit of template-fu, found a way to make CallbackList a specialization of CallbackListWithDetails (so instead of CallbackList and CallbackListWithDetails<Details>, we could just have CallbackList<> and CallbackList<Details>). In the long run, I think this will be a much more manageable API than having 2 unrelated classes--wdyt?
Here's a few in-prgress comments. Will look at your templated CL now. https://codereview.chromium.org/22877038/diff/64001/base/callback_list_intern... File base/callback_list_internal.h (right): https://codereview.chromium.org/22877038/diff/64001/base/callback_list_intern... base/callback_list_internal.h:51: Iterator(CallbackListBase* list); On 2013/08/30 00:28:02, Cait Phillips wrote: > On 2013/08/29 19:30:48, awong wrote: > > explicit > > > > Also, this should jsut take a const base::WeakPtr<CallbackListBase>&. > > > > Then you don't need the GetWeakPtr() method in CallbackListBase. > I think GetWeakPtr is still needed, as CallbackList and CallbackListWithDetails > will still need a way to get a weak_ptr when they instantiate an iterator. How about replacing it with a GetIterator()? https://codereview.chromium.org/22877038/diff/79001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/79001/base/callback_list.h#newc... base/callback_list.h:23: // iterator. So, it safely handles the case of an callback removing itself So, it safely -> It safely https://codereview.chromium.org/22877038/diff/79001/base/callback_list.h#newc... base/callback_list.h:50: // // TODO (caitkp): A less clunky approach here? On 2013/08/30 19:49:48, Avi wrote: > How would this be less clunky? > > ScopedClosureRunner! Yay! Agreed...I think we can remove this comment. https://codereview.chromium.org/22877038/diff/79001/base/callback_list.h#newc... base/callback_list.h:90: internal::CallbackListImpl::Iterator it(list_impl_->GetWeakPtr()); I still feel like this should just be: list_impl_->GetIteartor() And then we don't need the method CallbackListImpl::GetWeakPtr(). https://codereview.chromium.org/22877038/diff/79001/base/callback_list.h#newc... base/callback_list.h:92: while((cb = static_cast<CallbackType*>(it.GetNext())) != NULL) { Add a comment explaining why we know this static_cast<> is safe. https://codereview.chromium.org/22877038/diff/79001/base/callback_list.h#newc... base/callback_list.h:108: scoped_ptr<base::internal::CallbackListImpl> list_impl_; This doesn't seem like it needs to be dynamically allocated does it?
https://codereview.chromium.org/22877038/diff/79001/base/callback_list.cc File base/callback_list.cc (right): https://codereview.chromium.org/22877038/diff/79001/base/callback_list.cc#new... base/callback_list.cc:1: #include "base/callback_list.h" Missing copyright. https://codereview.chromium.org/22877038/diff/79001/base/callback_list.cc#new... base/callback_list.cc:33: } // namespace base } // namespace base https://codereview.chromium.org/22877038/diff/79001/base/callback_list_intern... File base/callback_list_internal.cc (right): https://codereview.chromium.org/22877038/diff/79001/base/callback_list_intern... base/callback_list_internal.cc:82: return base::Bind(&CallbackListImpl::RemoveCallback, Unretained(this), cb); Should this be bound to a weakptr? https://codereview.chromium.org/22877038/diff/79001/base/callback_list_intern... File base/callback_list_internal.h (right): https://codereview.chromium.org/22877038/diff/79001/base/callback_list_intern... base/callback_list_internal.h:19: enum CallbackNotificationType { I would put this inside CallbackListImpl. https://codereview.chromium.org/22877038/diff/79001/base/callback_list_intern... base/callback_list_internal.h:30: // Holds the CallbackList methods that don't require specialization to reduce don't -> do not https://codereview.chromium.org/22877038/diff/79001/base/callback_list_intern... base/callback_list_internal.h:60: // list. Add something about the meaning of the return value, what thread it's safe to run on, and whether it's safe to run after the originating CallbackList is gone. https://codereview.chromium.org/22877038/diff/79001/base/callback_list_intern... base/callback_list_internal.h:67: // callbacks. Can you add a little more explaining how this how this relates to Compact() and why this isn't a "definitely non-zero" sort of check? https://codereview.chromium.org/22877038/diff/79001/base/callback_list_intern... base/callback_list_internal.h:71: void RemoveCallback(base::internal::CallbackBase* cb); I think all the following should be private. https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:70: struct CallbackHelper { Anything that isn't in the public API should be in base::internal. https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:80: class CallbackListBase { As discussed on IM, this feels like it should be specialized on CallbackType explicitly. In the CallbackList<> declaration, you know enough to statically specify the signature, so I think you can avoid the indirection through a Helper. So something like // Declare, but not define to get the arity right. template <typename CallbackType> class CallbackListBase; // Only provide 1 specialization to whitelist only Callback<void(const T&)> types. template <typename Details> class CallbackListBase<Callback<void(const Details&)> > { /* All the stuff currently there */ }; followed by template <typename Details> class CallbackList : public CallbackListBase<void(const Details&)> { /* implementation of void Run(const Details&); */ }; template <> class CallbackList<void> : public CallbackListBase<Closure> { /* implementation of void Run(void); */ }; https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:82: typedef typename CallbackHelper<Details>::Type CallbackType; I think you may need to typedef CallbackNotificationType here if you move the enum declaration into CallbackListImpl. https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:100: bool might_have_callbacks() { One more thought...if this is necessary to expose, would it be better to actually just store an int that has the exact number of registered callbacks? You could decrement on each remove even if you're just tombstoning things with a NULL. That feels like a cleaner API since, as a user, I'm not certain what I would do if this returns true... Either that, or just remove might_have_callbacks() from the public API. https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:107: void VisitCallbacks(Visitor visitor) { The visitor is a nifty way to reuse code, but since we only have 2 implementations, and it's just a simple Iterator() loop, duplicating the logic seems easiest. BTW, is it worth having this might_have_callbacks() check? It seems more common with containers to just construct the iterator always and have the conditional immediately fail. That seems more analogous to for(vector<Foo>::const_iterator it = v.begin(); it != v.end(); ++it) { ... } https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:173: } // namespace base nit: } // namespace base https://codereview.chromium.org/22877038/diff/88001/base/callback_list_intern... File base/callback_list_internal.h (right): https://codereview.chromium.org/22877038/diff/88001/base/callback_list_intern... base/callback_list_internal.h:61: base::Closure AddCallback(base::internal::CallbackBase* cb); Rename AddCallback/RemoveCallback to Add/Remove to match the public API? https://codereview.chromium.org/22877038/diff/88001/base/callback_list_intern... base/callback_list_internal.h:82: size_t size() const { return callbacks_.size(); } This doesn't seem necessary. Remove? https://codereview.chromium.org/22877038/diff/88001/base/callback_list_unitte... File base/callback_list_unittest.cc (right): https://codereview.chromium.org/22877038/diff/88001/base/callback_list_unitte... base/callback_list_unittest.cc:21: Listener(int scaler) : total_(0), scaler_(scaler) {} explicit https://codereview.chromium.org/22877038/diff/88001/base/callback_list_unitte... base/callback_list_unittest.cc:22: ~Listener() {} No need to declare trivial destructors. Please remove here and elsewhere. https://codereview.chromium.org/22877038/diff/88001/base/callback_list_unitte... base/callback_list_unittest.cc:34: explicit Remover() : total_(0), removal_cb_(base::Closure()) {} -explicit https://codereview.chromium.org/22877038/diff/88001/base/callback_list_unitte... base/callback_list_unittest.cc:145: // the correct details, and those removed from the list will not be run. Thank you for adding these comments! I like them! https://codereview.chromium.org/22877038/diff/88001/base/callback_list_unitte... base/callback_list_unittest.cc:350: base::Bind(&ListDestructor::DestroyList, base::Unretained(&destructor))); Can this be used? https://code.google.com/p/chromium/codesearch#chromium/src/base/bind_helpers.... If so, can we remove ListDestructor? https://codereview.chromium.org/22877038/diff/88001/base/callback_list_unitte... base/callback_list_unittest.cc:357: // |a| never gets called, as |cb_list| got deleted first. Hmm...this is allowing a list to delete itself from a callback. Do we have the full reentrancy contract in mind? Let's say you have: CallbackList<void>* cl = new CallbackList<void>(); cl->Add(Bind(&DeletePointer<CallbackList<void>*>, cl)); cl->Add(Bind(&CallbackList<void>::Clear, Unretained(&cl))); cl->Run(); Will that reentrantly call Clear?
https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:23: // iterator. So, it safely handles the case of an callback removing itself an->a https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:24: // or other callback from the list while callbacks are being run. either 'other'->'another' or 'callback'->'callbacks'. I think the 'while callbacks are being run' is redundant. If the actor is a callback, it is by definition while callbacks are being run. https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:84: explicit CallbackListBase(CallbackNotificationType type) Should be protected. https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:90: base::Closure Add(const CallbackType& cb) { It seems like, in almost all cases, it would probably be a bug to not invoke the removal closure. Yet it would be easy to overlook. I suggest at a minimum making this method WARN_UNUSED_RESULT. The caller should at a minimum be copying the closure somewhere. I would also propose returning a scoped_ptr<Handle> where Handle is some object that, when it is destroyed, removes the registration. This at least switchs the failure mode from a use-after-free to never receiving the expected notification. WARN_UNUSED_RESULT would still be useful and catch cases where the client forgets to store the handle. https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:95: // Delete (or nullify, if called during iteration), all callbacks in the list. I don't think that the parenthetical is relevant clients (i.e., it's not part of the public contract). It is better placed inside the implementation of CallbackListImpl::Clear. https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:100: bool might_have_callbacks() { What client would call this? https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:105: // Execute all active (non-null) callbacks with |details| parameter. ditto on the parenthetical. And the rest (about |details|) is also now incorrect. // Visits each callback, respecting the CallbackNotificationType with regards to callbacks added during iteration. https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:128: class Visitor { can be private https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:141: // Execute all active (non-null) callbacks with |details| parameter. ditto on the parenthetical. https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:154: class Visitor { private https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:164: // Execute all active (non-null) callbacks. parenthetical https://codereview.chromium.org/22877038/diff/88001/base/callback_list_intern... File base/callback_list_internal.cc (right): https://codereview.chromium.org/22877038/diff/88001/base/callback_list_intern... base/callback_list_internal.cc:9: #include "base/bind.h" The only purpose of bind in this file is for building the 'removal closure'. If you follow my suggestion elsewhere about using a Handle object instead of a Closure you can get rid of the Bind and then I think concerns about template bloat should be pretty minor. In that case, I would propose folding CallbackListImpl into CallbackListBase and making it use base::Callback<void(Detail)> rather than CallbackBase. That would eliminate the encapsulation violation, eliminate a cast or two, etc. https://codereview.chromium.org/22877038/diff/88001/base/callback_list_intern... base/callback_list_internal.cc:53: tmp->Reset(); Why is the Reset necessary? https://codereview.chromium.org/22877038/diff/88001/base/callback_list_intern... base/callback_list_internal.cc:68: tmp->Reset(); Again, why is the Reset necessary? https://codereview.chromium.org/22877038/diff/88001/base/callback_list_intern... File base/callback_list_internal.h (right): https://codereview.chromium.org/22877038/diff/88001/base/callback_list_intern... base/callback_list_internal.h:19: enum CallbackNotificationType { I would strongly prefer that we get opinionated and select one or the other of these modes to implement. https://codereview.chromium.org/22877038/diff/88001/base/callback_list_intern... base/callback_list_internal.h:43: explicit Iterator(const base::WeakPtr<CallbackListImpl>& list); Instead of exposing GetWeakPtr and allowing clients to construct their own Iterator, how about exposing a GetIterator() and then making GetWeakPtr private? Either make it copyable (consistent with STL iterators) so that clients can assign it to a local variable or return a scoped_ptr<Iterator>. https://codereview.chromium.org/22877038/diff/88001/base/callback_list_intern... base/callback_list_internal.h:61: base::Closure AddCallback(base::internal::CallbackBase* cb); Take a scoped_ptr. I think that's the new standard for documenting ownership transfer. https://codereview.chromium.org/22877038/diff/88001/base/callback_list_intern... base/callback_list_internal.h:61: base::Closure AddCallback(base::internal::CallbackBase* cb); Document the purpose of the return value. https://codereview.chromium.org/22877038/diff/88001/base/callback_list_intern... base/callback_list_internal.h:68: bool might_have_callbacks() const { return size() != 0; } Why would a client call this? https://codereview.chromium.org/22877038/diff/88001/base/callback_list_intern... base/callback_list_internal.h:71: void RemoveCallback(base::internal::CallbackBase* cb); Seems this could be private (no reason not to insist that clients use the 'remove closure'). https://codereview.chromium.org/22877038/diff/88001/base/callback_list_intern... base/callback_list_internal.h:78: void Compact(); Any reason for a client to call this (as opposed to the Iterator)?
https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:100: bool might_have_callbacks() { On 2013/09/04 19:35:55, erikwright wrote: > What client would call this? IIRC, the discussion with Cait is that some clients want to assert the list is empty on destruction. Perhaps we just want an AssertEmpty() or something.
Did some ruthless pruning of behaviors that might someday be needed but aren't currently. Hopefully this simplifies things a bit. Thanks for the thorough reviews! https://codereview.chromium.org/22877038/diff/79001/base/callback_list.cc File base/callback_list.cc (right): https://codereview.chromium.org/22877038/diff/79001/base/callback_list.cc#new... base/callback_list.cc:1: #include "base/callback_list.h" On 2013/09/04 18:48:21, awong wrote: > Missing copyright. Done. https://codereview.chromium.org/22877038/diff/79001/base/callback_list.cc#new... base/callback_list.cc:33: } // namespace base On 2013/09/04 18:48:21, awong wrote: > } // namespace base Done. https://codereview.chromium.org/22877038/diff/79001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/79001/base/callback_list.h#newc... base/callback_list.h:23: // iterator. So, it safely handles the case of an callback removing itself On 2013/09/04 17:44:35, awong wrote: > So, it safely -> It safely Done. https://codereview.chromium.org/22877038/diff/79001/base/callback_list.h#newc... base/callback_list.h:50: // // TODO (caitkp): A less clunky approach here? On 2013/09/04 17:44:35, awong wrote: > On 2013/08/30 19:49:48, Avi wrote: > > How would this be less clunky? > > > > ScopedClosureRunner! Yay! > > Agreed...I think we can remove this comment. Done. https://codereview.chromium.org/22877038/diff/79001/base/callback_list.h#newc... base/callback_list.h:90: internal::CallbackListImpl::Iterator it(list_impl_->GetWeakPtr()); On 2013/09/04 17:44:35, awong wrote: > I still feel like this should just be: > > list_impl_->GetIteartor() > > And then we don't need the method CallbackListImpl::GetWeakPtr(). Done. https://codereview.chromium.org/22877038/diff/79001/base/callback_list.h#newc... base/callback_list.h:92: while((cb = static_cast<CallbackType*>(it.GetNext())) != NULL) { On 2013/09/04 17:44:35, awong wrote: > Add a comment explaining why we know this static_cast<> is safe. Done. https://codereview.chromium.org/22877038/diff/79001/base/callback_list.h#newc... base/callback_list.h:108: scoped_ptr<base::internal::CallbackListImpl> list_impl_; On 2013/09/04 17:44:35, awong wrote: > This doesn't seem like it needs to be dynamically allocated does it? Done. https://codereview.chromium.org/22877038/diff/79001/base/callback_list_intern... File base/callback_list_internal.cc (right): https://codereview.chromium.org/22877038/diff/79001/base/callback_list_intern... base/callback_list_internal.cc:82: return base::Bind(&CallbackListImpl::RemoveCallback, Unretained(this), cb); On 2013/09/04 18:48:21, awong wrote: > Should this be bound to a weakptr? I think this is what I had originally, but we'd decided that might encourage people to get a bit sloppy with the lifetimes of callback lists and their observers. https://codereview.chromium.org/22877038/diff/79001/base/callback_list_intern... File base/callback_list_internal.h (right): https://codereview.chromium.org/22877038/diff/79001/base/callback_list_intern... base/callback_list_internal.h:19: enum CallbackNotificationType { On 2013/09/04 18:48:21, awong wrote: > I would put this inside CallbackListImpl. Done. https://codereview.chromium.org/22877038/diff/79001/base/callback_list_intern... base/callback_list_internal.h:30: // Holds the CallbackList methods that don't require specialization to reduce On 2013/09/04 18:48:21, awong wrote: > don't -> do not Done. https://codereview.chromium.org/22877038/diff/79001/base/callback_list_intern... base/callback_list_internal.h:60: // list. On 2013/09/04 18:48:21, awong wrote: > Add something about the meaning of the return value, what thread it's safe to > run on, and whether it's safe to run after the originating CallbackList is gone. Done. https://codereview.chromium.org/22877038/diff/79001/base/callback_list_intern... base/callback_list_internal.h:67: // callbacks. On 2013/09/04 18:48:21, awong wrote: > Can you add a little more explaining how this how this relates to Compact() and > why this isn't a "definitely non-zero" sort of check? Replacing this with AssertEmpty for clarity. https://codereview.chromium.org/22877038/diff/79001/base/callback_list_intern... base/callback_list_internal.h:71: void RemoveCallback(base::internal::CallbackBase* cb); On 2013/09/04 18:48:21, awong wrote: > I think all the following should be private. Done. https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:23: // iterator. So, it safely handles the case of an callback removing itself On 2013/09/04 19:35:55, erikwright wrote: > an->a Done. https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:24: // or other callback from the list while callbacks are being run. On 2013/09/04 19:35:55, erikwright wrote: > either 'other'->'another' or 'callback'->'callbacks'. > > I think the 'while callbacks are being run' is redundant. If the actor is a > callback, it is by definition while callbacks are being run. Done. https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:70: struct CallbackHelper { On 2013/09/04 18:48:21, awong wrote: > Anything that isn't in the public API should be in base::internal. Done. https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:80: class CallbackListBase { On 2013/09/04 18:48:21, awong wrote: > As discussed on IM, this feels like it should be specialized on CallbackType > explicitly. > > In the CallbackList<> declaration, you know enough to statically specify the > signature, so I think you can avoid the indirection through a Helper. > > So something like > // Declare, but not define to get the arity right. > template <typename CallbackType> class CallbackListBase; > > // Only provide 1 specialization to whitelist only Callback<void(const T&)> > types. > template <typename Details> class CallbackListBase<Callback<void(const > Details&)> > { /* All the stuff currently there */ }; > > followed by > template <typename Details> class CallbackList : public > CallbackListBase<void(const Details&)> { /* implementation of void Run(const > Details&); */ }; > > template <> class CallbackList<void> : public CallbackListBase<Closure> { /* > implementation of void Run(void); */ }; Done. https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:82: typedef typename CallbackHelper<Details>::Type CallbackType; On 2013/09/04 18:48:21, awong wrote: > I think you may need to typedef CallbackNotificationType here if you move the > enum declaration into CallbackListImpl. Done. https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:84: explicit CallbackListBase(CallbackNotificationType type) On 2013/09/04 19:35:55, erikwright wrote: > Should be protected. Done. https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:90: base::Closure Add(const CallbackType& cb) { On 2013/09/04 19:35:55, erikwright wrote: > It seems like, in almost all cases, it would probably be a bug to not invoke the > removal closure. Yet it would be easy to overlook. > > I suggest at a minimum making this method WARN_UNUSED_RESULT. The caller should > at a minimum be copying the closure somewhere. > > I would also propose returning a scoped_ptr<Handle> where Handle is some object > that, when it is destroyed, removes the registration. This at least switchs the > failure mode from a use-after-free to never receiving the expected notification. > > WARN_UNUSED_RESULT would still be useful and catch cases where the client > forgets to store the handle. Going with WARN_UNUSED_RESULT for now. Will look into moving to handles. https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:95: // Delete (or nullify, if called during iteration), all callbacks in the list. On 2013/09/04 19:35:55, erikwright wrote: > I don't think that the parenthetical is relevant clients (i.e., it's not part of > the public contract). It is better placed inside the implementation of > CallbackListImpl::Clear. Done. https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:100: bool might_have_callbacks() { On 2013/09/04 19:50:41, awong wrote: > On 2013/09/04 19:35:55, erikwright wrote: > > What client would call this? > > IIRC, the discussion with Cait is that some clients want to assert the list is > empty on destruction. Perhaps we just want an AssertEmpty() or something. Switched to an AssertEmpty, which checks 1. no active iterators, 2. no callbacks. I think this is more clear. https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:105: // Execute all active (non-null) callbacks with |details| parameter. On 2013/09/04 19:35:55, erikwright wrote: > ditto on the parenthetical. And the rest (about |details|) is also now > incorrect. > > // Visits each callback, respecting the CallbackNotificationType with regards to > callbacks added during iteration. Done. https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:107: void VisitCallbacks(Visitor visitor) { On 2013/09/04 18:48:21, awong wrote: > The visitor is a nifty way to reuse code, but since we only have 2 > implementations, and it's just a simple Iterator() loop, duplicating the logic > seems easiest. > > BTW, is it worth having this might_have_callbacks() check? It seems more common > with containers to just construct the iterator always and have the conditional > immediately fail. > > That seems more analogous to > > for(vector<Foo>::const_iterator it = v.begin(); it != v.end(); ++it) { > ... > } Done. https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:128: class Visitor { On 2013/09/04 19:35:55, erikwright wrote: > can be private Done. https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:141: // Execute all active (non-null) callbacks with |details| parameter. On 2013/09/04 19:35:55, erikwright wrote: > ditto on the parenthetical. Done. https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:154: class Visitor { On 2013/09/04 19:35:55, erikwright wrote: > private Done. https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:164: // Execute all active (non-null) callbacks. On 2013/09/04 19:35:55, erikwright wrote: > parenthetical Done. https://codereview.chromium.org/22877038/diff/88001/base/callback_list.h#newc... base/callback_list.h:173: } // namespace base On 2013/09/04 18:48:21, awong wrote: > nit: > } // namespace base Done. https://codereview.chromium.org/22877038/diff/88001/base/callback_list_intern... File base/callback_list_internal.cc (right): https://codereview.chromium.org/22877038/diff/88001/base/callback_list_intern... base/callback_list_internal.cc:53: tmp->Reset(); On 2013/09/04 19:35:55, erikwright wrote: > Why is the Reset necessary? Done. https://codereview.chromium.org/22877038/diff/88001/base/callback_list_intern... base/callback_list_internal.cc:68: tmp->Reset(); On 2013/09/04 19:35:55, erikwright wrote: > Again, why is the Reset necessary? Done. https://codereview.chromium.org/22877038/diff/88001/base/callback_list_intern... File base/callback_list_internal.h (right): https://codereview.chromium.org/22877038/diff/88001/base/callback_list_intern... base/callback_list_internal.h:19: enum CallbackNotificationType { On 2013/09/04 19:35:55, erikwright wrote: > I would strongly prefer that we get opinionated and select one or the other of > these modes to implement. Agree. By default, all of the current NotificationSystem uses NOTIFY_ALL. How about we choose that one? https://codereview.chromium.org/22877038/diff/88001/base/callback_list_intern... base/callback_list_internal.h:43: explicit Iterator(const base::WeakPtr<CallbackListImpl>& list); On 2013/09/04 19:35:55, erikwright wrote: > Instead of exposing GetWeakPtr and allowing clients to construct their own > Iterator, how about exposing a GetIterator() and then making GetWeakPtr private? > > Either make it copyable (consistent with STL iterators) so that clients can > assign it to a local variable or return a scoped_ptr<Iterator>. Done. https://codereview.chromium.org/22877038/diff/88001/base/callback_list_intern... base/callback_list_internal.h:61: base::Closure AddCallback(base::internal::CallbackBase* cb); On 2013/09/04 19:35:55, erikwright wrote: > Take a scoped_ptr. I think that's the new standard for documenting ownership > transfer. CallbackBase has a protected dtor, so a scoped_ptr won't work here. https://codereview.chromium.org/22877038/diff/88001/base/callback_list_intern... base/callback_list_internal.h:61: base::Closure AddCallback(base::internal::CallbackBase* cb); On 2013/09/04 18:48:21, awong wrote: > Rename AddCallback/RemoveCallback to Add/Remove to match the public API? Done. https://codereview.chromium.org/22877038/diff/88001/base/callback_list_intern... base/callback_list_internal.h:61: base::Closure AddCallback(base::internal::CallbackBase* cb); On 2013/09/04 19:35:55, erikwright wrote: > Document the purpose of the return value. Done. https://codereview.chromium.org/22877038/diff/88001/base/callback_list_intern... base/callback_list_internal.h:68: bool might_have_callbacks() const { return size() != 0; } On 2013/09/04 19:35:55, erikwright wrote: > Why would a client call this? removing it https://codereview.chromium.org/22877038/diff/88001/base/callback_list_intern... base/callback_list_internal.h:71: void RemoveCallback(base::internal::CallbackBase* cb); On 2013/09/04 19:35:55, erikwright wrote: > Seems this could be private (no reason not to insist that clients use the > 'remove closure'). Done. https://codereview.chromium.org/22877038/diff/88001/base/callback_list_intern... base/callback_list_internal.h:78: void Compact(); On 2013/09/04 19:35:55, erikwright wrote: > Any reason for a client to call this (as opposed to the Iterator)? No -- it should only be called by the iterator. https://codereview.chromium.org/22877038/diff/88001/base/callback_list_intern... base/callback_list_internal.h:82: size_t size() const { return callbacks_.size(); } On 2013/09/04 18:48:21, awong wrote: > This doesn't seem necessary. Remove? Done. https://codereview.chromium.org/22877038/diff/88001/base/callback_list_unitte... File base/callback_list_unittest.cc (right): https://codereview.chromium.org/22877038/diff/88001/base/callback_list_unitte... base/callback_list_unittest.cc:21: Listener(int scaler) : total_(0), scaler_(scaler) {} On 2013/09/04 18:48:21, awong wrote: > explicit Done. https://codereview.chromium.org/22877038/diff/88001/base/callback_list_unitte... base/callback_list_unittest.cc:22: ~Listener() {} On 2013/09/04 18:48:21, awong wrote: > No need to declare trivial destructors. Please remove here and elsewhere. Done. https://codereview.chromium.org/22877038/diff/88001/base/callback_list_unitte... base/callback_list_unittest.cc:34: explicit Remover() : total_(0), removal_cb_(base::Closure()) {} On 2013/09/04 18:48:21, awong wrote: > -explicit Done. https://codereview.chromium.org/22877038/diff/88001/base/callback_list_unitte... base/callback_list_unittest.cc:350: base::Bind(&ListDestructor::DestroyList, base::Unretained(&destructor))); On 2013/09/04 18:48:21, awong wrote: > Can this be used? > > https://code.google.com/p/chromium/codesearch#chromium/src/base/bind_helpers.... > > If so, can we remove ListDestructor? Done. https://codereview.chromium.org/22877038/diff/88001/base/callback_list_unitte... base/callback_list_unittest.cc:357: // |a| never gets called, as |cb_list| got deleted first. On 2013/09/04 18:48:21, awong wrote: > Hmm...this is allowing a list to delete itself from a callback. Do we have the > full reentrancy contract in mind? > > Let's say you have: > > > CallbackList<void>* cl = new CallbackList<void>(); > > cl->Add(Bind(&DeletePointer<CallbackList<void>*>, cl)); > cl->Add(Bind(&CallbackList<void>::Clear, Unretained(&cl))); > > cl->Run(); > > Will that reentrantly call Clear? I think, upon reentrancy, the iterator's weak ptr to the list will be invalid (as we just deleted the list), so the iterator will return NULL and no more callbacks will be run.
Getting close! I like the ruthless pruning. :) Most of these are style nits with 3 actual function changes regarding Iterator pass-by-copy, the Remove closure, and AssertEmpty(). https://codereview.chromium.org/22877038/diff/79001/base/callback_list_intern... File base/callback_list_internal.cc (right): https://codereview.chromium.org/22877038/diff/79001/base/callback_list_intern... base/callback_list_internal.cc:82: return base::Bind(&CallbackListImpl::RemoveCallback, Unretained(this), cb); On 2013/09/04 22:09:25, Cait Phillips wrote: > On 2013/09/04 18:48:21, awong wrote: > > Should this be bound to a weakptr? > I think this is what I had originally, but we'd decided that might encourage > people to get a bit sloppy with the lifetimes of callback lists and their > observers. Right...hmm...on rereading, I worry that we're going to make it easy to get into a use-after-free. How about doing this pattern to get the best of both worlds: Add a static function like: // Use a static function to avoid WeakPtr cancellation behavior // in Bind(). This allows strict enforcement that the caller // must use the deregister the callback before the CallbackList is // deleted. static void CheckedRemove(const WeakPtr<CallbackListImpl>& list, base::internal::CallbackBase* cb) { list->Remove(cb); } Then in Add(), do: return base::Bind(&CheckedRemove, weak_factory.GetWeakPtr(), cb); That way the user still has to consider lifetimes, and we don't enter undefined behavior if they do the wrong thing. (If you think I'm going too far with enforcing this, let me know). https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h#newc... base/callback_list.h:70: class CallbackListBase { CallbackListBase should be in base::internal. https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h#newc... base/callback_list.h:75: base::Closure Add(const CallbackType& cb) WARN_UNUSED_RESULT { I'm going to slightly disagree with erikwright here...do we want the user to always handle this return? If users of this class don't care to AssertEmpty() before the CallbackList destruction, it seems odd to force them to explicitly ignore the removal callback. I think not having the WARN_UNUSED_RESULT is better. https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h#newc... base/callback_list.h:109: CallbackList() : CallbackListBase<CallbackType>() {} There's no need to explicitly call the default constructor of a base class. This is just a no-op. https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h#newc... base/callback_list.h:113: scoped_ptr<internal::CallbackListImpl::Iterator> it = this->GetIterator(); indentation. https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h#newc... base/callback_list.h:115: // static_cast is safe here, all callbacks were added via here, all -> here. All https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h#newc... base/callback_list.h:116: // CallbackListBase::Add and so must be of type ::Add -> ::Add() https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h#newc... base/callback_list.h:118: while((cb = static_cast<CallbackType*>(it->GetNext())) != This can fit on one line. https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h#newc... base/callback_list.h:130: CallbackList() : CallbackListBase<Closure>() {} Remove explicit invocation of base class's default constructor. https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h#newc... base/callback_list.h:136: // static_cast is safe here, all callbacks were added via here, all -> here. All https://codereview.chromium.org/22877038/diff/99001/base/callback_list_intern... File base/callback_list_internal.h (right): https://codereview.chromium.org/22877038/diff/99001/base/callback_list_intern... base/callback_list_internal.h:29: typedef std::vector<base::internal::CallbackBase*> ListType; This typedef can be private. https://codereview.chromium.org/22877038/diff/99001/base/callback_list_intern... base/callback_list_internal.h:57: void AssertEmpty(); This is missing an implementation. https://codereview.chromium.org/22877038/diff/99001/base/callback_list_intern... base/callback_list_internal.h:60: scoped_ptr<CallbackListImpl::Iterator> GetIterator(); What about returning the Iterator by copy like erikwright was saying? I think you'd have to add a copy constructor that also incremented active_iterator_count_. With copy-ellision, it'll probably never get invoked so there's no cost. The upside is you avoid a malloc. https://codereview.chromium.org/22877038/diff/99001/base/callback_list_intern... base/callback_list_internal.h:62: // Compact the list (remove any elements which were nulled out during nulled -> NULLed
https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h#newc... base/callback_list.h:75: base::Closure Add(const CallbackType& cb) WARN_UNUSED_RESULT { On 2013/09/04 23:20:07, awong wrote: > I'm going to slightly disagree with erikwright here...do we want the user to > always handle this return? > > If users of this class don't care to AssertEmpty() before the CallbackList > destruction, it seems odd to force them to explicitly ignore the removal > callback. I think not having the WARN_UNUSED_RESULT is better. AIUI, the general model is publisher lifetime exceeds the listener's lifetime. In that case, when an event is published after a listener is destroyed we will have a use-after-free. Ergo, the client has an obligation to unregister itself upon its destruction. Developers who are not Cait or Albert will not be familiar with this nuance of the API and could easily overlook the necessity of storing and correctly invoking the Closure returned from Add - lovely file comments notwithstanding. As I've mentioned, I actually prefer not returning a Closure but returning a scoped_ptr<Handle> instead. Handle::~Handle will unregister the listener, with the result that negligence will lead to not receiving events. That outcome is much more likely to be noted by developers and tests than a racey use-after-free. WARN_UNUSED_RESULT could and probably should still be applied in that case. The EventRegistrar that Cait is working on would hide these details in any case. Only clients that did their own unsubscription would need to use this.
https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h#newc... base/callback_list.h:75: base::Closure Add(const CallbackType& cb) WARN_UNUSED_RESULT { On 2013/09/05 14:14:31, erikwright wrote: > As I've mentioned, I actually prefer not returning a Closure but returning a > scoped_ptr<Handle> instead. Handle::~Handle will unregister the listener Or a ScopedClosureRunner? Same basic philosophy as returning a scoped_ptr to make ownership clear.
On 2013/09/05 15:03:47, Avi wrote: > https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h > File base/callback_list.h (right): > > https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h#newc... > base/callback_list.h:75: base::Closure Add(const CallbackType& cb) > WARN_UNUSED_RESULT { > On 2013/09/05 14:14:31, erikwright wrote: > > As I've mentioned, I actually prefer not returning a Closure but returning a > > scoped_ptr<Handle> instead. Handle::~Handle will unregister the listener > > Or a ScopedClosureRunner? Same basic philosophy as returning a scoped_ptr to > make ownership clear. That would have the same effect, yes, but with some disadvantages: 1. It's not movable, so you would need to return a scoped_ptr<ScopedClosureRunner> anyway. 2. It's in bind_helpers.h, so you are requiring example_publisher.h to transitively include bind_helpers.h. 3. It requires a Closure, so to make one you need to include bind.h. Since the creation of this "cancel callback" or ScopedClosureRunner is the only thing that requires the use of bind in this implementation, I'd prefer to choose the alternative that doesn't require it. That would really minimize the concerns about template bloat with regards to callback_list_impl.cc, and I believe the implementation would be significantly simpler if we just combined CallbackListImpl with CallbackListBase. In particular, that would negate the need to use CallbackBase, negating the need to add the friend declaration in callback_internal.h.
https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h#newc... base/callback_list.h:75: base::Closure Add(const CallbackType& cb) WARN_UNUSED_RESULT { On 2013/09/05 14:14:31, erikwright wrote: > On 2013/09/04 23:20:07, awong wrote: > > I'm going to slightly disagree with erikwright here...do we want the user to > > always handle this return? > > > > If users of this class don't care to AssertEmpty() before the CallbackList > > destruction, it seems odd to force them to explicitly ignore the removal > > callback. I think not having the WARN_UNUSED_RESULT is better. > > AIUI, the general model is publisher lifetime exceeds the listener's lifetime. > In that case, when an event is published after a listener is destroyed we will > have a use-after-free. > > Ergo, the client has an obligation to unregister itself upon its destruction. I see what you're saying but I'm not completely convinced. Assume we have a class X. In the case where X is refcounted or we grab a WeakPtr<X>, then this is a non issue. What we're worried about is this case: cl.Add(Bind(&X::Foo, base::Unretained(this)); The "Unretained" is already a very strong indicator that lifetimes are a problem. If we have code structures that ensure CallbackList exists the listener, then we're just adding clutter to the call site. I think this code-smell is well known in Chrome (or if it's not...then we have some serious problems). > Developers who are not Cait or Albert will not be familiar with this nuance of > the API and could easily overlook the necessity of storing and correctly > invoking the Closure returned from Add - lovely file comments notwithstanding. > > As I've mentioned, I actually prefer not returning a Closure but returning a > scoped_ptr<Handle> instead. Handle::~Handle will unregister the listener, with > the result that negligence will lead to not receiving events. That outcome is > much more likely to be noted by developers and tests than a racey > use-after-free. WARN_UNUSED_RESULT could and probably should still be applied in > that case. > > The EventRegistrar that Cait is working on would hide these details in any case. > Only clients that did their own unsubscription would need to use this. Hmm...I think this might be where our difference in opinion is coming in. I'm expecting this to be used beyond just replacing Notifications and the question we're deciding is whether it's okay to make it harder for a code pattern where you *know* the CallbackList is destroyed before the listeners. Erik, I can't tell over text so I'm going to ask directly...how strongly do think leaving the API as is, w/o WARN_UNSED_RESULT, is going to cause long term harm in code maintenance? Low, med, high? On this evaluation, I'm in the "low" category...
On 2013/09/05 17:42:17, awong wrote: > https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h > File base/callback_list.h (right): > > https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h#newc... > base/callback_list.h:75: base::Closure Add(const CallbackType& cb) > WARN_UNUSED_RESULT { > On 2013/09/05 14:14:31, erikwright wrote: > > On 2013/09/04 23:20:07, awong wrote: > > > I'm going to slightly disagree with erikwright here...do we want the user to > > > always handle this return? > > > > > > If users of this class don't care to AssertEmpty() before the CallbackList > > > destruction, it seems odd to force them to explicitly ignore the removal > > > callback. I think not having the WARN_UNUSED_RESULT is better. > > > > AIUI, the general model is publisher lifetime exceeds the listener's lifetime. > > In that case, when an event is published after a listener is destroyed we will > > have a use-after-free. > > > > Ergo, the client has an obligation to unregister itself upon its destruction. > > I see what you're saying but I'm not completely convinced. Assume we have a > class X. In the case where X is refcounted or we grab a WeakPtr<X>, then this is > a non issue. What we're worried about is this case: > > cl.Add(Bind(&X::Foo, base::Unretained(this)); > > The "Unretained" is already a very strong indicator that lifetimes are a > problem. If we have code structures that ensure CallbackList exists the > listener, then we're just adding clutter to the call site. > > I think this code-smell is well known in Chrome (or if it's not...then we have > some serious problems). > > > > Developers who are not Cait or Albert will not be familiar with this nuance of > > the API and could easily overlook the necessity of storing and correctly > > invoking the Closure returned from Add - lovely file comments notwithstanding. > > > > As I've mentioned, I actually prefer not returning a Closure but returning a > > scoped_ptr<Handle> instead. Handle::~Handle will unregister the listener, with > > the result that negligence will lead to not receiving events. That outcome is > > much more likely to be noted by developers and tests than a racey > > use-after-free. WARN_UNUSED_RESULT could and probably should still be applied > in > > that case. > > > > The EventRegistrar that Cait is working on would hide these details in any > case. > > Only clients that did their own unsubscription would need to use this. > > Hmm...I think this might be where our difference in opinion is coming in. I'm > expecting this to be used beyond just replacing Notifications and the question > we're deciding is whether it's okay to make it harder for a code pattern where > you *know* the CallbackList is destroyed before the listeners. > > Erik, I can't tell over text so I'm going to ask directly...how strongly do > think leaving the API as is, w/o WARN_UNSED_RESULT, is going to cause long term > harm in code maintenance? Low, med, high? > > On this evaluation, I'm in the "low" category... We had an IM discussion which I will summarize for the record: We all agree that refcounting and weakptr are problematic and, at best, can paper over poorly defined lifetime contracts. We all agree that they will be used anyway by developers, but that it's OK to make this less attractive (i.e., require an explicit waiving of the right to unsubscribe). The main surprise was that Albert intends for this class to be used in cases where listeners outlive publishers and that was not my expectation. I propose the following: 1. Publisher outlives Listener is a very common use-case. 2. If we actually need a callback list for this use-case the callback list we design should strongly promote safety in this use-case. 3. It's possible that the API that does (2) might not be appropriate for the reverse case where Listener outlives Publisher. In fact, I'll go further: If I have a fixed set of listeners, I can just have a single callback that manually delegates to a series of listeners. i.e.: MyClass::MyClass() { sample_observable_object_.reset(new SampleObservableObject(base::Bind(&MyClass::OnSomeEvent, base::Unretained(this)))); } MyClass::OnSomeEvent() { some_other_object_.OnSomeEvent(); and_another_one_.OnSomeEvent(); } If I have an unknown set of listeners, I feel that I probably also have unknown lifetime contracts. Who is enforcing the guarantee that the listeners outlive the publisher? Cait tells me that, to her knowledge, we don't need support for listener-outlives-publisher. If we do, I would prefer to see it implemented with a single callback. If necessary, that callback can delegate to a fixed set of other interested objects. If we absolutely need support for listener-outlives-publisher with a dynamic set of listeners I would want to consider the specific case. Until then I prefer not to support it. And even if we do support it I think it is arguably most correct for it to be a different class/API in order to make the distinction between the two possible lifetime guarantees crystal clear. As to the risk of use-after-free, I personally think that it's guaranteed if we don't force developers to think about unsubscription. Sure, when you and I pass a callback the first thing we are thinking about is how do I know it's released before I'm destroyed, but I know without a doubt that many contributors don't have the same instincts. And such bugs will by definition be racey.
On 2013/09/05 20:07:23, erikwright wrote: > On 2013/09/05 17:42:17, awong wrote: > > https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h > > File base/callback_list.h (right): > > > > > https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h#newc... > > base/callback_list.h:75: base::Closure Add(const CallbackType& cb) > > WARN_UNUSED_RESULT { > > On 2013/09/05 14:14:31, erikwright wrote: > > > On 2013/09/04 23:20:07, awong wrote: > > > > I'm going to slightly disagree with erikwright here...do we want the user > to > > > > always handle this return? > > > > > > > > If users of this class don't care to AssertEmpty() before the CallbackList > > > > destruction, it seems odd to force them to explicitly ignore the removal > > > > callback. I think not having the WARN_UNUSED_RESULT is better. > > > > > > AIUI, the general model is publisher lifetime exceeds the listener's > lifetime. > > > In that case, when an event is published after a listener is destroyed we > will > > > have a use-after-free. > > > > > > Ergo, the client has an obligation to unregister itself upon its > destruction. > > > > I see what you're saying but I'm not completely convinced. Assume we have a > > class X. In the case where X is refcounted or we grab a WeakPtr<X>, then this > is > > a non issue. What we're worried about is this case: > > > > cl.Add(Bind(&X::Foo, base::Unretained(this)); > > > > The "Unretained" is already a very strong indicator that lifetimes are a > > problem. If we have code structures that ensure CallbackList exists the > > listener, then we're just adding clutter to the call site. > > > > I think this code-smell is well known in Chrome (or if it's not...then we have > > some serious problems). > > > > > > > Developers who are not Cait or Albert will not be familiar with this nuance > of > > > the API and could easily overlook the necessity of storing and correctly > > > invoking the Closure returned from Add - lovely file comments > notwithstanding. > > > > > > As I've mentioned, I actually prefer not returning a Closure but returning a > > > scoped_ptr<Handle> instead. Handle::~Handle will unregister the listener, > with > > > the result that negligence will lead to not receiving events. That outcome > is > > > much more likely to be noted by developers and tests than a racey > > > use-after-free. WARN_UNUSED_RESULT could and probably should still be > applied > > in > > > that case. > > > > > > The EventRegistrar that Cait is working on would hide these details in any > > case. > > > Only clients that did their own unsubscription would need to use this. > > > > Hmm...I think this might be where our difference in opinion is coming in. I'm > > expecting this to be used beyond just replacing Notifications and the question > > we're deciding is whether it's okay to make it harder for a code pattern where > > you *know* the CallbackList is destroyed before the listeners. > > > > Erik, I can't tell over text so I'm going to ask directly...how strongly do > > think leaving the API as is, w/o WARN_UNSED_RESULT, is going to cause long > term > > harm in code maintenance? Low, med, high? > > > > On this evaluation, I'm in the "low" category... > > We had an IM discussion which I will summarize for the record: > > We all agree that refcounting and weakptr are problematic and, at best, can > paper over poorly defined lifetime contracts. > > We all agree that they will be used anyway by developers, but that it's OK to > make this less attractive (i.e., require an explicit waiving of the right to > unsubscribe). > > The main surprise was that Albert intends for this class to be used in cases > where listeners outlive publishers and that was not my expectation. > > I propose the following: > > 1. Publisher outlives Listener is a very common use-case. > 2. If we actually need a callback list for this use-case the callback list we > design should strongly promote safety in this use-case. > 3. It's possible that the API that does (2) might not be appropriate for the > reverse case where Listener outlives Publisher. > > In fact, I'll go further: > > If I have a fixed set of listeners, I can just have a single callback that > manually delegates to a series of listeners. i.e.: > > MyClass::MyClass() { > sample_observable_object_.reset(new > SampleObservableObject(base::Bind(&MyClass::OnSomeEvent, > base::Unretained(this)))); > } > > MyClass::OnSomeEvent() { > some_other_object_.OnSomeEvent(); > and_another_one_.OnSomeEvent(); > } > > If I have an unknown set of listeners, I feel that I probably also have unknown > lifetime contracts. Who is enforcing the guarantee that the listeners outlive > the publisher? > > Cait tells me that, to her knowledge, we don't need support for > listener-outlives-publisher. If we do, I would prefer to see it implemented with > a single callback. If necessary, that callback can delegate to a fixed set of > other interested objects. If we absolutely need support for > listener-outlives-publisher with a dynamic set of listeners I would want to > consider the specific case. Until then I prefer not to support it. > > And even if we do support it I think it is arguably most correct for it to be a > different class/API in order to make the distinction between the two possible > lifetime guarantees crystal clear. > > As to the risk of use-after-free, I personally think that it's guaranteed if we > don't force developers to think about unsubscription. Sure, when you and I pass > a callback the first thing we are thinking about is how do I know it's released > before I'm destroyed, but I know without a doubt that many contributors don't > have the same instincts. And such bugs will by definition be racey. Here's a counter proposal. What about default AssertEmpty() in ~CallbackList() and then add a CallbackList::SetAssertEmptyOnDestruction(bool)? Then if someone was the listener-outlives-publisher use case, they can just set that member variable after the CBList's creation. This satisfies the UaF concerns, you don't clutter the call sites, and you don't need a second class that also serves a "list of callbacks" style of semantics. -Albert
https://codereview.chromium.org/22877038/diff/113001/base/callback_list_inter... File base/callback_list_internal.cc (right): https://codereview.chromium.org/22877038/diff/113001/base/callback_list_inter... base/callback_list_internal.cc:73: DCHECK(active_iterator_count_ == 0 && callbacks_.size() == 0); This should be a CHECK(). In production, if this is false, we leave ourselves open to a UaF.
On 2013/09/05 22:34:42, awong wrote: > On 2013/09/05 20:07:23, erikwright wrote: > > On 2013/09/05 17:42:17, awong wrote: > > > https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h > > > File base/callback_list.h (right): > > > > > > > > > https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h#newc... > > > base/callback_list.h:75: base::Closure Add(const CallbackType& cb) > > > WARN_UNUSED_RESULT { > > > On 2013/09/05 14:14:31, erikwright wrote: > > > > On 2013/09/04 23:20:07, awong wrote: > > > > > I'm going to slightly disagree with erikwright here...do we want the > user > > to > > > > > always handle this return? > > > > > > > > > > If users of this class don't care to AssertEmpty() before the > CallbackList > > > > > destruction, it seems odd to force them to explicitly ignore the removal > > > > > callback. I think not having the WARN_UNUSED_RESULT is better. > > > > > > > > AIUI, the general model is publisher lifetime exceeds the listener's > > lifetime. > > > > In that case, when an event is published after a listener is destroyed we > > will > > > > have a use-after-free. > > > > > > > > Ergo, the client has an obligation to unregister itself upon its > > destruction. > > > > > > I see what you're saying but I'm not completely convinced. Assume we have a > > > class X. In the case where X is refcounted or we grab a WeakPtr<X>, then > this > > is > > > a non issue. What we're worried about is this case: > > > > > > cl.Add(Bind(&X::Foo, base::Unretained(this)); > > > > > > The "Unretained" is already a very strong indicator that lifetimes are a > > > problem. If we have code structures that ensure CallbackList exists the > > > listener, then we're just adding clutter to the call site. > > > > > > I think this code-smell is well known in Chrome (or if it's not...then we > have > > > some serious problems). > > > > > > > > > > Developers who are not Cait or Albert will not be familiar with this > nuance > > of > > > > the API and could easily overlook the necessity of storing and correctly > > > > invoking the Closure returned from Add - lovely file comments > > notwithstanding. > > > > > > > > As I've mentioned, I actually prefer not returning a Closure but returning > a > > > > scoped_ptr<Handle> instead. Handle::~Handle will unregister the listener, > > with > > > > the result that negligence will lead to not receiving events. That outcome > > is > > > > much more likely to be noted by developers and tests than a racey > > > > use-after-free. WARN_UNUSED_RESULT could and probably should still be > > applied > > > in > > > > that case. > > > > > > > > The EventRegistrar that Cait is working on would hide these details in any > > > case. > > > > Only clients that did their own unsubscription would need to use this. > > > > > > Hmm...I think this might be where our difference in opinion is coming in. > I'm > > > expecting this to be used beyond just replacing Notifications and the > question > > > we're deciding is whether it's okay to make it harder for a code pattern > where > > > you *know* the CallbackList is destroyed before the listeners. > > > > > > Erik, I can't tell over text so I'm going to ask directly...how strongly do > > > think leaving the API as is, w/o WARN_UNSED_RESULT, is going to cause long > > term > > > harm in code maintenance? Low, med, high? > > > > > > On this evaluation, I'm in the "low" category... > > > > We had an IM discussion which I will summarize for the record: > > > > We all agree that refcounting and weakptr are problematic and, at best, can > > paper over poorly defined lifetime contracts. > > > > We all agree that they will be used anyway by developers, but that it's OK to > > make this less attractive (i.e., require an explicit waiving of the right to > > unsubscribe). > > > > The main surprise was that Albert intends for this class to be used in cases > > where listeners outlive publishers and that was not my expectation. > > > > I propose the following: > > > > 1. Publisher outlives Listener is a very common use-case. > > 2. If we actually need a callback list for this use-case the callback list we > > design should strongly promote safety in this use-case. > > 3. It's possible that the API that does (2) might not be appropriate for the > > reverse case where Listener outlives Publisher. > > > > In fact, I'll go further: > > > > If I have a fixed set of listeners, I can just have a single callback that > > manually delegates to a series of listeners. i.e.: > > > > MyClass::MyClass() { > > sample_observable_object_.reset(new > > SampleObservableObject(base::Bind(&MyClass::OnSomeEvent, > > base::Unretained(this)))); > > } > > > > MyClass::OnSomeEvent() { > > some_other_object_.OnSomeEvent(); > > and_another_one_.OnSomeEvent(); > > } > > > > If I have an unknown set of listeners, I feel that I probably also have > unknown > > lifetime contracts. Who is enforcing the guarantee that the listeners outlive > > the publisher? > > > > Cait tells me that, to her knowledge, we don't need support for > > listener-outlives-publisher. If we do, I would prefer to see it implemented > with > > a single callback. If necessary, that callback can delegate to a fixed set of > > other interested objects. If we absolutely need support for > > listener-outlives-publisher with a dynamic set of listeners I would want to > > consider the specific case. Until then I prefer not to support it. > > > > And even if we do support it I think it is arguably most correct for it to be > a > > different class/API in order to make the distinction between the two possible > > lifetime guarantees crystal clear. > > > > As to the risk of use-after-free, I personally think that it's guaranteed if > we > > don't force developers to think about unsubscription. Sure, when you and I > pass > > a callback the first thing we are thinking about is how do I know it's > released > > before I'm destroyed, but I know without a doubt that many contributors don't > > have the same instincts. And such bugs will by definition be racey. > > Here's a counter proposal. What about default AssertEmpty() in ~CallbackList() > and then add a CallbackList::SetAssertEmptyOnDestruction(bool)? > > Then if someone was the listener-outlives-publisher use case, they can just set > that member variable after the CBList's creation. > > This satisfies the UaF concerns, you don't clutter the call sites, and you don't > need a second class that also serves a "list of callbacks" style of semantics. > > -Albert IIUC, the situation we're trying to avoid here is: Listener registers, listener is destroyed without deregistering, publisher calls the (now defunct) listener causing a use-after-free. Since this can only happen after the listener has been destroyed but before the publisher is destroyed, it seems asserting emptiness in the dtor of the publisher won't buy us much here (as the crash will have already happened). I guess if we reached this point without crashing, it would tell us that there is the potential for a use-after-free to occur, which is better than nothing. In an ideal world we'd be able to assert that deregistration had happened before destroying the listener, but since that's not feasible, it seems like reminding listeners upon registering that they will also have to deregister is reasonable. Also in an ideal world, we'd only do the reminding when AssertEmptyOnDestruction was true, (since listeners must deregister themselves in this case), but again, I'm not sure how feasible that is... -Cait
On Thu, Sep 5, 2013 at 4:27 PM, <caitkp@chromium.org> wrote: > On 2013/09/05 22:34:42, awong wrote: > >> On 2013/09/05 20:07:23, erikwright wrote: >> > On 2013/09/05 17:42:17, awong wrote: >> > > https://codereview.chromium.**org/22877038/diff/99001/base/** >> callback_list.h<https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h> >> > > File base/callback_list.h (right): >> > > >> > > >> > >> > > https://codereview.chromium.**org/22877038/diff/99001/base/** > callback_list.h#newcode75<https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h#newcode75> > >> > > base/callback_list.h:75: base::Closure Add(const CallbackType& cb) >> > > WARN_UNUSED_RESULT { >> > > On 2013/09/05 14:14:31, erikwright wrote: >> > > > On 2013/09/04 23:20:07, awong wrote: >> > > > > I'm going to slightly disagree with erikwright here...do we want >> the >> user >> > to >> > > > > always handle this return? >> > > > > >> > > > > If users of this class don't care to AssertEmpty() before the >> CallbackList >> > > > > destruction, it seems odd to force them to explicitly ignore the >> > removal > >> > > > > callback. I think not having the WARN_UNUSED_RESULT is better. >> > > > >> > > > AIUI, the general model is publisher lifetime exceeds the listener's >> > lifetime. >> > > > In that case, when an event is published after a listener is >> destroyed >> > we > >> > will >> > > > have a use-after-free. >> > > > >> > > > Ergo, the client has an obligation to unregister itself upon its >> > destruction. >> > > >> > > I see what you're saying but I'm not completely convinced. Assume we >> have >> > a > >> > > class X. In the case where X is refcounted or we grab a WeakPtr<X>, >> then >> this >> > is >> > > a non issue. What we're worried about is this case: >> > > >> > > cl.Add(Bind(&X::Foo, base::Unretained(this)); >> > > >> > > The "Unretained" is already a very strong indicator that lifetimes >> are a >> > > problem. If we have code structures that ensure CallbackList exists >> the >> > > listener, then we're just adding clutter to the call site. >> > > >> > > I think this code-smell is well known in Chrome (or if it's >> not...then we >> have >> > > some serious problems). >> > > >> > > >> > > > Developers who are not Cait or Albert will not be familiar with this >> nuance >> > of >> > > > the API and could easily overlook the necessity of storing and >> correctly >> > > > invoking the Closure returned from Add - lovely file comments >> > notwithstanding. >> > > > >> > > > As I've mentioned, I actually prefer not returning a Closure but >> > returning > >> a >> > > > scoped_ptr<Handle> instead. Handle::~Handle will unregister the >> > listener, > >> > with >> > > > the result that negligence will lead to not receiving events. That >> > outcome > >> > is >> > > > much more likely to be noted by developers and tests than a racey >> > > > use-after-free. WARN_UNUSED_RESULT could and probably should still >> be >> > applied >> > > in >> > > > that case. >> > > > >> > > > The EventRegistrar that Cait is working on would hide these details >> in >> > any > >> > > case. >> > > > Only clients that did their own unsubscription would need to use >> this. >> > > >> > > Hmm...I think this might be where our difference in opinion is coming >> in. >> I'm >> > > expecting this to be used beyond just replacing Notifications and the >> question >> > > we're deciding is whether it's okay to make it harder for a code >> pattern >> where >> > > you *know* the CallbackList is destroyed before the listeners. >> > > >> > > Erik, I can't tell over text so I'm going to ask directly...how >> strongly >> > do > >> > > think leaving the API as is, w/o WARN_UNSED_RESULT, is going to cause >> long >> > term >> > > harm in code maintenance? Low, med, high? >> > > >> > > On this evaluation, I'm in the "low" category... >> > >> > We had an IM discussion which I will summarize for the record: >> > >> > We all agree that refcounting and weakptr are problematic and, at best, >> can >> > paper over poorly defined lifetime contracts. >> > >> > We all agree that they will be used anyway by developers, but that it's >> OK >> > to > >> > make this less attractive (i.e., require an explicit waiving of the >> right to >> > unsubscribe). >> > >> > The main surprise was that Albert intends for this class to be used in >> cases >> > where listeners outlive publishers and that was not my expectation. >> > >> > I propose the following: >> > >> > 1. Publisher outlives Listener is a very common use-case. >> > 2. If we actually need a callback list for this use-case the callback >> list >> > we > >> > design should strongly promote safety in this use-case. >> > 3. It's possible that the API that does (2) might not be appropriate >> for the >> > reverse case where Listener outlives Publisher. >> > >> > In fact, I'll go further: >> > >> > If I have a fixed set of listeners, I can just have a single callback >> that >> > manually delegates to a series of listeners. i.e.: >> > >> > MyClass::MyClass() { >> > sample_observable_object_.**reset(new >> > SampleObservableObject(base::**Bind(&MyClass::OnSomeEvent, >> > base::Unretained(this)))); >> > } >> > >> > MyClass::OnSomeEvent() { >> > some_other_object_.**OnSomeEvent(); >> > and_another_one_.OnSomeEvent()**; >> > } >> > >> > If I have an unknown set of listeners, I feel that I probably also have >> unknown >> > lifetime contracts. Who is enforcing the guarantee that the listeners >> > outlive > >> > the publisher? >> > >> > Cait tells me that, to her knowledge, we don't need support for >> > listener-outlives-publisher. If we do, I would prefer to see it >> implemented >> with >> > a single callback. If necessary, that callback can delegate to a fixed >> set >> > of > >> > other interested objects. If we absolutely need support for >> > listener-outlives-publisher with a dynamic set of listeners I would >> want to >> > consider the specific case. Until then I prefer not to support it. >> > >> > And even if we do support it I think it is arguably most correct for it >> to >> > be > >> a >> > different class/API in order to make the distinction between the two >> > possible > >> > lifetime guarantees crystal clear. >> > >> > As to the risk of use-after-free, I personally think that it's >> guaranteed if >> we >> > don't force developers to think about unsubscription. Sure, when you >> and I >> pass >> > a callback the first thing we are thinking about is how do I know it's >> released >> > before I'm destroyed, but I know without a doubt that many contributors >> > don't > >> > have the same instincts. And such bugs will by definition be racey. >> > > Here's a counter proposal. What about default AssertEmpty() in >> > ~CallbackList() > >> and then add a CallbackList::**SetAssertEmptyOnDestruction(**bool)? >> > > Then if someone was the listener-outlives-publisher use case, they can >> just >> > set > >> that member variable after the CBList's creation. >> > > This satisfies the UaF concerns, you don't clutter the call sites, and you >> > don't > >> need a second class that also serves a "list of callbacks" style of >> semantics. >> > > -Albert >> > > IIUC, the situation we're trying to avoid here is: Listener registers, > listener > is destroyed without deregistering, publisher calls the (now defunct) > listener > Right...sorry, I got things flipped around again. This really feels like we're protecting users against bad use Bind() with Unretained(). The use of Unretained() is an indication that you know how to manage lifetimes external to the callback system. mesasge_loop->PostTask(Bind(&X::Foo, base::Unretained(foo))) or any API that takes a callback is equivalently dangerous when used with Unretained(). All of them imply that the something has stored a pointer to your class and you need to understand the lifecycle of that pointer. I'm not sure why CallbackList::Add() requires special consideration. Let me poll another one of the Callback people to see what they think. -Albert causing a use-after-free. Since this can only happen after the listener has > been > destroyed but before the publisher is destroyed, it seems asserting > emptiness in > the dtor of the publisher won't buy us much here (as the crash will have > already > happened). I guess if we reached this point without crashing, it would > tell us > that there is the potential for a use-after-free to occur, which is better > than > nothing. > > In an ideal world we'd be able to assert that deregistration had happened > before > destroying the listener, but since that's not feasible, it seems like > reminding > listeners upon registering that they will also have to deregister is > reasonable. > Also in an ideal world, we'd only do the reminding when > AssertEmptyOnDestruction > was true, (since listeners must deregister themselves in this case), but > again, > I'm not sure how feasible that is... > -Cait > > > https://codereview.chromium.**org/22877038/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Thu, Sep 5, 2013 at 5:57 PM, Albert J. Wong (王重傑) <ajwong@chromium.org>wrote: > > > > On Thu, Sep 5, 2013 at 4:27 PM, <caitkp@chromium.org> wrote: > >> On 2013/09/05 22:34:42, awong wrote: >> >>> On 2013/09/05 20:07:23, erikwright wrote: >>> > On 2013/09/05 17:42:17, awong wrote: >>> > > https://codereview.chromium.**org/22877038/diff/99001/base/** >>> callback_list.h<https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h> >>> > > File base/callback_list.h (right): >>> > > >>> > > >>> > >>> >> >> https://codereview.chromium.**org/22877038/diff/99001/base/** >> callback_list.h#newcode75<https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h#newcode75> >> >>> > > base/callback_list.h:75: base::Closure Add(const CallbackType& cb) >>> > > WARN_UNUSED_RESULT { >>> > > On 2013/09/05 14:14:31, erikwright wrote: >>> > > > On 2013/09/04 23:20:07, awong wrote: >>> > > > > I'm going to slightly disagree with erikwright here...do we want >>> the >>> user >>> > to >>> > > > > always handle this return? >>> > > > > >>> > > > > If users of this class don't care to AssertEmpty() before the >>> CallbackList >>> > > > > destruction, it seems odd to force them to explicitly ignore the >>> >> removal >> >>> > > > > callback. I think not having the WARN_UNUSED_RESULT is better. >>> > > > >>> > > > AIUI, the general model is publisher lifetime exceeds the >>> listener's >>> > lifetime. >>> > > > In that case, when an event is published after a listener is >>> destroyed >>> >> we >> >>> > will >>> > > > have a use-after-free. >>> > > > >>> > > > Ergo, the client has an obligation to unregister itself upon its >>> > destruction. >>> > > >>> > > I see what you're saying but I'm not completely convinced. Assume >>> we have >>> >> a >> >>> > > class X. In the case where X is refcounted or we grab a WeakPtr<X>, >>> then >>> this >>> > is >>> > > a non issue. What we're worried about is this case: >>> > > >>> > > cl.Add(Bind(&X::Foo, base::Unretained(this)); >>> > > >>> > > The "Unretained" is already a very strong indicator that lifetimes >>> are a >>> > > problem. If we have code structures that ensure CallbackList exists >>> the >>> > > listener, then we're just adding clutter to the call site. >>> > > >>> > > I think this code-smell is well known in Chrome (or if it's >>> not...then we >>> have >>> > > some serious problems). >>> > > >>> > > >>> > > > Developers who are not Cait or Albert will not be familiar with >>> this >>> nuance >>> > of >>> > > > the API and could easily overlook the necessity of storing and >>> correctly >>> > > > invoking the Closure returned from Add - lovely file comments >>> > notwithstanding. >>> > > > >>> > > > As I've mentioned, I actually prefer not returning a Closure but >>> >> returning >> >>> a >>> > > > scoped_ptr<Handle> instead. Handle::~Handle will unregister the >>> >> listener, >> >>> > with >>> > > > the result that negligence will lead to not receiving events. That >>> >> outcome >> >>> > is >>> > > > much more likely to be noted by developers and tests than a racey >>> > > > use-after-free. WARN_UNUSED_RESULT could and probably should still >>> be >>> > applied >>> > > in >>> > > > that case. >>> > > > >>> > > > The EventRegistrar that Cait is working on would hide these >>> details in >>> >> any >> >>> > > case. >>> > > > Only clients that did their own unsubscription would need to use >>> this. >>> > > >>> > > Hmm...I think this might be where our difference in opinion is >>> coming in. >>> I'm >>> > > expecting this to be used beyond just replacing Notifications and the >>> question >>> > > we're deciding is whether it's okay to make it harder for a code >>> pattern >>> where >>> > > you *know* the CallbackList is destroyed before the listeners. >>> > > >>> > > Erik, I can't tell over text so I'm going to ask directly...how >>> strongly >>> >> do >> >>> > > think leaving the API as is, w/o WARN_UNSED_RESULT, is going to >>> cause long >>> > term >>> > > harm in code maintenance? Low, med, high? >>> > > >>> > > On this evaluation, I'm in the "low" category... >>> > >>> > We had an IM discussion which I will summarize for the record: >>> > >>> > We all agree that refcounting and weakptr are problematic and, at >>> best, can >>> > paper over poorly defined lifetime contracts. >>> > >>> > We all agree that they will be used anyway by developers, but that >>> it's OK >>> >> to >> >>> > make this less attractive (i.e., require an explicit waiving of the >>> right to >>> > unsubscribe). >>> > >>> > The main surprise was that Albert intends for this class to be used in >>> cases >>> > where listeners outlive publishers and that was not my expectation. >>> > >>> > I propose the following: >>> > >>> > 1. Publisher outlives Listener is a very common use-case. >>> > 2. If we actually need a callback list for this use-case the callback >>> list >>> >> we >> >>> > design should strongly promote safety in this use-case. >>> > 3. It's possible that the API that does (2) might not be appropriate >>> for the >>> > reverse case where Listener outlives Publisher. >>> > >>> > In fact, I'll go further: >>> > >>> > If I have a fixed set of listeners, I can just have a single callback >>> that >>> > manually delegates to a series of listeners. i.e.: >>> > >>> > MyClass::MyClass() { >>> > sample_observable_object_.**reset(new >>> > SampleObservableObject(base::**Bind(&MyClass::OnSomeEvent, >>> > base::Unretained(this)))); >>> > } >>> > >>> > MyClass::OnSomeEvent() { >>> > some_other_object_.**OnSomeEvent(); >>> > and_another_one_.OnSomeEvent()**; >>> > } >>> > >>> > If I have an unknown set of listeners, I feel that I probably also have >>> unknown >>> > lifetime contracts. Who is enforcing the guarantee that the listeners >>> >> outlive >> >>> > the publisher? >>> > >>> > Cait tells me that, to her knowledge, we don't need support for >>> > listener-outlives-publisher. If we do, I would prefer to see it >>> implemented >>> with >>> > a single callback. If necessary, that callback can delegate to a fixed >>> set >>> >> of >> >>> > other interested objects. If we absolutely need support for >>> > listener-outlives-publisher with a dynamic set of listeners I would >>> want to >>> > consider the specific case. Until then I prefer not to support it. >>> > >>> > And even if we do support it I think it is arguably most correct for >>> it to >>> >> be >> >>> a >>> > different class/API in order to make the distinction between the two >>> >> possible >> >>> > lifetime guarantees crystal clear. >>> > >>> > As to the risk of use-after-free, I personally think that it's >>> guaranteed if >>> we >>> > don't force developers to think about unsubscription. Sure, when you >>> and I >>> pass >>> > a callback the first thing we are thinking about is how do I know it's >>> released >>> > before I'm destroyed, but I know without a doubt that many contributors >>> >> don't >> >>> > have the same instincts. And such bugs will by definition be racey. >>> >> >> Here's a counter proposal. What about default AssertEmpty() in >>> >> ~CallbackList() >> >>> and then add a CallbackList::**SetAssertEmptyOnDestruction(**bool)? >>> >> >> Then if someone was the listener-outlives-publisher use case, they can >>> just >>> >> set >> >>> that member variable after the CBList's creation. >>> >> >> This satisfies the UaF concerns, you don't clutter the call sites, and >>> you >>> >> don't >> >>> need a second class that also serves a "list of callbacks" style of >>> semantics. >>> >> >> -Albert >>> >> >> IIUC, the situation we're trying to avoid here is: Listener registers, >> listener >> is destroyed without deregistering, publisher calls the (now defunct) >> listener >> > > Right...sorry, I got things flipped around again. > > This really feels like we're protecting users against bad use Bind() with > Unretained(). The use of Unretained() is an indication that you know how to > manage lifetimes external to the callback system. > > mesasge_loop->PostTask(Bind(&X::Foo, base::Unretained(foo))) > > or any API that takes a callback is equivalently dangerous when used with > Unretained(). All of them imply that the something has stored a pointer to > your class and you need to understand the lifecycle of that pointer. I'm > not sure why CallbackList::Add() requires special consideration. > > Let me poll another one of the Callback people to see what they think. > Bounced it over with akalin@ and piman@. Seems like I'm in the minority on this one. How's this sound: (1) CallbackList::Add() still returns a Closure (2) CallbackList::Add() gains a WARN_UNUSED_RESULT (3) We remove the Clear() and AssertEmpty() from the public API since we're requiring everyone deregister themselves. (4) ~CallbackList always does AssertEmpty() (which can be implemented as a DCHECK) If we start seeing people wanting a CallbackList that can be deleted before everyone unregisters, we can decide them to either remove the WARN_UNUSED_RESIULT or add another API. That sound okay? -Albert > > -Albert > > > > causing a use-after-free. Since this can only happen after the listener >> has been >> destroyed but before the publisher is destroyed, it seems asserting >> emptiness in >> the dtor of the publisher won't buy us much here (as the crash will have >> already >> happened). I guess if we reached this point without crashing, it would >> tell us >> that there is the potential for a use-after-free to occur, which is >> better than >> nothing. >> >> In an ideal world we'd be able to assert that deregistration had happened >> before >> destroying the listener, but since that's not feasible, it seems like >> reminding >> listeners upon registering that they will also have to deregister is >> reasonable. >> Also in an ideal world, we'd only do the reminding when >> AssertEmptyOnDestruction >> was true, (since listeners must deregister themselves in this case), but >> again, >> I'm not sure how feasible that is... > > >> -Cait >> >> >> https://codereview.chromium.**org/22877038/<https://codereview.chromium.org/2... >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
> If we start seeing people wanting a CallbackList that can be deleted before > everyone unregisters, we can decide them to either remove the > WARN_UNUSED_RESIULT or add another API. > > That sound okay? SGTM
> How's this sound: > > (1) CallbackList::Add() still returns a Closure > (2) CallbackList::Add() gains a WARN_UNUSED_RESULT > (3) We remove the Clear() and AssertEmpty() from the public API since > we're requiring everyone deregister themselves. > (4) ~CallbackList always does AssertEmpty() (which can be implemented as > a DCHECK) > > If we start seeing people wanting a CallbackList that can be deleted before > everyone unregisters, we can decide them to either remove the > WARN_UNUSED_RESIULT or add another API. > > That sound okay? I agree with everything except (1). a. The two static casts in callback_list.h are ugly to the point of needing comments asserting their safety. b. By using a handle we no longer need friendship with CallbackBase. c. By using a handle we no longer need bind. d. By making CallbackListImpl a template class that can refer to the derived Callback type we can obviate the need to store pointers to CallbackBase instances and instead store instances of the derived Callback type. e. By storing instances instead of pointers we get rid of the manual memory management in CallbackListImpl. f. A Closure is less self-documenting than a class like "Subscription" or "CallbackList::Handle". g. A Closure can be called an arbitrary number of times whereas a scoped_ptr<Subscription> where the only public method is a virtual destructor cannot possibly be misused (because it can only be destroyed once).
On Fri, Sep 6, 2013 at 8:19 AM, <erikwright@chromium.org> wrote: > How's this sound: >> > > (1) CallbackList::Add() still returns a Closure >> (2) CallbackList::Add() gains a WARN_UNUSED_RESULT >> (3) We remove the Clear() and AssertEmpty() from the public API since >> we're requiring everyone deregister themselves. >> (4) ~CallbackList always does AssertEmpty() (which can be implemented >> as >> a DCHECK) >> > > If we start seeing people wanting a CallbackList that can be deleted >> before >> everyone unregisters, we can decide them to either remove the >> WARN_UNUSED_RESIULT or add another API. >> > > That sound okay? >> > > I agree with everything except (1). > > a. The two static casts in callback_list.h are ugly to the point of needing > comments asserting their safety. b. By using a handle we no longer need friendship with CallbackBase. c. By using a handle we no longer need bind. > d. By making CallbackListImpl a template class that can refer to the > derived > Callback type we can obviate the need to store pointers to CallbackBase > instances and instead store instances of the derived Callback type. e. By storing instances instead of pointers we get rid of the manual memory > management in CallbackListImpl. > f. A Closure is less self-documenting than a class like "Subscription" or > "CallbackList::Handle". g. A Closure can be called an arbitrary number of times whereas a > scoped_ptr<Subscription> where the only public method is a virtual > destructor > cannot possibly be misused (because it can only be destroyed once). > Those are convincing arguments...especially (b) and (g). I'd prefer Subscription over Handle since Handle is extremely generic. Lastly, one more naming quip. This is diverging enough from Google3 CallbackList's behavior that I wonder if we should pick a different name for the class entirely (eg, CallbackRegistry, ListenerRegistry) lest people who transfer in to Chrome be confused. However, I don't feel strongly enough to push on this point. > > > https://codereview.chromium.**org/22877038/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Fri, Sep 6, 2013 at 9:08 AM, Albert J. Wong (王重傑) <ajwong@chromium.org>wrote: > On Fri, Sep 6, 2013 at 8:19 AM, <erikwright@chromium.org> wrote: > >> How's this sound: >>> >> >> (1) CallbackList::Add() still returns a Closure >>> (2) CallbackList::Add() gains a WARN_UNUSED_RESULT >>> (3) We remove the Clear() and AssertEmpty() from the public API since >>> we're requiring everyone deregister themselves. >>> (4) ~CallbackList always does AssertEmpty() (which can be implemented >>> as >>> a DCHECK) >>> >> >> If we start seeing people wanting a CallbackList that can be deleted >>> before >>> everyone unregisters, we can decide them to either remove the >>> WARN_UNUSED_RESIULT or add another API. >>> >> >> That sound okay? >>> >> >> I agree with everything except (1). >> >> a. The two static casts in callback_list.h are ugly to the point of >> needing >> comments asserting their safety. > > b. By using a handle we no longer need friendship with CallbackBase. > > c. By using a handle we no longer need bind. >> d. By making CallbackListImpl a template class that can refer to the >> derived >> Callback type we can obviate the need to store pointers to CallbackBase >> instances and instead store instances of the derived Callback type. > > e. By storing instances instead of pointers we get rid of the manual memory >> management in CallbackListImpl. >> f. A Closure is less self-documenting than a class like "Subscription" or >> "CallbackList::Handle". > > g. A Closure can be called an arbitrary number of times whereas a >> scoped_ptr<Subscription> where the only public method is a virtual >> destructor >> cannot possibly be misused (because it can only be destroyed once). >> > > Those are convincing arguments...especially (b) and (g). I'd prefer > Subscription over Handle since Handle is extremely generic. > > Lastly, one more naming quip. This is diverging enough from Google3 > CallbackList's behavior that I wonder if we should pick a different name > for the class entirely (eg, CallbackRegistry, ListenerRegistry) lest people > who transfer in to Chrome be confused. However, I don't feel strongly > enough to push on this point. > Heck...even SubscriptionList with SubscriptionList::Subscription might might sense. > > >> >> >> https://codereview.chromium.**org/22877038/<https://codereview.chromium.org/2... >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
In summary: (1) CallbackList is now CallbackRegistry (2) CallbackRegistry::Add() returns a scoped_ptr<CallbackHandle>, and will WARN_UNUSED_RESULT (3) Clear() and Remove() are gone. (4) ~CallbackRegistry always does AssertEmpty() (5) CallbackRegistry stores Callback<void(...)>'s not base::internal::CallbackBase ptrs. (6) CallbackListImpl went away, as did the bind() in CallbackRegistryBase https://codereview.chromium.org/22877038/diff/79001/base/callback_list_intern... File base/callback_list_internal.cc (right): https://codereview.chromium.org/22877038/diff/79001/base/callback_list_intern... base/callback_list_internal.cc:82: return base::Bind(&CallbackListImpl::RemoveCallback, Unretained(this), cb); On 2013/09/04 23:20:07, awong wrote: > On 2013/09/04 22:09:25, Cait Phillips wrote: > > On 2013/09/04 18:48:21, awong wrote: > > > Should this be bound to a weakptr? > > I think this is what I had originally, but we'd decided that might encourage > > people to get a bit sloppy with the lifetimes of callback lists and their > > observers. > > Right...hmm...on rereading, I worry that we're going to make it easy to get into > a use-after-free. How about doing this pattern to get the best of both worlds: > > Add a static function like: > > // Use a static function to avoid WeakPtr cancellation behavior > // in Bind(). This allows strict enforcement that the caller > // must use the deregister the callback before the CallbackList is > // deleted. > static void CheckedRemove(const WeakPtr<CallbackListImpl>& list, > base::internal::CallbackBase* cb) { > list->Remove(cb); > } > > Then in Add(), do: > > return base::Bind(&CheckedRemove, weak_factory.GetWeakPtr(), cb); > > > That way the user still has to consider lifetimes, and we don't enter undefined > behavior if they do the wrong thing. (If you think I'm going too far with > enforcing this, let me know). Done. https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h File base/callback_list.h (right): https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h#newc... base/callback_list.h:70: class CallbackListBase { On 2013/09/04 23:20:07, awong wrote: > CallbackListBase should be in base::internal. Done. https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h#newc... base/callback_list.h:109: CallbackList() : CallbackListBase<CallbackType>() {} On 2013/09/04 23:20:07, awong wrote: > There's no need to explicitly call the default constructor of a base class. > This is just a no-op. Done. https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h#newc... base/callback_list.h:113: scoped_ptr<internal::CallbackListImpl::Iterator> it = this->GetIterator(); On 2013/09/04 23:20:07, awong wrote: > indentation. Done. https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h#newc... base/callback_list.h:115: // static_cast is safe here, all callbacks were added via On 2013/09/04 23:20:07, awong wrote: > here, all -> here. All Done. https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h#newc... base/callback_list.h:116: // CallbackListBase::Add and so must be of type On 2013/09/04 23:20:07, awong wrote: > ::Add -> ::Add() Done. https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h#newc... base/callback_list.h:118: while((cb = static_cast<CallbackType*>(it->GetNext())) != On 2013/09/04 23:20:07, awong wrote: > This can fit on one line. Done. https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h#newc... base/callback_list.h:130: CallbackList() : CallbackListBase<Closure>() {} On 2013/09/04 23:20:07, awong wrote: > Remove explicit invocation of base class's default constructor. Done. https://codereview.chromium.org/22877038/diff/99001/base/callback_list.h#newc... base/callback_list.h:136: // static_cast is safe here, all callbacks were added via On 2013/09/04 23:20:07, awong wrote: > here, all -> here. All Done. https://codereview.chromium.org/22877038/diff/99001/base/callback_list_intern... File base/callback_list_internal.h (right): https://codereview.chromium.org/22877038/diff/99001/base/callback_list_intern... base/callback_list_internal.h:29: typedef std::vector<base::internal::CallbackBase*> ListType; On 2013/09/04 23:20:07, awong wrote: > This typedef can be private. Done. https://codereview.chromium.org/22877038/diff/99001/base/callback_list_intern... base/callback_list_internal.h:57: void AssertEmpty(); On 2013/09/04 23:20:07, awong wrote: > This is missing an implementation. Done. https://codereview.chromium.org/22877038/diff/99001/base/callback_list_intern... base/callback_list_internal.h:60: scoped_ptr<CallbackListImpl::Iterator> GetIterator(); On 2013/09/04 23:20:07, awong wrote: > What about returning the Iterator by copy like erikwright was saying? > > I think you'd have to add a copy constructor that also incremented > active_iterator_count_. With copy-ellision, it'll probably never get invoked so > there's no cost. The upside is you avoid a malloc. Done. https://codereview.chromium.org/22877038/diff/99001/base/callback_list_intern... base/callback_list_internal.h:62: // Compact the list (remove any elements which were nulled out during On 2013/09/04 23:20:07, awong wrote: > nulled -> NULLed Done.
mostly small nits. Main concern is CallbackHandle being in its own file as a top-level class. I don't know what other people would do with such an interface and would rather not expose that to base as a whole. https://codereview.chromium.org/22877038/diff/124001/base/callback_handle.h File base/callback_handle.h (right): https://codereview.chromium.org/22877038/diff/124001/base/callback_handle.h#n... base/callback_handle.h:10: class CallbackHandle { Are you expecting to forward declare this class? If not, I don't see a reason to make it top-level and would just nest it inside the CallbackRegistryBase. Also, I think Subscription is a better name. CallbackHandle implies too many things since Callback<> itself is basically a ref-counted handle to the result of a Bind(). https://codereview.chromium.org/22877038/diff/124001/base/callback_registry.h File base/callback_registry.h (right): https://codereview.chromium.org/22877038/diff/124001/base/callback_registry.h... base/callback_registry.h:40: // callback_registry_.Run(foo); I'd rename this to Notify() now. Seems more sensible when used in concert with Registry. https://codereview.chromium.org/22877038/diff/124001/base/callback_registry.h... base/callback_registry.h:74: typedef base::Callback<void(const Details&)> CallbackType; I don't think you need this CallbackType either do you? https://codereview.chromium.org/22877038/diff/124001/base/callback_registry.h... base/callback_registry.h:75: typedef typename base::internal::CallbackRegistryBase<CallbackType>::Iterator You shouldn't need this typedef since Iterator should just be inherited. I also don't think you need the CallbackType typedef here. https://codereview.chromium.org/22877038/diff/124001/base/callback_registry_i... File base/callback_registry_internal.h (right): https://codereview.chromium.org/22877038/diff/124001/base/callback_registry_i... base/callback_registry_internal.h:27: virtual ~CallbackRegistryBase() { No need for virtual since no one should have a pointer to this. Make the destructor protected? https://codereview.chromium.org/22877038/diff/124001/base/callback_registry_i... base/callback_registry_internal.h:42: void AssertEmpty() { Make this protected? https://codereview.chromium.org/22877038/diff/124001/base/callback_registry_i... base/callback_registry_internal.h:47: typedef std::list<CallbackType> ListType; nit: maybe use std::slist? https://codereview.chromium.org/22877038/diff/124001/base/callback_registry_i... base/callback_registry_internal.h:71: if (!list_) I don't think it's possible for list_ to be NULL.
https://codereview.chromium.org/22877038/diff/124001/base/callback_handle.h File base/callback_handle.h (right): https://codereview.chromium.org/22877038/diff/124001/base/callback_handle.h#n... base/callback_handle.h:10: class CallbackHandle { On 2013/09/06 19:03:24, awong wrote: > Are you expecting to forward declare this class? > > If not, I don't see a reason to make it top-level and would just nest it inside > the CallbackRegistryBase. > > Also, I think Subscription is a better name. > > CallbackHandle implies too many things since Callback<> itself is basically a > ref-counted handle to the result of a Bind(). I think it makes sense to have a top-level base-class here. If I have to store handles/subscriptions from several different registries (esp. if I don't want to deregister any of them before my destruction), then it would be easier for them to all be the same base type so I can stick them in a container, whereas if I have one CallbackRegistry<A>::Handle, one CallbackRegistry<B>::Handle etc. I have to hold them all separately.
https://codereview.chromium.org/22877038/diff/124001/base/callback_handle.h File base/callback_handle.h (right): https://codereview.chromium.org/22877038/diff/124001/base/callback_handle.h#n... base/callback_handle.h:10: class CallbackHandle { On 2013/09/06 19:18:37, Cait Phillips wrote: > On 2013/09/06 19:03:24, awong wrote: > > Are you expecting to forward declare this class? > > > > If not, I don't see a reason to make it top-level and would just nest it > inside > > the CallbackRegistryBase. > > > > Also, I think Subscription is a better name. > > > > CallbackHandle implies too many things since Callback<> itself is basically a > > ref-counted handle to the result of a Bind(). > > I think it makes sense to have a top-level base-class here. If I have to store > handles/subscriptions from several different registries (esp. if I don't want to > deregister any of them before my destruction), then it would be easier for them > to all be the same base type so I can stick them in a container, whereas if I > have one CallbackRegistry<A>::Handle, one CallbackRegistry<B>::Handle etc. I > have to hold them all separately. Gotcha. I'd still move it back into callback_registry.h and give it a name that implies it's (a) for use with CallbackRegistry (b) has something to do with deregistering, unsubscribing something from the registry. As a standalone interface with a name like CallbackHandle, it's too generic.
https://codereview.chromium.org/22877038/diff/124001/base/callback_handle.h File base/callback_handle.h (right): https://codereview.chromium.org/22877038/diff/124001/base/callback_handle.h#n... base/callback_handle.h:10: class CallbackHandle { On 2013/09/06 19:31:02, awong wrote: > On 2013/09/06 19:18:37, Cait Phillips wrote: > > On 2013/09/06 19:03:24, awong wrote: > > > Are you expecting to forward declare this class? > > > > > > If not, I don't see a reason to make it top-level and would just nest it > > inside > > > the CallbackRegistryBase. > > > > > > Also, I think Subscription is a better name. > > > > > > CallbackHandle implies too many things since Callback<> itself is basically > a > > > ref-counted handle to the result of a Bind(). > > > > I think it makes sense to have a top-level base-class here. If I have to store > > handles/subscriptions from several different registries (esp. if I don't want > to > > deregister any of them before my destruction), then it would be easier for > them > > to all be the same base type so I can stick them in a container, whereas if I > > have one CallbackRegistry<A>::Handle, one CallbackRegistry<B>::Handle etc. I > > have to hold them all separately. > > Gotcha. > > I'd still move it back into callback_registry.h and give it a name that implies > it's > (a) for use with CallbackRegistry > (b) has something to do with deregistering, unsubscribing something from the > registry. > > As a standalone interface with a name like CallbackHandle, it's too generic. I don't think moving it into callback_registry.h will work (unless we move callbackRegistryBase there as well, as CallbackRegistryBase needs to know about CallbackHandle). My preference would be to keep CallbackHandle as a separate file, but give it a better name (CallbackRegistrySubscription? or just Subscription if that's too long)?
On 2013/09/06 20:18:30, Cait Phillips wrote: > https://codereview.chromium.org/22877038/diff/124001/base/callback_handle.h > File base/callback_handle.h (right): > > https://codereview.chromium.org/22877038/diff/124001/base/callback_handle.h#n... > base/callback_handle.h:10: class CallbackHandle { > On 2013/09/06 19:31:02, awong wrote: > > On 2013/09/06 19:18:37, Cait Phillips wrote: > > > On 2013/09/06 19:03:24, awong wrote: > > > > Are you expecting to forward declare this class? > > > > > > > > If not, I don't see a reason to make it top-level and would just nest it > > > inside > > > > the CallbackRegistryBase. > > > > > > > > Also, I think Subscription is a better name. > > > > > > > > CallbackHandle implies too many things since Callback<> itself is > basically > > a > > > > ref-counted handle to the result of a Bind(). > > > > > > I think it makes sense to have a top-level base-class here. If I have to > store > > > handles/subscriptions from several different registries (esp. if I don't > want > > to > > > deregister any of them before my destruction), then it would be easier for > > them > > > to all be the same base type so I can stick them in a container, whereas if > I > > > have one CallbackRegistry<A>::Handle, one CallbackRegistry<B>::Handle etc. I > > > have to hold them all separately. > > > > Gotcha. > > > > I'd still move it back into callback_registry.h and give it a name that > implies > > it's > > (a) for use with CallbackRegistry > > (b) has something to do with deregistering, unsubscribing something from the > > registry. > > > > As a standalone interface with a name like CallbackHandle, it's too generic. > > I don't think moving it into callback_registry.h will work (unless we move > callbackRegistryBase there as well, as CallbackRegistryBase needs to know about > CallbackHandle). > > My preference would be to keep CallbackHandle as a separate file, but give it a > better name (CallbackRegistrySubscription? or just Subscription if that's too > long)? I'd just move everything into callback_registry.h, but keep namespace internal separation. If you grep for "namespace internal" on base, many systems seem to keep things all together. Look at base/memory/weak_ptr.h base/lazy_instance.h, etc for examples. In this case, since most of the API is in the base class, people will likely need to look at the "internal" file anyways.
Haven't looked at the tests yet. https://codereview.chromium.org/22877038/diff/124001/base/callback_handle.h File base/callback_handle.h (right): https://codereview.chromium.org/22877038/diff/124001/base/callback_handle.h#n... base/callback_handle.h:12: virtual ~CallbackHandle() {} Add an '= 0' here to force this class to be derived. IIRC that requires you to define the method out of line. So you would have: class CallbackHandle { public: virtual ~CallbackHandle() = 0; }; CallbackHandle::~CallbackHandle() {} It would still be correct, IMHO, to define the destructor in the .h file in this case. If you don't do this, you need to mark this class DISALLOW_... and define a default constructor. https://codereview.chromium.org/22877038/diff/124001/base/callback_registry_i... File base/callback_registry_internal.h (right): https://codereview.chromium.org/22877038/diff/124001/base/callback_registry_i... base/callback_registry_internal.h:22: // This class is meant as an implementation detail for CallbackRegistry do not "do" should start a new sentence (be preceded by a "."). https://codereview.chromium.org/22877038/diff/124001/base/callback_registry_i... base/callback_registry_internal.h:32: // the same list more than once. The returned handle should be retained until The warning against adding a callback more than once can be removed. If the client does that, they'll get what they asked for (double notification). https://codereview.chromium.org/22877038/diff/124001/base/callback_registry_i... base/callback_registry_internal.h:33: // the listener wishes to deregister the callback, at which point, deleting The bit about the returned handle is a bit repetitive. How about "The callback will remain registered until the returned CallbackHandle is destroyed, which must occur before the CallbackList is destroyed." https://codereview.chromium.org/22877038/diff/124001/base/callback_registry_i... base/callback_registry_internal.h:42: void AssertEmpty() { On 2013/09/06 19:03:24, awong wrote: > Make this protected? I would actually just inline it in the destructor. In fact, the name is inaccurate and the checks redundant: 1) It asserts both that the list is both empty AND there are no active iterators. 2) The check against iterators is redundant since there may not be any active iterators if the list is empty. I think both checks are useful (to catch internal bugs) but I would create two distinct DCHECKs so that you can actually tell which one fired. And I would check active_iterator_count_ first because, again, it should only be > 0 if callbacks_.size() is also > 0, so we only get additional information if it is checked first. https://codereview.chromium.org/22877038/diff/124001/base/callback_registry_i... base/callback_registry_internal.h:47: typedef std::list<CallbackType> ListType; On 2013/09/06 19:03:24, awong wrote: > nit: maybe use std::slist? That would make removal O(N). https://codereview.chromium.org/22877038/diff/124001/base/callback_registry_i... base/callback_registry_internal.h:65: if (list_ && --list_->active_iterator_count_ == 0) { also here, list_ is never null https://codereview.chromium.org/22877038/diff/124001/base/callback_registry_i... base/callback_registry_internal.h:73: while ((list_iter_ != list_->callbacks_.end()) && (*list_iter_).is_null()) '(*list_iter_).' -> 'list_iter_->' https://codereview.chromium.org/22877038/diff/124001/base/callback_registry_i... base/callback_registry_internal.h:85: remove blank line https://codereview.chromium.org/22877038/diff/124001/base/callback_registry_i... base/callback_registry_internal.h:127: }; DISALLOW...
Drive-by! :) https://codereview.chromium.org/22877038/diff/124001/base/callback_handle.h File base/callback_handle.h (right): https://codereview.chromium.org/22877038/diff/124001/base/callback_handle.h#n... base/callback_handle.h:12: virtual ~CallbackHandle() {} On 2013/09/06 21:01:07, erikwright wrote: > Add an '= 0' here to force this class to be derived. That won't work. Every class needs to have a destructor, so you can't make it pure virtual. If you want to prevent someone from instantiating an object of this class (as opposed to a subclass), make the constructor protected. > IIRC that requires you to define the method out of line. So you would have: > > class CallbackHandle { > public: > virtual ~CallbackHandle() = 0; > }; > > CallbackHandle::~CallbackHandle() {} > > It would still be correct, IMHO, to define the destructor in the .h file in this > case. > > If you don't do this, you need to mark this class DISALLOW_... and define a > default constructor.
I wish there was a way to not have the public API buried under the impl. details, but any effort to declare things in one place and implement in another just seemed to make things more confusing. https://codereview.chromium.org/22877038/diff/124001/base/callback_registry.h File base/callback_registry.h (right): https://codereview.chromium.org/22877038/diff/124001/base/callback_registry.h... base/callback_registry.h:40: // callback_registry_.Run(foo); On 2013/09/06 19:03:24, awong wrote: > I'd rename this to Notify() now. Seems more sensible when used in concert with > Registry. Done. https://codereview.chromium.org/22877038/diff/124001/base/callback_registry.h... base/callback_registry.h:74: typedef base::Callback<void(const Details&)> CallbackType; On 2013/09/06 19:03:24, awong wrote: > I don't think you need this CallbackType either do you? Done. https://codereview.chromium.org/22877038/diff/124001/base/callback_registry.h... base/callback_registry.h:75: typedef typename base::internal::CallbackRegistryBase<CallbackType>::Iterator On 2013/09/06 19:03:24, awong wrote: > You shouldn't need this typedef since Iterator should just be inherited. > > I also don't think you need the CallbackType typedef here. I either need to typedef it or refer to it in full, which is a bit of a mouthful: base::internal::CallbackRegistryBase<Callback<void(const Details&)> >::Iterator. https://codereview.chromium.org/22877038/diff/124001/base/callback_registry_i... File base/callback_registry_internal.h (right): https://codereview.chromium.org/22877038/diff/124001/base/callback_registry_i... base/callback_registry_internal.h:22: // This class is meant as an implementation detail for CallbackRegistry do not On 2013/09/06 21:01:07, erikwright wrote: > "do" should start a new sentence (be preceded by a "."). Done. https://codereview.chromium.org/22877038/diff/124001/base/callback_registry_i... base/callback_registry_internal.h:27: virtual ~CallbackRegistryBase() { On 2013/09/06 19:03:24, awong wrote: > No need for virtual since no one should have a pointer to this. Make the > destructor protected? Done. https://codereview.chromium.org/22877038/diff/124001/base/callback_registry_i... base/callback_registry_internal.h:32: // the same list more than once. The returned handle should be retained until On 2013/09/06 21:01:07, erikwright wrote: > The warning against adding a callback more than once can be removed. > > If the client does that, they'll get what they asked for (double notification). Done. https://codereview.chromium.org/22877038/diff/124001/base/callback_registry_i... base/callback_registry_internal.h:33: // the listener wishes to deregister the callback, at which point, deleting On 2013/09/06 21:01:07, erikwright wrote: > The bit about the returned handle is a bit repetitive. > > How about "The callback will remain registered until the returned CallbackHandle > is destroyed, which must occur before the CallbackList is destroyed." Done. https://codereview.chromium.org/22877038/diff/124001/base/callback_registry_i... base/callback_registry_internal.h:42: void AssertEmpty() { On 2013/09/06 19:03:24, awong wrote: > Make this protected? Done. https://codereview.chromium.org/22877038/diff/124001/base/callback_registry_i... base/callback_registry_internal.h:42: void AssertEmpty() { On 2013/09/06 21:01:07, erikwright wrote: > On 2013/09/06 19:03:24, awong wrote: > > Make this protected? > > I would actually just inline it in the destructor. In fact, the name is > inaccurate and the checks redundant: > > 1) It asserts both that the list is both empty AND there are no active > iterators. > 2) The check against iterators is redundant since there may not be any active > iterators if the list is empty. > > I think both checks are useful (to catch internal bugs) but I would create two > distinct DCHECKs so that you can actually tell which one fired. And I would > check active_iterator_count_ first because, again, it should only be > 0 if > callbacks_.size() is also > 0, so we only get additional information if it is > checked first. Done. https://codereview.chromium.org/22877038/diff/124001/base/callback_registry_i... base/callback_registry_internal.h:65: if (list_ && --list_->active_iterator_count_ == 0) { On 2013/09/06 21:01:07, erikwright wrote: > also here, list_ is never null Done. https://codereview.chromium.org/22877038/diff/124001/base/callback_registry_i... base/callback_registry_internal.h:71: if (!list_) On 2013/09/06 19:03:24, awong wrote: > I don't think it's possible for list_ to be NULL. Done. https://codereview.chromium.org/22877038/diff/124001/base/callback_registry_i... base/callback_registry_internal.h:73: while ((list_iter_ != list_->callbacks_.end()) && (*list_iter_).is_null()) On 2013/09/06 21:01:07, erikwright wrote: > '(*list_iter_).' -> 'list_iter_->' Done. https://codereview.chromium.org/22877038/diff/124001/base/callback_registry_i... base/callback_registry_internal.h:85: On 2013/09/06 21:01:07, erikwright wrote: > remove blank line Done. https://codereview.chromium.org/22877038/diff/124001/base/callback_registry_i... base/callback_registry_internal.h:127: }; On 2013/09/06 21:01:07, erikwright wrote: > DISALLOW... Done.
Yeah...that's the problem with templates. You can't easily extract out the interface. https://codereview.chromium.org/22877038/diff/124001/base/callback_handle.h File base/callback_handle.h (right): https://codereview.chromium.org/22877038/diff/124001/base/callback_handle.h#n... base/callback_handle.h:12: virtual ~CallbackHandle() {} On 2013/09/06 21:08:58, Bernhard Bauer wrote: > On 2013/09/06 21:01:07, erikwright wrote: > > Add an '= 0' here to force this class to be derived. > > That won't work. Every class needs to have a destructor, so you can't make it > pure virtual. If you want to prevent someone from instantiating an object of > this class (as opposed to a subclass), make the constructor protected. I might be wrong, but I thought the pattern was to make the destructor pure virtual while *also* providing the definition. (Pure virtual doesn't imply not concrete definition...) > > IIRC that requires you to define the method out of line. So you would have: > > > > class CallbackHandle { > > public: > > virtual ~CallbackHandle() = 0; > > }; > > > > CallbackHandle::~CallbackHandle() {} > > > > It would still be correct, IMHO, to define the destructor in the .h file in > this > > case. > > > > If you don't do this, you need to mark this class DISALLOW_... and define a > > default constructor. > https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h File base/callback_registry.h (right): https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h... base/callback_registry.h:41: // CallbackRegistry<Foo> callback_registry_(); nit: callback_registry_(); -> callback_registry_; https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h... base/callback_registry.h:62: // scoped_ptr<base::Subscription> foo_callback_handle_; foo_callback_handle_ -> foo_subscription_ https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h... base/callback_registry.h:67: class Subscription { base::Subscription is too general a name but before bikeshedding that....it also occurs to me that if you're returning a scoped_ptr<base::Subscription>, I'm not sure how you're going to store these in a container like you indicated might be useful in a previous response. Are you going to call .release() on all the returned values? Something like: vector<base::Subscription*> v; v.push_back(registry.Add(...).release()); ? More and more, I'm realizing I don't understand all the usage requirements enough. Can you a list of those into the CL description? I've pinged a few other people for what they think, and it keeps coming up that we don't have a centralized list of requirements to sanity check the API semantics against. https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h... base/callback_registry.h:78: // This class is meant as an implementation detail for CallbackRegistry. Do not This sentence is implied by base::internal. Can be dropped if you want. https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h... base/callback_registry.h:160: SubscriptionImpl( nit: The parameters should be able to align with the ( https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h... base/callback_registry.h:191: Callback<void(const Details&)> > { nit: should indent 2 more spaces. https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h... base/callback_registry.h:194: base::Callback<void(const Details&)> >::Iterator Iterator; If removing this doesn't compile, why does it work in CallbackRegistry<void> below? https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h... base/callback_registry.h:194: base::Callback<void(const Details&)> >::Iterator Iterator; You seemed to be able to remove this for CallbackRegistry<void>. Why is it necessary here then?
https://codereview.chromium.org/22877038/diff/124001/base/callback_handle.h File base/callback_handle.h (right): https://codereview.chromium.org/22877038/diff/124001/base/callback_handle.h#n... base/callback_handle.h:12: virtual ~CallbackHandle() {} On 2013/09/06 21:08:58, Bernhard Bauer wrote: > On 2013/09/06 21:01:07, erikwright wrote: > > Add an '= 0' here to force this class to be derived. > > That won't work. Every class needs to have a destructor, so you can't make it > pure virtual. If you want to prevent someone from instantiating an object of > this class (as opposed to a subclass), make the constructor protected. You can define it but still mark it pure virtual: erikwright@argon18:~$ cat test.cc #include <iostream> class A { public: virtual ~A() = 0; }; A::~A() { std::cout << "World." << std::endl; } class B : public A { public: virtual ~B() { std::cout << "Hello "; } }; int main(int argc, char** argv) { B b; } erikwright@argon18:~$ gcc /usr/local/google/home/erikwright/test.cc -lstdc++ erikwright@argon18:~$ ./a.out Hello World. erikwright@argon18:~$ This approach is, IMHO, more correct than the protected constructor because the protected constructor still permits the class itself (for example, via a public static method, or from an instance method invoked via a derived class) to instantiate an instance of itself. With the '= 0' above, it's impossible for anyone, including 'A' to instantiate A. > > > IIRC that requires you to define the method out of line. So you would have: > > > > class CallbackHandle { > > public: > > virtual ~CallbackHandle() = 0; > > }; > > > > CallbackHandle::~CallbackHandle() {} > > > > It would still be correct, IMHO, to define the destructor in the .h file in > this > > case. > > > > If you don't do this, you need to mark this class DISALLOW_... and define a > > default constructor. >
https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h File base/callback_registry.h (right): https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h... base/callback_registry.h:194: base::Callback<void(const Details&)> >::Iterator Iterator; On 2013/09/06 23:49:30, awong wrote: > You seemed to be able to remove this for CallbackRegistry<void>. Why is it > necessary here then? That's because, here, the base class is dependent on a template parameter whereas in the case below it is a specific concrete class whose declaration is known. The compiler, when it parses line 199, needs to know what Iterator is. But whether or not there is a type (Iterator) defined in this scope cannot be known at this time. For example, I could declare a specialization of CallbackRegistryBase<int> that doesn't declare Iterator. Thus the typedef is necessary in order to tell the compiler "there is a type base::internal::CallbackRegistryBase<...>::Iterator and I'm going to refer to it as Iterator henceforth".
https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h File base/callback_registry.h (right): https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h... base/callback_registry.h:67: class Subscription { On 2013/09/06 23:49:30, awong wrote: > base::Subscription is too general a name but before bikeshedding that....it also > occurs to me that if you're returning a scoped_ptr<base::Subscription>, I'm not > sure how you're going to store these in a container like you indicated might be > useful in a previous response. > > Are you going to call .release() on all the returned values? Something like: > > vector<base::Subscription*> v; > v.push_back(registry.Add(...).release()); > > ? > > More and more, I'm realizing I don't understand all the usage requirements > enough. Can you a list of those into the CL description? I've pinged a few > other people for what they think, and it keeps coming up that we don't have a > centralized list of requirements to sanity check the API semantics against. Anyone managing more than one subscription will, in all likelihood, use an EventRegistrar (Cait's got a POC for one lying around somewhere). So that will be the only class that will really worry about grouping these things together. But in any case, we have ScopedVector, and I would assume that we would do something like ScopedVector subscriptions_; ... Callback<void(int)> cb = base::Bind(...); subscriptions_.push_back(some_service.AddXCallback(cb).Pass()); Assuming we add a push_back overload taking scoped_ptr (which seems sensible) I think that makes sense. I forget if Pass() is even required in that case.
https://codereview.chromium.org/22877038/diff/124001/base/callback_handle.h File base/callback_handle.h (right): https://codereview.chromium.org/22877038/diff/124001/base/callback_handle.h#n... base/callback_handle.h:12: virtual ~CallbackHandle() {} On 2013/09/09 16:09:38, erikwright wrote: > On 2013/09/06 21:08:58, Bernhard Bauer wrote: > > On 2013/09/06 21:01:07, erikwright wrote: > > > Add an '= 0' here to force this class to be derived. > > > > That won't work. Every class needs to have a destructor, so you can't make it > > pure virtual. If you want to prevent someone from instantiating an object of > > this class (as opposed to a subclass), make the constructor protected. > > You can define it but still mark it pure virtual: > > erikwright@argon18:~$ cat test.cc > #include <iostream> > > class A { > public: > virtual ~A() = 0; > }; > > A::~A() { > std::cout << "World." << std::endl; > } > > class B : public A { > public: > virtual ~B() { > std::cout << "Hello "; > } > }; > > int main(int argc, char** argv) { > B b; > } > erikwright@argon18:~$ gcc /usr/local/google/home/erikwright/test.cc -lstdc++ > erikwright@argon18:~$ ./a.out > Hello World. > erikwright@argon18:~$ Oh, wow. TIL :) > This approach is, IMHO, more correct than the protected constructor because the > protected constructor still permits the class itself (for example, via a public > static method, or from an instance method invoked via a derived class) to > instantiate an instance of itself. > > With the '= 0' above, it's impossible for anyone, including 'A' to instantiate > A. Well, yes, but usually it's easy to find and look through all the code in A and verify that it doesn't do that. Of course, somebody could go ahead later and change it, but if they can already modify class A, they could simply remove the `= 0` from the destructor as well ;-) > > > > > IIRC that requires you to define the method out of line. So you would have: > > > > > > class CallbackHandle { > > > public: > > > virtual ~CallbackHandle() = 0; > > > }; > > > > > > CallbackHandle::~CallbackHandle() {} > > > > > > It would still be correct, IMHO, to define the destructor in the .h file in > > this > > > case. > > > > > > If you don't do this, you need to mark this class DISALLOW_... and define a > > > default constructor. > > >
https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h File base/callback_registry.h (right): https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h... base/callback_registry.h:67: class Subscription { On 2013/09/09 16:24:34, erikwright wrote: > On 2013/09/06 23:49:30, awong wrote: > > base::Subscription is too general a name but before bikeshedding that....it > also > > occurs to me that if you're returning a scoped_ptr<base::Subscription>, I'm > not > > sure how you're going to store these in a container like you indicated might > be > > useful in a previous response. > > > > Are you going to call .release() on all the returned values? Something like: > > > > vector<base::Subscription*> v; > > v.push_back(registry.Add(...).release()); > > > > ? > > > > More and more, I'm realizing I don't understand all the usage requirements > > enough. Can you a list of those into the CL description? I've pinged a few > > other people for what they think, and it keeps coming up that we don't have a > > centralized list of requirements to sanity check the API semantics against. > > Anyone managing more than one subscription will, in all likelihood, use an > EventRegistrar (Cait's got a POC for one lying around somewhere). So that will > be the only class that will really worry about grouping these things together. > > But in any case, we have ScopedVector, and I would assume that we would do > something like > > ScopedVector subscriptions_; > > ... > > Callback<void(int)> cb = base::Bind(...); > subscriptions_.push_back(some_service.AddXCallback(cb).Pass()); > > Assuming we add a push_back overload taking scoped_ptr (which seems sensible) I > think that makes sense. I forget if Pass() is even required in that case. If there's only one expected user, I am weary of adding a general RAII interface like Subscription into base::. Let me IM the two of you on this. (tangent: Pass() isn't required there since the expression is already an rvalue...however ScopedVector does indeed seem to be missing something that takes the scoped_ptr<> directly.) https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h... base/callback_registry.h:190: : public base::internal::CallbackRegistryBase< base:: is overqualified here. We're already in base, so you should do internal::CallbackRegistryBase. Same comment for all base::internal, and base:: qualifications in this file. https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h... base/callback_registry.h:194: base::Callback<void(const Details&)> >::Iterator Iterator; On 2013/09/09 16:18:01, erikwright wrote: > On 2013/09/06 23:49:30, awong wrote: > > You seemed to be able to remove this for CallbackRegistry<void>. Why is it > > necessary here then? > > That's because, here, the base class is dependent on a template parameter > whereas in the case below it is a specific concrete class whose declaration is > known. > > The compiler, when it parses line 199, needs to know what Iterator is. But > whether or not there is a type (Iterator) defined in this scope cannot be known > at this time. For example, I could declare a specialization of > CallbackRegistryBase<int> that doesn't declare Iterator. > > Thus the typedef is necessary in order to tell the compiler "there is a type > base::internal::CallbackRegistryBase<...>::Iterator and I'm going to refer to it > as Iterator henceforth". Ah right...it's that stupid 2-phase dependant lookup thing with templates. See my new comment at line 199. https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h... base/callback_registry.h:199: Iterator it = this->GetIterator(); There's only one use of the typedef here. At the very least it should be moved out of the public API (no one outside this class should need to declare an Iterator). However, since it's only a single use, it seems more sensible to just make this: typename internal::CallbackRegistryBase<void(const Details&)>::Iterator it = this->GetIterator();
Summary: (1) kill base::Subscription (2) Move it into CallbackRegistry<T>::Subscription w/o virtual destruction (3) Punt on EventRegistry impl discussion (4) Commit. Convert some code (ajwong CCed). Then iterate if it becomes gross. (5) Have a beer. We had a long chat over IM about base::Subscription. My main concern was that base::Subscription was too general to have in the public base:: API. Having something that's effectively "has a virtual destructor" in base might allow modules to create implicitly typing relations for each other. Erik and Cait were concerned that there would be a need to for people to contain subscriptions from multiple CallbackRegistry<T> types and not having some sort of relation in the public API would make that hard. In particular, the eventual "EventRegistry" implementation would need this sort of abstraction. There were some ideas pushed around for allowing EventRegistry (eg, put base::Subscription in base::internal, let EventRegistry create an external SubscriptionAdapter type hierarchy that erased could erase CallbackRegistry<T>::Subscription differences). Decided we needed to see real use cases so punted on this for now. Also discussed making CallbackRegistry<T>::Subscription actually a scoped_ptr<SubscriptionImpl>. Pros: Less verbose when typing. Cons: typedefing a scoped_ptr<> is strange and allows that type to be implicitly used with scoped_ptr<T> APIs. Decided to go with NOT including the scoped_ptr<> in the typedef. Lastly, Erik declare it's beer time. This seemed to be the single contentious point of this CL and brought a sense of warm well being to all involved with the discussion. Indeed...I wonder if more beer might be the key to smoother code reviews and whirled peas. Let me know when things are ready for another pass! -Albert On 2013/09/09 17:19:26, awong wrote: > https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h > File base/callback_registry.h (right): > > https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h... > base/callback_registry.h:67: class Subscription { > On 2013/09/09 16:24:34, erikwright wrote: > > On 2013/09/06 23:49:30, awong wrote: > > > base::Subscription is too general a name but before bikeshedding that....it > > also > > > occurs to me that if you're returning a scoped_ptr<base::Subscription>, I'm > > not > > > sure how you're going to store these in a container like you indicated might > > be > > > useful in a previous response. > > > > > > Are you going to call .release() on all the returned values? Something > like: > > > > > > vector<base::Subscription*> v; > > > v.push_back(registry.Add(...).release()); > > > > > > ? > > > > > > More and more, I'm realizing I don't understand all the usage requirements > > > enough. Can you a list of those into the CL description? I've pinged a few > > > other people for what they think, and it keeps coming up that we don't have > a > > > centralized list of requirements to sanity check the API semantics against. > > > > Anyone managing more than one subscription will, in all likelihood, use an > > EventRegistrar (Cait's got a POC for one lying around somewhere). So that will > > be the only class that will really worry about grouping these things together. > > > > But in any case, we have ScopedVector, and I would assume that we would do > > something like > > > > ScopedVector subscriptions_; > > > > ... > > > > Callback<void(int)> cb = base::Bind(...); > > subscriptions_.push_back(some_service.AddXCallback(cb).Pass()); > > > > Assuming we add a push_back overload taking scoped_ptr (which seems sensible) > I > > think that makes sense. I forget if Pass() is even required in that case. > > If there's only one expected user, I am weary of adding a general RAII interface > like Subscription into base::. Let me IM the two of you on this. > > (tangent: Pass() isn't required there since the expression is already an > rvalue...however ScopedVector does indeed seem to be missing something that > takes the scoped_ptr<> directly.) > > https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h... > base/callback_registry.h:190: : public base::internal::CallbackRegistryBase< > base:: is overqualified here. We're already in base, so you should do > internal::CallbackRegistryBase. > > Same comment for all base::internal, and base:: qualifications in this file. > > https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h... > base/callback_registry.h:194: base::Callback<void(const Details&)> >::Iterator > Iterator; > On 2013/09/09 16:18:01, erikwright wrote: > > On 2013/09/06 23:49:30, awong wrote: > > > You seemed to be able to remove this for CallbackRegistry<void>. Why is it > > > necessary here then? > > > > That's because, here, the base class is dependent on a template parameter > > whereas in the case below it is a specific concrete class whose declaration is > > known. > > > > The compiler, when it parses line 199, needs to know what Iterator is. But > > whether or not there is a type (Iterator) defined in this scope cannot be > known > > at this time. For example, I could declare a specialization of > > CallbackRegistryBase<int> that doesn't declare Iterator. > > > > Thus the typedef is necessary in order to tell the compiler "there is a type > > base::internal::CallbackRegistryBase<...>::Iterator and I'm going to refer to > it > > as Iterator henceforth". > > Ah right...it's that stupid 2-phase dependant lookup thing with templates. > > See my new comment at line 199. > > https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h... > base/callback_registry.h:199: Iterator it = this->GetIterator(); > There's only one use of the typedef here. At the very least it should be moved > out of the public API (no one outside this class should need to declare an > Iterator). > > However, since it's only a single use, it seems more sensible to just make this: > > typename internal::CallbackRegistryBase<void(const Details&)>::Iterator it = > this->GetIterator();
Hi Albert -- PTAL. Subscription is now a nested class of CallbackRegistry, as discussed. https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h File base/callback_registry.h (right): https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h... base/callback_registry.h:41: // CallbackRegistry<Foo> callback_registry_(); On 2013/09/06 23:49:30, awong wrote: > nit: callback_registry_(); -> callback_registry_; Done. https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h... base/callback_registry.h:62: // scoped_ptr<base::Subscription> foo_callback_handle_; On 2013/09/06 23:49:30, awong wrote: > foo_callback_handle_ -> foo_subscription_ Done. https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h... base/callback_registry.h:78: // This class is meant as an implementation detail for CallbackRegistry. Do not On 2013/09/06 23:49:30, awong wrote: > This sentence is implied by base::internal. Can be dropped if you want. Done. https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h... base/callback_registry.h:160: SubscriptionImpl( On 2013/09/06 23:49:30, awong wrote: > nit: The parameters should be able to align with the ( Done. https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h... base/callback_registry.h:190: : public base::internal::CallbackRegistryBase< On 2013/09/09 17:19:28, awong wrote: > base:: is overqualified here. We're already in base, so you should do > internal::CallbackRegistryBase. > > Same comment for all base::internal, and base:: qualifications in this file. Done. https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h... base/callback_registry.h:191: Callback<void(const Details&)> > { On 2013/09/06 23:49:30, awong wrote: > nit: should indent 2 more spaces. Done. https://codereview.chromium.org/22877038/diff/138001/base/callback_registry.h... base/callback_registry.h:199: Iterator it = this->GetIterator(); On 2013/09/09 17:19:28, awong wrote: > There's only one use of the typedef here. At the very least it should be moved > out of the public API (no one outside this class should need to declare an > Iterator). > > However, since it's only a single use, it seems more sensible to just make this: > > typename internal::CallbackRegistryBase<void(const Details&)>::Iterator it = > this->GetIterator(); > Done.
https://codereview.chromium.org/22877038/diff/122001/base/callback_registry.h File base/callback_registry.h (right): https://codereview.chromium.org/22877038/diff/122001/base/callback_registry.h... base/callback_registry.h:73: typedef std::list<CallbackType> ListType; Put this typedef in Subscription? https://codereview.chromium.org/22877038/diff/122001/base/callback_registry.h... base/callback_registry.h:78: typename ListType::iterator iter) nit: indent https://codereview.chromium.org/22877038/diff/122001/base/callback_registry.h... base/callback_registry.h:180: : public base::internal::CallbackRegistryBase< The base:: still seems to be here. https://codereview.chromium.org/22877038/diff/122001/base/callback_registry.h... base/callback_registry.h:184: base::Callback<void(const Details&)> >::Iterator Iterator; Move the iterator typedef into Notify or just directly use the full type for the single declaration?
oops--Looks like a couple of my clean up changes weren't picked up in the last patch. https://codereview.chromium.org/22877038/diff/122001/base/callback_registry.h File base/callback_registry.h (right): https://codereview.chromium.org/22877038/diff/122001/base/callback_registry.h... base/callback_registry.h:73: typedef std::list<CallbackType> ListType; On 2013/09/09 19:30:20, awong wrote: > Put this typedef in Subscription? Done. https://codereview.chromium.org/22877038/diff/122001/base/callback_registry.h... base/callback_registry.h:78: typename ListType::iterator iter) On 2013/09/09 19:30:20, awong wrote: > nit: indent Done. https://codereview.chromium.org/22877038/diff/122001/base/callback_registry.h... base/callback_registry.h:180: : public base::internal::CallbackRegistryBase< On 2013/09/09 19:30:20, awong wrote: > The base:: still seems to be here. Done. https://codereview.chromium.org/22877038/diff/122001/base/callback_registry.h... base/callback_registry.h:184: base::Callback<void(const Details&)> >::Iterator Iterator; On 2013/09/09 19:30:20, awong wrote: > Move the iterator typedef into Notify or just directly use the full type for the > single declaration? Done.
https://codereview.chromium.org/22877038/diff/122001/base/callback_registry.h File base/callback_registry.h (right): https://codereview.chromium.org/22877038/diff/122001/base/callback_registry.h... base/callback_registry.h:57: // void OnFoo(const Foo& foo) { nit: put this in private. https://codereview.chromium.org/22877038/diff/122001/base/callback_registry.h... base/callback_registry.h:67: namespace internal { blank line between namespace and the implementation. https://codereview.chromium.org/22877038/diff/122001/base/callback_registry.h... base/callback_registry.h:68: // Holds the methods shared by all specializations of CallbackRegistry to avoid I'm of the opinion that this comment is unnecessary, but I'll defer to Albert. https://codereview.chromium.org/22877038/diff/122001/base/callback_registry.h... base/callback_registry.h:129: list_iter_++; ++list_iter_; ? https://codereview.chromium.org/22877038/diff/122001/base/callback_registry.h... base/callback_registry.h:133: list_iter_++; ditto? https://codereview.chromium.org/22877038/diff/122001/base/callback_registry.h... base/callback_registry.h:169: remove blank
https://codereview.chromium.org/22877038/diff/122001/base/callback_registry.h File base/callback_registry.h (right): https://codereview.chromium.org/22877038/diff/122001/base/callback_registry.h... base/callback_registry.h:68: // Holds the methods shared by all specializations of CallbackRegistry to avoid On 2013/09/09 19:56:18, erikwright wrote: > I'm of the opinion that this comment is unnecessary, but I'll defer to Albert. Yeah, you're right. Let's remove it too. https://codereview.chromium.org/22877038/diff/46001/base/callback_registry.h File base/callback_registry.h (right): https://codereview.chromium.org/22877038/diff/46001/base/callback_registry.h#... base/callback_registry.h:97: scoped_ptr<Subscription> Add(const CallbackType& cb) WARN_UNUSED_RESULT { Let's whack this WARN_UNUSED_RESULT as well. The scoped_ptr<> behavior is already enough and it doesn't seem common to sprinkle WARN_UNUSED_RESULT everywhere we use a scoped_ptr. https://codereview.chromium.org/22877038/diff/46001/base/callback_registry.h#... base/callback_registry.h:101: return scoped_ptr<Subscription>(new Subscription(this, it)); FYI, you could also do return make_scoped_ptr(new Subscription(this, it)); Both are fine. https://codereview.chromium.org/22877038/diff/46001/base/callback_registry.h#... base/callback_registry.h:145: DCHECK(active_iterator_count_ == 0); DCHECK_EQ? https://codereview.chromium.org/22877038/diff/46001/base/callback_registry.h#... base/callback_registry.h:182: Callback<void(const Details&)> >::Subscription Subscription; Is this needed? I just did a small test case, and I don't believe you need to redeclare it since any use of CallbackRegistry already disambiguates.
https://codereview.chromium.org/22877038/diff/46001/base/callback_registry_un... File base/callback_registry_unittest.cc (right): https://codereview.chromium.org/22877038/diff/46001/base/callback_registry_un... base/callback_registry_unittest.cc:9: #include "base/compiler_specific.h" I think you meant to include basictypes.h for DISALLOW_COPY_AND_ASSIGN. I don't see a need for compiler_specific in this test. https://codereview.chromium.org/22877038/diff/46001/base/callback_registry_un... base/callback_registry_unittest.cc:11: #include "base/memory/weak_ptr.h" no longer needed? https://codereview.chromium.org/22877038/diff/46001/base/callback_registry_un... base/callback_registry_unittest.cc:42: int total_; blank line after. https://codereview.chromium.org/22877038/diff/46001/base/callback_registry_un... base/callback_registry_unittest.cc:132: EXPECT_EQ(20, a.total_); nit: I expected this to be 100 (i.e., I thought by name that MultiplyTotal multiplied the current value by X). https://codereview.chromium.org/22877038/diff/46001/base/callback_registry_un... base/callback_registry_unittest.cc:184: // - If the CallbackNotificationType is |CALLBACKS_NOTIFY_ALL|, the newly added comment no longer relevant https://codereview.chromium.org/22877038/diff/46001/base/callback_registry_un... base/callback_registry_unittest.cc:209: } // namespace base Add test for Empty callback list.
https://codereview.chromium.org/22877038/diff/122001/base/callback_registry.h File base/callback_registry.h (right): https://codereview.chromium.org/22877038/diff/122001/base/callback_registry.h... base/callback_registry.h:57: // void OnFoo(const Foo& foo) { On 2013/09/09 19:56:18, erikwright wrote: > nit: put this in private. Done. https://codereview.chromium.org/22877038/diff/122001/base/callback_registry.h... base/callback_registry.h:67: namespace internal { On 2013/09/09 19:56:18, erikwright wrote: > blank line between namespace and the implementation. Done. https://codereview.chromium.org/22877038/diff/122001/base/callback_registry.h... base/callback_registry.h:68: // Holds the methods shared by all specializations of CallbackRegistry to avoid On 2013/09/09 20:04:54, awong wrote: > On 2013/09/09 19:56:18, erikwright wrote: > > I'm of the opinion that this comment is unnecessary, but I'll defer to Albert. > > Yeah, you're right. Let's remove it too. Done. https://codereview.chromium.org/22877038/diff/122001/base/callback_registry.h... base/callback_registry.h:129: list_iter_++; On 2013/09/09 19:56:18, erikwright wrote: > ++list_iter_; ? Done. https://codereview.chromium.org/22877038/diff/122001/base/callback_registry.h... base/callback_registry.h:133: list_iter_++; On 2013/09/09 19:56:18, erikwright wrote: > ditto? Done. https://codereview.chromium.org/22877038/diff/122001/base/callback_registry.h... base/callback_registry.h:169: On 2013/09/09 19:56:18, erikwright wrote: > remove blank Done. https://codereview.chromium.org/22877038/diff/46001/base/callback_registry.h File base/callback_registry.h (right): https://codereview.chromium.org/22877038/diff/46001/base/callback_registry.h#... base/callback_registry.h:97: scoped_ptr<Subscription> Add(const CallbackType& cb) WARN_UNUSED_RESULT { On 2013/09/09 20:04:55, awong wrote: > Let's whack this WARN_UNUSED_RESULT as well. The scoped_ptr<> behavior is > already enough and it doesn't seem common to sprinkle WARN_UNUSED_RESULT > everywhere we use a scoped_ptr. Done. https://codereview.chromium.org/22877038/diff/46001/base/callback_registry.h#... base/callback_registry.h:145: DCHECK(active_iterator_count_ == 0); On 2013/09/09 20:04:55, awong wrote: > DCHECK_EQ? Done. https://codereview.chromium.org/22877038/diff/46001/base/callback_registry.h#... base/callback_registry.h:182: Callback<void(const Details&)> >::Subscription Subscription; On 2013/09/09 20:04:55, awong wrote: > Is this needed? > > I just did a small test case, and I don't believe you need to redeclare it since > any use of CallbackRegistry already disambiguates. Done. https://codereview.chromium.org/22877038/diff/46001/base/callback_registry_un... File base/callback_registry_unittest.cc (right): https://codereview.chromium.org/22877038/diff/46001/base/callback_registry_un... base/callback_registry_unittest.cc:9: #include "base/compiler_specific.h" On 2013/09/09 20:08:14, erikwright wrote: > I think you meant to include basictypes.h for DISALLOW_COPY_AND_ASSIGN. > > I don't see a need for compiler_specific in this test. Done. https://codereview.chromium.org/22877038/diff/46001/base/callback_registry_un... base/callback_registry_unittest.cc:11: #include "base/memory/weak_ptr.h" On 2013/09/09 20:08:14, erikwright wrote: > no longer needed? Done. https://codereview.chromium.org/22877038/diff/46001/base/callback_registry_un... base/callback_registry_unittest.cc:42: int total_; On 2013/09/09 20:08:14, erikwright wrote: > blank line after. Done. https://codereview.chromium.org/22877038/diff/46001/base/callback_registry_un... base/callback_registry_unittest.cc:132: EXPECT_EQ(20, a.total_); On 2013/09/09 20:08:14, erikwright wrote: > nit: I expected this to be 100 (i.e., I thought by name that MultiplyTotal > multiplied the current value by X). IncrementByMultipleOfScaler? https://codereview.chromium.org/22877038/diff/46001/base/callback_registry_un... base/callback_registry_unittest.cc:184: // - If the CallbackNotificationType is |CALLBACKS_NOTIFY_ALL|, the newly added On 2013/09/09 20:08:14, erikwright wrote: > comment no longer relevant Done. https://codereview.chromium.org/22877038/diff/46001/base/callback_registry_un... base/callback_registry_unittest.cc:209: } // namespace base On 2013/09/09 20:08:14, erikwright wrote: > Add test for Empty callback list. Done.
LGTM. LVGTM. https://codereview.chromium.org/22877038/diff/152001/base/callback_registry.h File base/callback_registry.h (right): https://codereview.chromium.org/22877038/diff/152001/base/callback_registry.h... base/callback_registry.h:58: // void OnFoo(const Foo& foo) { Is it actually correct that this is 'const Foo&' while the CallbackRegistry template parameter is 'Foo'? Maybe that works, I'm not sure. https://codereview.chromium.org/22877038/diff/152001/base/callback_registry.h... base/callback_registry.h:100: return scoped_ptr<Subscription>(new Subscription(this, it)); nit: I'm all for inlining 'it'.
LGTM w/nits from Erik addressed. https://codereview.chromium.org/22877038/diff/152001/base/callback_registry.h File base/callback_registry.h (right): https://codereview.chromium.org/22877038/diff/152001/base/callback_registry.h... base/callback_registry.h:58: // void OnFoo(const Foo& foo) { On 2013/09/10 14:07:09, erikwright wrote: > Is it actually correct that this is 'const Foo&' while the CallbackRegistry > template parameter is 'Foo'? > > Maybe that works, I'm not sure. Yep...the specialization of CallbackRegistryBase is actually CallbackRegistryBase<Callback<void(const Details&)> >. It might have been slightly more clear to have done CallbackRegistryBase as a forward declare + partial specialization. template <typename CallbackType> class CallbackRegistryBase; template <typename Detail> class CallbackRegistryBase<Callback<void(const Detail&)> > { /* What was here previously */ }; I didn't mention it because the difference isn't huge.
https://codereview.chromium.org/22877038/diff/152001/base/callback_registry.h File base/callback_registry.h (right): https://codereview.chromium.org/22877038/diff/152001/base/callback_registry.h... base/callback_registry.h:58: // void OnFoo(const Foo& foo) { On 2013/09/10 14:07:09, erikwright wrote: > Is it actually correct that this is 'const Foo&' while the CallbackRegistry > template parameter is 'Foo'? > > Maybe that works, I'm not sure. This is by design. CallbackRegistry<T> expects callbacks of type Callback<void(const T&)>, except when T is void, then it expects closures. https://codereview.chromium.org/22877038/diff/152001/base/callback_registry.h... base/callback_registry.h:100: return scoped_ptr<Subscription>(new Subscription(this, it)); On 2013/09/10 14:07:09, erikwright wrote: > nit: I'm all for inlining 'it'. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/22877038/156001
Be sure to fix up the CL description before checking in.
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/22877038/156001
Retried try job too often on win7_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/22877038/156001
Message was sent while issue was closed.
Change committed as 222559
Message was sent while issue was closed.
Whoooooo!!!!!!!!!!!!!! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
