Make Callback::Reset() return a copy to support use-cases where Run() ends up modifying |*this|.

Make Callback::Reset() return a copy to support use-cases where Run() ends up modifying |*this|.  Callers can use 
cb.Reset().Run(args...);
to avoid reentrancy-like bugs.

Replace the special-purpose versions of ResetAndRunCB in the media/ codebase
with this more-general facility.

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

[Ami GONE FROM CHROMIUM, 2012-03-17 05:05:12]

Fred/Will: the idiom of ResetAndRun has been important in the media codebase
where we frequently have one callback's Run() enqueue another callback in the
same slot that held the original, and it's important that slot be is_null() by
the time the Run() gets there.  I'd like to make the idiom a first-class member
of Callback instead of having it languish in a media/ header.  WDYT?

https://chromiumcodereview.appspot.com/9717021/diff/5001/base/callback_unitte...
File base/callback_unittest.cc (left):

https://chromiumcodereview.appspot.com/9717021/diff/5001/base/callback_unitte...
base/callback_unittest.cc:14: class HelperObject {
I couldn't find where this was used, so deleted it.

https://chromiumcodereview.appspot.com/9717021/diff/5001/base/callback_unitte...
File base/callback_unittest.cc (right):

https://chromiumcodereview.appspot.com/9717021/diff/5001/base/callback_unitte...
base/callback_unittest.cc:5: #include "base/bind.h"
I suspect the FakeInvoker business below is meant to get around the need for
bind.h but 5m thought failed to clarify how.  Let me know if I should think
about it harder.

[akalin, 2012-03-19 21:53:20]

I think this could come in handy in other places in the code, but I'm not a fan
of it adding n more methods.  I talked about it with Ami, and he wanted to avoid
the whole one-shot vs. repeatable callback thing, but I had another idea; what
about making Reset() return a copy?  Then one could simply do:

c.Reset().Run(foo, bar, baz);

In most cases, if the return value of Reset() is ignored, it should be compiled
away (I hope).

Will, what do you think?

http://codereview.chromium.org/9717021/diff/5001/base/callback_unittest.cc
File base/callback_unittest.cc (right):

http://codereview.chromium.org/9717021/diff/5001/base/callback_unittest.cc#ne...
base/callback_unittest.cc:5: #include "base/bind.h"
On 2012/03/17 05:05:12, Ami Fischman wrote:
> I suspect the FakeInvoker business below is meant to get around the need for
> bind.h but 5m thought failed to clarify how.  Let me know if I should think
> about it harder.

I don't remember why Albert wanted to explicitly avoid #including bind.h here,
but I supposed it's okay.

http://codereview.chromium.org/9717021/diff/7003/base/callback.h.pump
File base/callback.h.pump (right):

http://codereview.chromium.org/9717021/diff/7003/base/callback.h.pump#newcode47
base/callback.h.pump:47: // If a Callback variable must be empty during the
Run() of a previous value of
I couldn't parse this comment.  Maybe something like:

// ResetAndRun() is like Run(), but the callback variable is reset before it's
run.  This is handy if Run() reads/writes the callback variable.

?

http://codereview.chromium.org/9717021/diff/7003/base/callback.h.pump#newcode299
base/callback.h.pump:299: 
remove extra space

http://codereview.chromium.org/9717021/diff/7003/base/callback_unittest.cc
File base/callback_unittest.cc (right):

http://codereview.chromium.org/9717021/diff/7003/base/callback_unittest.cc#ne...
base/callback_unittest.cc:126: TestForReentrancy() {
constructor list syntax?

http://codereview.chromium.org/9717021/diff/7003/base/callback_unittest.cc#ne...
base/callback_unittest.cc:131: CHECK(cb.is_null());
ASSERT_TRUE

[Ami GONE FROM CHROMIUM, 2012-03-20 13:02:41]

On Mon, Mar 19, 2012 at 10:53 PM, <akalin@chromium.org> wrote:

> I think this could come in handy in other places in the code, but I'm not
> a fan
> of it adding n more methods.  I talked about it with Ami, and he wanted to
> avoid
> the whole one-shot vs. repeatable callback thing, but I had another idea;
> what
> about making Reset() return a copy?  Then one could simply do:
> c.Reset().Run(foo, bar, baz);
>

That would only work if Reset() moved down into Callback<> (out of
CallbackBase), at which point it's unclear to me that it's better than
ResetAndRun().
(I don't mind making that change if you/Will think it's worthy, but it's
not the slam dunk I thought it was before I realized this)


> http://codereview.chromium.**org/9717021/diff/7003/base/**
>
callback.h.pump#newcode47<http://codereview.chromium.org/9717021/diff/7003/base/callback.h.pump#newcode47>
> base/callback.h.pump:47: // If a Callback variable must be empty during
> the Run() of a previous value of
> I couldn't parse this comment.  Maybe something like:
> // ResetAndRun() is like Run(), but the callback variable is reset
> before it's run.  This is handy if Run() reads/writes the callback
> variable.
>

Changed.


> http://codereview.chromium.**org/9717021/diff/7003/base/**
>
callback.h.pump#newcode299<http://codereview.chromium.org/9717021/diff/7003/base/callback.h.pump#newcode299>
> base/callback.h.pump:299:
> remove extra space
>

Done.


> http://codereview.chromium.**org/9717021/diff/7003/base/**
>
callback_unittest.cc#**newcode126<http://codereview.chromium.org/9717021/diff/7003/base/callback_unittest.cc#newcode126>
> base/callback_unittest.cc:126: TestForReentrancy() {
> constructor list syntax?
>

Done.


> http://codereview.chromium.**org/9717021/diff/7003/base/**
>
callback_unittest.cc#**newcode131<http://codereview.chromium.org/9717021/diff/7003/base/callback_unittest.cc#newcode131>
> base/callback_unittest.cc:131: CHECK(cb.is_null());
> ASSERT_TRUE
>

Done.

[willchan no longer on Chromium, 2012-03-20 17:06:45]

I'm hesitant due to having to add the method to each of the Callback classes.
Also, it doesn't seem like this needs to be in the Callback implementation
itself. Is there a reason it's better done here than in a callback_util.h?

On 2012/03/20 13:02:41, Ami Fischman wrote:
> On Mon, Mar 19, 2012 at 10:53 PM, <mailto:akalin@chromium.org> wrote:
> 
> > I think this could come in handy in other places in the code, but I'm not
> > a fan
> > of it adding n more methods.  I talked about it with Ami, and he wanted to
> > avoid
> > the whole one-shot vs. repeatable callback thing, but I had another idea;
> > what
> > about making Reset() return a copy?  Then one could simply do:
> > c.Reset().Run(foo, bar, baz);
> >
> 
> That would only work if Reset() moved down into Callback<> (out of
> CallbackBase), at which point it's unclear to me that it's better than
> ResetAndRun().
> (I don't mind making that change if you/Will think it's worthy, but it's
> not the slam dunk I thought it was before I realized this)
> 
> 
> > http://codereview.chromium.**org/9717021/diff/7003/base/**
> >
>
callback.h.pump#newcode47<http://codereview.chromium.org/9717021/diff/7003/base/callback.h.pump#newcode47>
> > base/callback.h.pump:47: // If a Callback variable must be empty during
> > the Run() of a previous value of
> > I couldn't parse this comment.  Maybe something like:
> > // ResetAndRun() is like Run(), but the callback variable is reset
> > before it's run.  This is handy if Run() reads/writes the callback
> > variable.
> >
> 
> Changed.
> 
> 
> > http://codereview.chromium.**org/9717021/diff/7003/base/**
> >
>
callback.h.pump#newcode299<http://codereview.chromium.org/9717021/diff/7003/base/callback.h.pump#newcode299>
> > base/callback.h.pump:299:
> > remove extra space
> >
> 
> Done.
> 
> 
> > http://codereview.chromium.**org/9717021/diff/7003/base/**
> >
>
callback_unittest.cc#**newcode126<http://codereview.chromium.org/9717021/diff/7003/base/callback_unittest.cc#newcode126>
> > base/callback_unittest.cc:126: TestForReentrancy() {
> > constructor list syntax?
> >
> 
> Done.
> 
> 
> > http://codereview.chromium.**org/9717021/diff/7003/base/**
> >
>
callback_unittest.cc#**newcode131<http://codereview.chromium.org/9717021/diff/7003/base/callback_unittest.cc#newcode131>
> > base/callback_unittest.cc:131: CHECK(cb.is_null());
> > ASSERT_TRUE
> >
> 
> Done.

[willchan no longer on Chromium, 2012-03-20 17:39:24]

Oh, and I'd rather expose Reset() to Callback than have a ResetAndRun() in
Callback.

On 2012/03/20 17:06:45, willchan wrote:
> I'm hesitant due to having to add the method to each of the Callback classes.
> Also, it doesn't seem like this needs to be in the Callback implementation
> itself. Is there a reason it's better done here than in a callback_util.h?
> 
> On 2012/03/20 13:02:41, Ami Fischman wrote:
> > On Mon, Mar 19, 2012 at 10:53 PM, <mailto:akalin@chromium.org> wrote:
> > 
> > > I think this could come in handy in other places in the code, but I'm not
> > > a fan
> > > of it adding n more methods.  I talked about it with Ami, and he wanted to
> > > avoid
> > > the whole one-shot vs. repeatable callback thing, but I had another idea;
> > > what
> > > about making Reset() return a copy?  Then one could simply do:
> > > c.Reset().Run(foo, bar, baz);
> > >
> > 
> > That would only work if Reset() moved down into Callback<> (out of
> > CallbackBase), at which point it's unclear to me that it's better than
> > ResetAndRun().
> > (I don't mind making that change if you/Will think it's worthy, but it's
> > not the slam dunk I thought it was before I realized this)
> > 
> > 
> > > http://codereview.chromium.**org/9717021/diff/7003/base/**
> > >
> >
>
callback.h.pump#newcode47<http://codereview.chromium.org/9717021/diff/7003/base/callback.h.pump#newcode47>
> > > base/callback.h.pump:47: // If a Callback variable must be empty during
> > > the Run() of a previous value of
> > > I couldn't parse this comment.  Maybe something like:
> > > // ResetAndRun() is like Run(), but the callback variable is reset
> > > before it's run.  This is handy if Run() reads/writes the callback
> > > variable.
> > >
> > 
> > Changed.
> > 
> > 
> > > http://codereview.chromium.**org/9717021/diff/7003/base/**
> > >
> >
>
callback.h.pump#newcode299<http://codereview.chromium.org/9717021/diff/7003/base/callback.h.pump#newcode299>
> > > base/callback.h.pump:299:
> > > remove extra space
> > >
> > 
> > Done.
> > 
> > 
> > > http://codereview.chromium.**org/9717021/diff/7003/base/**
> > >
> >
>
callback_unittest.cc#**newcode126<http://codereview.chromium.org/9717021/diff/7003/base/callback_unittest.cc#newcode126>
> > > base/callback_unittest.cc:126: TestForReentrancy() {
> > > constructor list syntax?
> > >
> > 
> > Done.
> > 
> > 
> > > http://codereview.chromium.**org/9717021/diff/7003/base/**
> > >
> >
>
callback_unittest.cc#**newcode131<http://codereview.chromium.org/9717021/diff/7003/base/callback_unittest.cc#newcode131>
> > > base/callback_unittest.cc:131: CHECK(cb.is_null());
> > > ASSERT_TRUE
> > >
> > 
> > Done.

[Ami GONE FROM CHROMIUM, 2012-03-20 19:40:12]

>
> Oh, and I'd rather expose Reset() to Callback than have a ResetAndRun() in
> Callback.
>

Ok, uploaded a version of the patch that does this (and updated CL
description).


> I'm hesitant due to having to add the method to each of the Callback
> classes.
>
>> Also, it doesn't seem like this needs to be in the Callback implementation
> itself. Is there a reason it's better done here than in a callback_util.h?
>

If I'm following you then I think that'd require another pump file, which
seems like a bit much for this, no?

[akalin, 2012-03-21 00:51:52]

> If I'm following you then I think that'd require another pump file, which
> seems like a bit much for this, no?

I don't think so, if you keep foo.Reset() as it is and define a separate helper
function.  Not sure what to call it: base::Reset()?  base::Pass()? 
base::Move()?  e.g.

template <typename T> Callback<T> Move(Callback<T>& callback) {
  using std::swap;
  Callback<T> tmp;
  swap(callback, tmp);
  return tmp;
}

So you can do:

base::Move(foo_cb).Run(bar, baz);

WDYT?

I'm liking this current patch set better than the previous one, also.

[Jeffrey Yasskin (google), 2012-03-21 02:21:07]

On 2012/03/21 00:51:52, akalin wrote:
> > If I'm following you then I think that'd require another pump file, which
> > seems like a bit much for this, no?
> 
> I don't think so, if you keep foo.Reset() as it is and define a separate
helper
> function.  Not sure what to call it: base::Reset()?  base::Pass()? 
> base::Move()?  e.g.
> 
> template <typename T> Callback<T> Move(Callback<T>& callback) {
>   using std::swap;
>   Callback<T> tmp;
>   swap(callback, tmp);
>   return tmp;
> }
> 
> So you can do:
> 
> base::Move(foo_cb).Run(bar, baz);
> 
> WDYT?

By analogy to std::atomic_exchange:

template <typename T> T exchange(T* obj, T value=T()) {
  using std::swap;
  T result;  // Lets NRVO work.
  swap(result, value);
  swap(*obj, result);
  return result;
}

In C++11 this can be simplified to:

template <typename T> T exchange(T* obj, T value=T()) {
  using std::swap;
  swap(*obj, value);
  return value;
}


The default argument is optional.

[Ami GONE FROM CHROMIUM, 2012-03-21 04:13:30]

>
> So you can do:
> base::Move(foo_cb).Run(bar, baz);
> WDYT?
>

I don't like it but my objection is purely aesthetic:
- It relies on pass-by-non-const-ref which would be a special-case in
chromium code
- It needs that b/c it's a free function whose only (practical) use will be
dealing w/ Callbacks, but it doesn't have Callback in the name or any other
scoping to imply it.
(the first objection is obviously much higher-order than the second)
The first objection could be resolved by passing a pointer instead but I
still think it's an ugly result.
I think Jeffrey's analogy to c++11 is very apt; it demonstrates how alien
this approach is to the chromium codebase (even though it might be the
future, it's not the present yet), and I'd wager fewer than 10 chromium
devs could describe "move semantics" in enough detail to parse a
use-example comment on the API.

However my desire to move this code out of media/ is greater than my desire
to impose my sense of aesthetics here.  I'll do whatever you/Will will LGTM
:)

[akalin, 2012-03-22 23:13:23]

On 2012/03/21 04:13:30, Ami Fischman wrote:
> >
> > So you can do:
> > base::Move(foo_cb).Run(bar, baz);
> > WDYT?
> >
> 
> I don't like it but my objection is purely aesthetic:
> - It relies on pass-by-non-const-ref which would be a special-case in
> chromium code
> - It needs that b/c it's a free function whose only (practical) use will be
> dealing w/ Callbacks, but it doesn't have Callback in the name or any other
> scoping to imply it.
> (the first objection is obviously much higher-order than the second)
> The first objection could be resolved by passing a pointer instead but I
> still think it's an ugly result.
> I think Jeffrey's analogy to c++11 is very apt; it demonstrates how alien
> this approach is to the chromium codebase (even though it might be the
> future, it's not the present yet), and I'd wager fewer than 10 chromium
> devs could describe "move semantics" in enough detail to parse a
> use-example comment on the API.
> 
> However my desire to move this code out of media/ is greater than my desire
> to impose my sense of aesthetics here.  I'll do whatever you/Will will LGTM
> :)

I think we can solve your objections by passing a pointer, and naming it
something other than 'Move'.  What about:

base::ResetAndReturn(&foo_cb).Run(bar, baz);

? (cue bikeshed)

It would still be type-agnostic (just swap with a default-initialized T).  I
prefer this over adding a methods to Callback because that would incur a cost
per Callback instantation, whereas having it as a free function means it only
incurs a cost when it's used.

willchan, wdyt?

[willchan no longer on Chromium, 2012-03-22 23:53:31]

Sorry for delaying you so long Ami, one last round before we make a call on
this.

* I don't way to rely on T&.
* I'd _prefer_ not to add more template instantiation costs to
base::Callback.
* I prefer base::Callback<T>::Reset() to base::Callback<T>::ResetAndRun().
* I prefer base::Exchange(T*, T) to base::Move(T&), and probably to
base::Move(T*) too. Exchange() is actually a useful concept and I could see
it being used elsewhere. I dunno how I feel about the default argument
though. I'm inclined to say no.
* I can imagine us having more base::Callback utility functions that do
stuff that isn't "core", but is still useful, like what Ami's done here. I
think I'd be in support of a base::ResetCallbackAndRun(base::Callback<T>*
cb) in a callback_util.h file.

Those are my rough thoughts here. Feel free to call me out on being
stupid/wrong on any of them.

On Thu, Mar 22, 2012 at 4:13 PM, <akalin@chromium.org> wrote:

> On 2012/03/21 04:13:30, Ami Fischman wrote:
>
>> >
>> > So you can do:
>> > base::Move(foo_cb).Run(bar, baz);
>> > WDYT?
>> >
>>
>
>  I don't like it but my objection is purely aesthetic:
>> - It relies on pass-by-non-const-ref which would be a special-case in
>> chromium code
>> - It needs that b/c it's a free function whose only (practical) use will
>> be
>> dealing w/ Callbacks, but it doesn't have Callback in the name or any
>> other
>> scoping to imply it.
>> (the first objection is obviously much higher-order than the second)
>> The first objection could be resolved by passing a pointer instead but I
>> still think it's an ugly result.
>> I think Jeffrey's analogy to c++11 is very apt; it demonstrates how alien
>> this approach is to the chromium codebase (even though it might be the
>> future, it's not the present yet), and I'd wager fewer than 10 chromium
>> devs could describe "move semantics" in enough detail to parse a
>> use-example comment on the API.
>>
>
>  However my desire to move this code out of media/ is greater than my
>> desire
>> to impose my sense of aesthetics here.  I'll do whatever you/Will will
>> LGTM
>> :)
>>
>
> I think we can solve your objections by passing a pointer, and naming it
> something other than 'Move'.  What about:
>
> base::ResetAndReturn(&foo_cb).**Run(bar, baz);
>
> ? (cue bikeshed)
>
> It would still be type-agnostic (just swap with a default-initialized T).
>  I
> prefer this over adding a methods to Callback because that would incur a
> cost
> per Callback instantation, whereas having it as a free function means it
> only
> incurs a cost when it's used.
>
> willchan, wdyt?
>
>
http://codereview.chromium.**org/9717021/<http://codereview.chromium.org/9717...
>

[akalin, 2012-03-22 23:56:09]

On Thu, Mar 22, 2012 at 4:51 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> Sorry for delaying you so long Ami, one last round before we make a call
> on this.
> * I can imagine us having more base::Callback utility functions that do
> stuff that isn't "core", but is still useful, like what Ami's done here. I
> think I'd be in support of a base::ResetCallbackAndRun(base::Callback<T>*
> cb) in a callback_util.h file.
>

Why don't we compromise on this (ResetCallbackAndRun in callback_util.h)
then?  If we need ResetAndRun for something else, we can always generalize
it then.

[Ami GONE FROM CHROMIUM, 2012-03-23 13:29:27]

On Fri, Mar 23, 2012 at 12:56 AM, Fred Akalin <akalin@chromium.org> wrote:

> On Thu, Mar 22, 2012 at 4:51 PM, William Chan (陈智昌) <willchan@chromium.org
> > wrote:
>
>> Sorry for delaying you so long Ami, one last round before we make a call
>> on this.
>> * I can imagine us having more base::Callback utility functions that do
>> stuff that isn't "core", but is still useful, like what Ami's done here. I
>> think I'd be in support of a base::ResetCallbackAndRun(base::Callback<T>*
>> cb) in a callback_util.h file.
>>
> Why don't we compromise on this (ResetCallbackAndRun in callback_util.h)
> then?  If we need ResetAndRun for something else, we can always generalize
> it then.
>

I think you both meant ResetAnd*Return*, not ResetAnd*Run* (Run requires
arity, so pump).
Implemented that as patchset #5; PTAL.

[akalin, 2012-03-23 18:42:07]

this LGTM

http://codereview.chromium.org/9717021/diff/26001/base/callback_unittest.cc
File base/callback_unittest.cc (right):

http://codereview.chromium.org/9717021/diff/26001/base/callback_unittest.cc#n...
base/callback_unittest.cc:129: cb(base::Bind(&TestForReentrancy::AssertCBIsNull,
no base::

http://codereview.chromium.org/9717021/diff/26001/base/callback_unittest.cc#n...
base/callback_unittest.cc:130: base::Unretained(this))) {
no base::

http://codereview.chromium.org/9717021/diff/26001/base/callback_unittest.cc#n...
base/callback_unittest.cc:144: base::ResetAndReturn(&tfr.cb).Run();
no base::

[willchan no longer on Chromium, 2012-03-23 20:59:20]

Please update the changelist description.

I was actually fine with using another pump file and having a ResetAndRun(), but
if this is enough for now, then sure. We can revisit later if this becomes
popular.

LGTM mod akalin's comments

[commit-bot: I haz the power, 2012-03-24 17:42:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fischman@chromium.org/9717021/26004

[Ami GONE FROM CHROMIUM, 2012-03-24 17:43:35]

Thanks for sticking with it.

http://codereview.chromium.org/9717021/diff/26001/base/callback_unittest.cc
File base/callback_unittest.cc (right):

http://codereview.chromium.org/9717021/diff/26001/base/callback_unittest.cc#n...
base/callback_unittest.cc:129: cb(base::Bind(&TestForReentrancy::AssertCBIsNull,
On 2012/03/23 18:42:07, akalin wrote:
> no base::

Done.

http://codereview.chromium.org/9717021/diff/26001/base/callback_unittest.cc#n...
base/callback_unittest.cc:130: base::Unretained(this))) {
On 2012/03/23 18:42:07, akalin wrote:
> no base::

Done.

http://codereview.chromium.org/9717021/diff/26001/base/callback_unittest.cc#n...
base/callback_unittest.cc:144: base::ResetAndReturn(&tfr.cb).Run();
On 2012/03/23 18:42:07, akalin wrote:
> no base::

Done.

[commit-bot: I haz the power, 2012-03-24 20:37:31]

Change committed as 128772