Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23514056:
Function-type templated CallbackRegistry (Closed)
|
Created:
7 years, 3 months ago by Cait (Slow) Modified:
7 years, 2 months ago CC:
chromium-reviews, erikwright+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFunction-type templated CallbackRegistry
BUG=268984
R=ajwong@chromium.org, erikwright@chromium.org, jam@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=225184
Patch Set 1 #
Total comments: 5
Patch Set 2 : Remove const refs requirement, sanity tests for all the arities #Patch Set 3 : CallbackParamTraits #
Total comments: 4
Patch Set 4 : #
Total comments: 1
Patch Set 5 : .nc file and existing instances converted #
Total comments: 3
Patch Set 6 : pure merge #Patch Set 7 : typename fix for WIN #Patch Set 8 : Pure merge 2 #
Messages
Total messages: 36 (0 generated)
Hi all, Here is a first pass at a multi-parameter friendly CallbackRegistry. A couple open questions: -Should we still require params to by const refs (const Foo&) or should we let clients use any type they desire? The current way results in things like CallbackRegistry<void(const int&)> which seem a bit odd (see unittests). TODO: -Tests for each arity.
I would *not* require const& on the function signature. Imagine passing a pointer: CallbackRegistry<void(const Foo*&)>. Letting people shoot themselves in the foot with too many copies is probably better. :) As for tests, I would also highly suggest NOT duplicating all tests for each arity. For bind_unittests.cc, I had 1 tests that ensured all arities are supported. After that, I just tested things based on the expect domain of input types. Testing via the domain of possible input types feels the more crucial set of assertions. https://codereview.chromium.org/23514056/diff/1/base/callback_registry.h File base/callback_registry.h (right): https://codereview.chromium.org/23514056/diff/1/base/callback_registry.h#newc... base/callback_registry.h:214: void Notify(const A1& a1) { BTW, forgot to mention...we should explicitly specify behavior for move-only types (eg scoped_ptr), array types, and other such fun. Look at CallbackParamTraits::ForwardType to see the domain of problems that can come up: https://code.google.com/p/chromium/codesearch#chromium/src/base/callback_inte... Move-only types almost certainly should be disallowed since the behavior is non-sensical in the context of multiple callbacks.
Oh, and if we're going add another pump file, we should run a sanity check by darin. He had strong opinions on those things IIRC.
On 2013/09/12 21:17:47, awong wrote: > I would *not* require const& on the function signature. Imagine passing a > pointer: CallbackRegistry<void(const Foo*&)>. Agreed. Developers will expect that they can treat this like Bind.
https://codereview.chromium.org/23514056/diff/1/base/callback_registry.h File base/callback_registry.h (right): https://codereview.chromium.org/23514056/diff/1/base/callback_registry.h#newc... base/callback_registry.h:201: remove blanks https://codereview.chromium.org/23514056/diff/1/base/callback_registry.h#newc... base/callback_registry.h:214: void Notify(const A1& a1) { On 2013/09/12 21:17:47, awong wrote: > BTW, forgot to mention...we should explicitly specify behavior for move-only > types (eg scoped_ptr), array types, and other such fun. Look at > CallbackParamTraits::ForwardType to see the domain of problems that can come up: > > https://code.google.com/p/chromium/codesearch#chromium/src/base/callback_inte... > > Move-only types almost certainly should be disallowed since the behavior is > non-sensical in the context of multiple callbacks. What's the danger here? It will fail to compile, and correctly so, for movable types, right? Line 219 would not find a viable constructor if A1 is scoped_ptr (assuming that we move the forced const&).
On Fri, Sep 13, 2013 at 8:05 AM, <erikwright@chromium.org> wrote: > > https://codereview.chromium.**org/23514056/diff/1/base/** > callback_registry.h<https://codereview.chromium.org/23514056/diff/1/base/callback_registry.h> > File base/callback_registry.h (right): > > https://codereview.chromium.**org/23514056/diff/1/base/** > callback_registry.h#newcode201<https://codereview.chromium.org/23514056/diff/1/base/callback_registry.h#newcode201> > base/callback_registry.h:201: > remove blanks > > > https://codereview.chromium.**org/23514056/diff/1/base/** > callback_registry.h#newcode214<https://codereview.chromium.org/23514056/diff/1/base/callback_registry.h#newcode214> > base/callback_registry.h:214: void Notify(const A1& a1) { > On 2013/09/12 21:17:47, awong wrote: > >> BTW, forgot to mention...we should explicitly specify behavior for >> > move-only > >> types (eg scoped_ptr), array types, and other such fun. Look at >> CallbackParamTraits::**ForwardType to see the domain of problems that >> > can come up: > > > https://code.google.com/p/**chromium/codesearch#chromium/** > src/base/callback_internal.h&**sq=package:chromium&type=cs&l=** > 82&rcl=1378422897<https://code.google.com/p/chromium/codesearch#chromium/src/base/callback_internal.h&sq=package:chromium&type=cs&l=82&rcl=1378422897> > > Move-only types almost certainly should be disallowed since the >> > behavior is > >> non-sensical in the context of multiple callbacks. >> > > What's the danger here? It will fail to compile, and correctly so, for > movable types, right? Line 219 would not find a viable constructor if A1 > is scoped_ptr (assuming that we move the forced const&). > Sorry, I was being unclear. I was just saying that if you end up copying the logic for CallbackParamTraits, you shouldn't bother with the bits for scoped_ptr, etc. > > https://codereview.chromium.**org/23514056/<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.
https://codereview.chromium.org/23514056/diff/1/base/callback_registry.h File base/callback_registry.h (right): https://codereview.chromium.org/23514056/diff/1/base/callback_registry.h#newc... base/callback_registry.h:201: On 2013/09/13 15:05:56, erikwright wrote: > remove blanks Done. https://codereview.chromium.org/23514056/diff/1/base/callback_registry.h#newc... base/callback_registry.h:214: void Notify(const A1& a1) { On 2013/09/13 15:05:56, erikwright wrote: > On 2013/09/12 21:17:47, awong wrote: > > BTW, forgot to mention...we should explicitly specify behavior for move-only > > types (eg scoped_ptr), array types, and other such fun. Look at > > CallbackParamTraits::ForwardType to see the domain of problems that can come > up: > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/callback_inte... > > > > Move-only types almost certainly should be disallowed since the behavior is > > non-sensical in the context of multiple callbacks. > > What's the danger here? It will fail to compile, and correctly so, for movable > types, right? Line 219 would not find a viable constructor if A1 is scoped_ptr > (assuming that we move the forced const&). So will I need to create something similar to CallbackParameterTraits then to ensure that params of different types (refs, ptrs etc), are handled properly (but w/o moveable types support)?
On 2013/09/13 19:07:58, Cait Phillips wrote: > https://codereview.chromium.org/23514056/diff/1/base/callback_registry.h > File base/callback_registry.h (right): > > https://codereview.chromium.org/23514056/diff/1/base/callback_registry.h#newc... > base/callback_registry.h:201: > On 2013/09/13 15:05:56, erikwright wrote: > > remove blanks > > Done. > > https://codereview.chromium.org/23514056/diff/1/base/callback_registry.h#newc... > base/callback_registry.h:214: void Notify(const A1& a1) { > On 2013/09/13 15:05:56, erikwright wrote: > > On 2013/09/12 21:17:47, awong wrote: > > > BTW, forgot to mention...we should explicitly specify behavior for move-only > > > types (eg scoped_ptr), array types, and other such fun. Look at > > > CallbackParamTraits::ForwardType to see the domain of problems that can come > > up: > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/callback_inte... > > > > > > Move-only types almost certainly should be disallowed since the behavior is > > > non-sensical in the context of multiple callbacks. > > > > What's the danger here? It will fail to compile, and correctly so, for movable > > types, right? Line 219 would not find a viable constructor if A1 is scoped_ptr > > (assuming that we move the forced const&). > > So will I need to create something similar to CallbackParameterTraits then to > ensure that params of different types (refs, ptrs etc), are handled properly > (but w/o moveable types support)? Thinking more about it, you probably can just reuse CallbackParameterTraits and the code will still fail to compile for move-only types if you can't invoke CallbackForward. I think the right take away is that we should create unittest (including non-compile ones) to assert the expected input domain to double check these various input type assertions.
On 2013/09/13 19:10:21, awong wrote: > On 2013/09/13 19:07:58, Cait Phillips wrote: > > https://codereview.chromium.org/23514056/diff/1/base/callback_registry.h > > File base/callback_registry.h (right): > > > > > https://codereview.chromium.org/23514056/diff/1/base/callback_registry.h#newc... > > base/callback_registry.h:201: > > On 2013/09/13 15:05:56, erikwright wrote: > > > remove blanks > > > > Done. > > > > > https://codereview.chromium.org/23514056/diff/1/base/callback_registry.h#newc... > > base/callback_registry.h:214: void Notify(const A1& a1) { > > On 2013/09/13 15:05:56, erikwright wrote: > > > On 2013/09/12 21:17:47, awong wrote: > > > > BTW, forgot to mention...we should explicitly specify behavior for > move-only > > > > types (eg scoped_ptr), array types, and other such fun. Look at > > > > CallbackParamTraits::ForwardType to see the domain of problems that can > come > > > up: > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/callback_inte... > > > > > > > > Move-only types almost certainly should be disallowed since the behavior > is > > > > non-sensical in the context of multiple callbacks. > > > > > > What's the danger here? It will fail to compile, and correctly so, for > movable > > > types, right? Line 219 would not find a viable constructor if A1 is > scoped_ptr > > > (assuming that we move the forced const&). > > > > So will I need to create something similar to CallbackParameterTraits then to > > ensure that params of different types (refs, ptrs etc), are handled properly > > (but w/o moveable types support)? > > Thinking more about it, you probably can just reuse CallbackParameterTraits and > the code will still fail to compile for move-only types if you can't invoke > CallbackForward. I think the right take away is that we should create unittest > (including non-compile ones) to assert the expected input domain to double check > these various input type assertions. So it will end up being: void Notify(internal::CallbackParamTraits<A1>::ForwardType a1){ ... cb->Run(a1) // don't invoke CallbackForward here. }
On 2013/09/13 19:21:35, Cait Phillips wrote: > On 2013/09/13 19:10:21, awong wrote: > > On 2013/09/13 19:07:58, Cait Phillips wrote: > > > https://codereview.chromium.org/23514056/diff/1/base/callback_registry.h > > > File base/callback_registry.h (right): > > > > > > > > > https://codereview.chromium.org/23514056/diff/1/base/callback_registry.h#newc... > > > base/callback_registry.h:201: > > > On 2013/09/13 15:05:56, erikwright wrote: > > > > remove blanks > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/23514056/diff/1/base/callback_registry.h#newc... > > > base/callback_registry.h:214: void Notify(const A1& a1) { > > > On 2013/09/13 15:05:56, erikwright wrote: > > > > On 2013/09/12 21:17:47, awong wrote: > > > > > BTW, forgot to mention...we should explicitly specify behavior for > > move-only > > > > > types (eg scoped_ptr), array types, and other such fun. Look at > > > > > CallbackParamTraits::ForwardType to see the domain of problems that can > > come > > > > up: > > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/callback_inte... > > > > > > > > > > Move-only types almost certainly should be disallowed since the behavior > > is > > > > > non-sensical in the context of multiple callbacks. > > > > > > > > What's the danger here? It will fail to compile, and correctly so, for > > movable > > > > types, right? Line 219 would not find a viable constructor if A1 is > > scoped_ptr > > > > (assuming that we move the forced const&). > > > > > > So will I need to create something similar to CallbackParameterTraits then > to > > > ensure that params of different types (refs, ptrs etc), are handled properly > > > (but w/o moveable types support)? > > > > Thinking more about it, you probably can just reuse CallbackParameterTraits > and > > the code will still fail to compile for move-only types if you can't invoke > > CallbackForward. I think the right take away is that we should create > unittest > > (including non-compile ones) to assert the expected input domain to double > check > > these various input type assertions. > > So it will end up being: > void Notify(internal::CallbackParamTraits<A1>::ForwardType a1){ > ... > cb->Run(a1) // don't invoke CallbackForward here. > } I believe so. Test will show a compile failure (or accidental success) really fast...
Albert: PTAL. I added the CallbackParamTraits. Will work on adding some non-compile tests to enforce the ban on moveable types.
https://codereview.chromium.org/23514056/diff/21001/base/callback_registry.h File base/callback_registry.h (right): https://codereview.chromium.org/23514056/diff/21001/base/callback_registry.h#... base/callback_registry.h:182: template <typename Sig> class CallbackRegistry; blank between decl and specialization. https://codereview.chromium.org/23514056/diff/21001/base/callback_registry.h#... base/callback_registry.h:213: void Notify(typename internal::CallbackParamTraits<A1>::ForwardType a1) { It will surprise no one to hear that I'm not a fan of reaching into callback_internal.h here. Suggestions: 1) Duplicate the subset of CallbackParamTraits that we want here. 2) Move CallbackParamTraits (or at least the ForwardType part) to template_util.h . i.e., base::ParameterTraits::ForwardType or base::ParameterForwardingType . 3) Make Callback<A1, ...> expose Callback<A1, ...>::A1ForwardType I prefer (2) personally. I'm OK to see that done in a separate follow-up CL, but I'd like to at least hear ajwong's perspective first.
https://codereview.chromium.org/23514056/diff/21001/base/callback_registry.h File base/callback_registry.h (right): https://codereview.chromium.org/23514056/diff/21001/base/callback_registry.h#... base/callback_registry.h:213: void Notify(typename internal::CallbackParamTraits<A1>::ForwardType a1) { On 2013/09/17 18:21:49, erikwright wrote: > It will surprise no one to hear that I'm not a fan of reaching into > callback_internal.h here. > > Suggestions: > > 1) Duplicate the subset of CallbackParamTraits that we want here. > 2) Move CallbackParamTraits (or at least the ForwardType part) to > template_util.h . i.e., base::ParameterTraits::ForwardType or > base::ParameterForwardingType . > 3) Make Callback<A1, ...> expose Callback<A1, ...>::A1ForwardType > > I prefer (2) personally. > > I'm OK to see that done in a separate follow-up CL, but I'd like to at least > hear ajwong's perspective first. I'm slammed right now between perf and a some outstanding CLs so I can't do the full review but I'll respond to this point. (1) is cute/paste which is worse than reaching into base::internal. (3) is polluting the public API. We could think about (2), but it's an odd set of behavior that really only makes sense if you are wrapping callback...which people should generally avoid doing anyways. Given all this, I think using callback_internal.h is the right solution.
On 2013/09/17 18:31:00, awong wrote: > https://codereview.chromium.org/23514056/diff/21001/base/callback_registry.h > File base/callback_registry.h (right): > > https://codereview.chromium.org/23514056/diff/21001/base/callback_registry.h#... > base/callback_registry.h:213: void Notify(typename > internal::CallbackParamTraits<A1>::ForwardType a1) { > On 2013/09/17 18:21:49, erikwright wrote: > > It will surprise no one to hear that I'm not a fan of reaching into > > callback_internal.h here. > > > > Suggestions: > > > > 1) Duplicate the subset of CallbackParamTraits that we want here. > > 2) Move CallbackParamTraits (or at least the ForwardType part) to > > template_util.h . i.e., base::ParameterTraits::ForwardType or > > base::ParameterForwardingType . > > 3) Make Callback<A1, ...> expose Callback<A1, ...>::A1ForwardType > > > > I prefer (2) personally. > > > > I'm OK to see that done in a separate follow-up CL, but I'd like to at least > > hear ajwong's perspective first. > > I'm slammed right now between perf and a some outstanding CLs so I can't do the > full review but I'll respond to this point. > > (1) is cute/paste which is worse than reaching into base::internal. > (3) is polluting the public API. > > We could think about (2), but it's an odd set of behavior that really only makes > sense if you are wrapping callback...which people should generally avoid doing > anyways. I don't see that people should generally avoid doing it. By definition, it should only be done (in a generic fashion) for very widely applicable patterns, but I think that we should, as a team, be actively seeking out widely applicable patterns that we can boil down to powerful utilities like this one. The behaviour is not, IMHO, really specific to Callback so much as to anyone that wants to wrap a callable thing (function pointer, callback, whatever). > > Given all this, I think using callback_internal.h is the right solution.
On Tue, Sep 17, 2013 at 11:38 AM, <erikwright@chromium.org> wrote: > On 2013/09/17 18:31:00, awong wrote: > >> https://codereview.chromium.**org/23514056/diff/21001/base/** >> callback_registry.h<https://codereview.chromium.org/23514056/diff/21001/base/callback_registry.h> >> File base/callback_registry.h (right): >> > > > https://codereview.chromium.**org/23514056/diff/21001/base/** > callback_registry.h#newcode213<https://codereview.chromium.org/23514056/diff/21001/base/callback_registry.h#newcode213> > >> base/callback_registry.h:213: void Notify(typename >> internal::CallbackParamTraits<**A1>::ForwardType a1) { >> On 2013/09/17 18:21:49, erikwright wrote: >> > It will surprise no one to hear that I'm not a fan of reaching into >> > callback_internal.h here. >> > >> > Suggestions: >> > >> > 1) Duplicate the subset of CallbackParamTraits that we want here. >> > 2) Move CallbackParamTraits (or at least the ForwardType part) to >> > template_util.h . i.e., base::ParameterTraits::**ForwardType or >> > base::ParameterForwardingType . >> > 3) Make Callback<A1, ...> expose Callback<A1, ...>::A1ForwardType >> > >> > I prefer (2) personally. >> > >> > I'm OK to see that done in a separate follow-up CL, but I'd like to at >> least >> > hear ajwong's perspective first. >> > > I'm slammed right now between perf and a some outstanding CLs so I can't >> do >> > the > >> full review but I'll respond to this point. >> > > (1) is cute/paste which is worse than reaching into base::internal. >> (3) is polluting the public API. >> > > We could think about (2), but it's an odd set of behavior that really only >> > makes > >> sense if you are wrapping callback...which people should generally avoid >> doing >> anyways. >> > > I don't see that people should generally avoid doing it. By definition, it > should only be done (in a generic fashion) for very widely applicable > patterns, > but I think that we should, as a team, be actively seeking out widely > applicable > patterns that we can boil down to powerful utilities like this one. > > The behaviour is not, IMHO, really specific to Callback so much as to > anyone > that wants to wrap a callable thing (function pointer, callback, whatever). Wrapping Callback is really hard to get right and is often an anti-pattern since most functionality should be done in the function that is being bound, not on the result of the Bind. If you have concrete use cases, I'd love to hear them. -Albert > Given all this, I think using callback_internal.h is the right solution. >> > > https://codereview.chromium.**org/23514056/<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.
PTAL. I'd like to get this CL in before there are too many uses of CallbackRegistry in the codebase, to avoid having to fix them retroactively. Albert: I have a commented-out non-compile test in callback_registry_unittest.cc, can I just stick it in a callback_registry_unittest.nc file, or is there more to it than that? https://codereview.chromium.org/23514056/diff/21001/base/callback_registry.h File base/callback_registry.h (right): https://codereview.chromium.org/23514056/diff/21001/base/callback_registry.h#... base/callback_registry.h:182: template <typename Sig> class CallbackRegistry; On 2013/09/17 18:21:49, erikwright wrote: > blank between decl and specialization. Done.
LGTM once the test is sorted out.
waiting to hear from other base OWNERS whether they're okay with another pump file. https://codereview.chromium.org/23514056/diff/29001/base/callback_registry_un... File base/callback_registry_unittest.cc (right): https://codereview.chromium.org/23514056/diff/29001/base/callback_registry_un... base/callback_registry_unittest.cc:97: /*class Foo { Look at base_unittests.nc and copy the pattern there. Currently, it's disabled on trunk until someone has time to figure out how to avoid it killing build times, but still better to have it than not.
On 2013/09/20 20:06:25, awong wrote: > waiting to hear from other base OWNERS whether they're okay with another pump > file. > > https://codereview.chromium.org/23514056/diff/29001/base/callback_registry_un... > File base/callback_registry_unittest.cc (right): > > https://codereview.chromium.org/23514056/diff/29001/base/callback_registry_un... > base/callback_registry_unittest.cc:97: /*class Foo { > Look at base_unittests.nc and copy the pattern there. > > Currently, it's disabled on trunk until someone has time to figure out how to > avoid it killing build times, but still better to have it than not. I'm hearing no complains on the pump file...but Darin wants this renamed back to CallbackList. Let's get this in first, and I can do that global rename since the bikeshed was my fault in the first place.
On 2013/09/20 23:58:12, awong wrote: > On 2013/09/20 20:06:25, awong wrote: > > waiting to hear from other base OWNERS whether they're okay with another pump > > file. > > > > > https://codereview.chromium.org/23514056/diff/29001/base/callback_registry_un... > > File base/callback_registry_unittest.cc (right): > > > > > https://codereview.chromium.org/23514056/diff/29001/base/callback_registry_un... > > base/callback_registry_unittest.cc:97: /*class Foo { > > Look at base_unittests.nc and copy the pattern there. > > > > Currently, it's disabled on trunk until someone has time to figure out how to > > avoid it killing build times, but still better to have it than not. > > I'm hearing no complains on the pump file...but Darin wants this renamed back to > CallbackList. > > Let's get this in first, and I can do that global rename since the bikeshed was > my fault in the first place. Send me a ping when you have the .nc file up and I'll take a final look.
On 2013/09/20 23:58:29, awong wrote: > On 2013/09/20 23:58:12, awong wrote: > > On 2013/09/20 20:06:25, awong wrote: > > > waiting to hear from other base OWNERS whether they're okay with another > pump > > > file. > > > > > > > > > https://codereview.chromium.org/23514056/diff/29001/base/callback_registry_un... > > > File base/callback_registry_unittest.cc (right): > > > > > > > > > https://codereview.chromium.org/23514056/diff/29001/base/callback_registry_un... > > > base/callback_registry_unittest.cc:97: /*class Foo { > > > Look at base_unittests.nc and copy the pattern there. > > > > > > Currently, it's disabled on trunk until someone has time to figure out how > to > > > avoid it killing build times, but still better to have it than not. > > > > I'm hearing no complains on the pump file...but Darin wants this renamed back > to > > CallbackList. > > > > Let's get this in first, and I can do that global rename since the bikeshed > was > > my fault in the first place. > > Send me a ping when you have the .nc file up and I'll take a final look. ping :)
LGTM !
https://codereview.chromium.org/23514056/diff/40001/base/callback_registry.h File base/callback_registry.h (right): https://codereview.chromium.org/23514056/diff/40001/base/callback_registry.h#... base/callback_registry.h:37: // scoped_ptr<base::CallbackRegistry<void(const Foo&)>::Subscription> curious: there's no way to replace the "void(const Foo&" here with OnFooCallback?
https://codereview.chromium.org/23514056/diff/40001/base/callback_registry.h File base/callback_registry.h (right): https://codereview.chromium.org/23514056/diff/40001/base/callback_registry.h#... base/callback_registry.h:37: // scoped_ptr<base::CallbackRegistry<void(const Foo&)>::Subscription> On 2013/09/24 02:16:31, jam wrote: > curious: there's no way to replace the "void(const Foo&" here with > OnFooCallback? I could typedef the signature instead of the callback, then define both the cb and the registry in terms of that: typedef void (FooCallbackType)(const Foo&) base::CallbackRegistry<FooCallbackType> ... base::Callback<FooCallbackType> ... I'm worried that the typedef of a f'n signature might just make things more confusing though?
chrome lgtm https://codereview.chromium.org/23514056/diff/40001/base/callback_registry.h File base/callback_registry.h (right): https://codereview.chromium.org/23514056/diff/40001/base/callback_registry.h#... base/callback_registry.h:37: // scoped_ptr<base::CallbackRegistry<void(const Foo&)>::Subscription> On 2013/09/24 14:20:37, Cait Phillips wrote: > On 2013/09/24 02:16:31, jam wrote: > > curious: there's no way to replace the "void(const Foo&" here with > > OnFooCallback? > > I could typedef the signature instead of the callback, then define both the cb > and the registry in terms of that: > typedef void (FooCallbackType)(const Foo&) > > base::CallbackRegistry<FooCallbackType> ... > base::Callback<FooCallbackType> ... > > I'm worried that the typedef of a f'n signature might just make things more > confusing though? I see, yeah I agree that doesn't look more readable. The more similar defining these callbacks is to using Bind, the more familiar it'll be to developers, as well as less confusing.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/23514056/48001
Sorry for I got bad news for ya. Compile failed with a clobber build on win_x64_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_x64_re... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/23514056/44001
Retried try job too often on linux_chromeos for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/23514056/44001
Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/23514056/95001
Retried try job too often on linux_rel for step(s) content_browsertests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/caitkp@chromium.org/23514056/95001
Retried try job too often on linux_aura for step(s) unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
Message was sent while issue was closed.
Committed patchset #8 manually as r225184 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
