Add a BarrierClosure.

Add a BarrierClosure.

Chromium frequently needs to execute a "final" closure once a given
number of other callbacks have run. BarrierClosure encapsulates that
logic while staying as light-weight as possible.

TBR=gbillock@chromium.org, willchan@chromium.org
BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=219446

[groby-ooo-7-16, 2013-08-22 21:10:10]

There are quite a few cases where Chromium could've benefited from a
BarrierClosure - adding this as a sketch for discussion.

ajwong: Heads up, since it's technically related to callbacks
gbillock: Heads up, since he's working on a more heavy-weight callback scheduler
(https://codereview.chromium.org/18969002/)
willchan: OWNERS review, please.

(For an example that essentially implements BarrierClosure without doing the
entirely right thing, see http://crbug.com/269707 - specifically,
SpellcheckMessageFilterMac's OnLocalCheckCompleted, OnRemoteCheckCompleted,
OnCheckCompleted)

[groby-ooo-7-16, 2013-08-22 21:27:29]

CC'ed darin, per ajwong's recommendation.

[awong, 2013-08-22 21:28:33]

Couple of nits.

question: did you cross-reference this with the internal NewBarrierClosure API? 
If not, please double check that to make sure we didn't miss something subtle.

https://codereview.chromium.org/22859056/diff/1/base/barrier_closure.cc
File base/barrier_closure.cc (right):

https://codereview.chromium.org/22859056/diff/1/base/barrier_closure.cc#newco...
base/barrier_closure.cc:40: const base::Closure& done_closure) {
nit: indentation

https://codereview.chromium.org/22859056/diff/1/base/barrier_closure.h
File base/barrier_closure.h (right):

https://codereview.chromium.org/22859056/diff/1/base/barrier_closure.h#newcode5
base/barrier_closure.h:5: #ifndef BASE_BARRIER_CLOSURE_
BASE_BARRIER_CLOSURE_ -> BASE_BARRIER_CLOSURE_H_

https://codereview.chromium.org/22859056/diff/1/base/barrier_closure.h#newcode19
base/barrier_closure.h:19: // maintained as a base::AtomicRefCount.
This should note the threading assumptions about where done_closure will be run,
and when it will be released.

https://codereview.chromium.org/22859056/diff/1/base/barrier_closure.h#newcode23
base/barrier_closure.h:23: }  // base
}  // namesapce base

#endif  // BASE_BARRIER_CLOSURE_H_

https://codereview.chromium.org/22859056/diff/1/base/barrier_closure_unittest.cc
File base/barrier_closure_unittest.cc (right):

https://codereview.chromium.org/22859056/diff/1/base/barrier_closure_unittest...
base/barrier_closure_unittest.cc:16: base::Closure
barrierClosure=base::BarrierClosure(0, doneClosure);
spaces around = please.

https://codereview.chromium.org/22859056/diff/1/base/barrier_closure_unittest...
base/barrier_closure_unittest.cc:24: base::Closure
barrierClosure=base::BarrierClosure(2, doneClosure);
spaces around = please.

[groby-ooo-7-16, 2013-08-22 21:59:44]

Yes, I did cross-reference. AFAICT, the semantics are identical - final closure
immediately on 0, otherwise after final Run(), atomic refcount.

https://codereview.chromium.org/22859056/diff/1/base/barrier_closure.cc
File base/barrier_closure.cc (right):

https://codereview.chromium.org/22859056/diff/1/base/barrier_closure.cc#newco...
base/barrier_closure.cc:40: const base::Closure& done_closure) {
On 2013/08/22 21:28:33, awong wrote:
> nit: indentation

Done.

https://codereview.chromium.org/22859056/diff/1/base/barrier_closure.h
File base/barrier_closure.h (right):

https://codereview.chromium.org/22859056/diff/1/base/barrier_closure.h#newcode5
base/barrier_closure.h:5: #ifndef BASE_BARRIER_CLOSURE_
On 2013/08/22 21:28:33, awong wrote:
> BASE_BARRIER_CLOSURE_ -> BASE_BARRIER_CLOSURE_H_

Done.

https://codereview.chromium.org/22859056/diff/1/base/barrier_closure.h#newcode19
base/barrier_closure.h:19: // maintained as a base::AtomicRefCount.
On 2013/08/22 21:28:33, awong wrote:
> This should note the threading assumptions about where done_closure will be
run,
> and when it will be released.

Done.

https://codereview.chromium.org/22859056/diff/1/base/barrier_closure.h#newcode23
base/barrier_closure.h:23: }  // base
On 2013/08/22 21:28:33, awong wrote:
> }  // namesapce base
> 
> #endif  // BASE_BARRIER_CLOSURE_H_

Done.

https://codereview.chromium.org/22859056/diff/1/base/barrier_closure_unittest.cc
File base/barrier_closure_unittest.cc (right):

https://codereview.chromium.org/22859056/diff/1/base/barrier_closure_unittest...
base/barrier_closure_unittest.cc:16: base::Closure
barrierClosure=base::BarrierClosure(0, doneClosure);
On 2013/08/22 21:28:33, awong wrote:
> spaces around = please.

Done.

https://codereview.chromium.org/22859056/diff/1/base/barrier_closure_unittest...
base/barrier_closure_unittest.cc:24: base::Closure
barrierClosure=base::BarrierClosure(2, doneClosure);
On 2013/08/22 21:28:33, awong wrote:
> spaces around = please.

Done.

[awong, 2013-08-22 22:05:15]

https://codereview.chromium.org/22859056/diff/9001/base/barrier_closure.cc
File base/barrier_closure.cc (right):

https://codereview.chromium.org/22859056/diff/9001/base/barrier_closure.cc#ne...
base/barrier_closure.cc:52: }  // base
// namespace base

https://codereview.chromium.org/22859056/diff/9001/base/barrier_closure.h
File base/barrier_closure.h (right):

https://codereview.chromium.org/22859056/diff/9001/base/barrier_closure.h#new...
base/barrier_closure.h:21: // be Reset() after that.
I would say

|done_closure| is also Reset() on the final calling thread but due to the
refcounted nature of callback, it is hard to know thread resources will be
released on.

[willchan no longer on Chromium, 2013-08-23 00:17:58]

I'


On Fri, Aug 23, 2013 at 6:05 AM, <ajwong@chromium.org> wrote:

>
> https://codereview.chromium.**org/22859056/diff/9001/base/**
>
barrier_closure.cc<https://codereview.chromium.org/22859056/diff/9001/base/barrier_closure.cc>
> File base/barrier_closure.cc (right):
>
> https://codereview.chromium.**org/22859056/diff/9001/base/**
>
barrier_closure.cc#newcode52<https://codereview.chromium.org/22859056/diff/9001/base/barrier_closure.cc#newcode52>
> base/barrier_closure.cc:52: }  // base
> // namespace base
>
> https://codereview.chromium.**org/22859056/diff/9001/base/**
>
barrier_closure.h<https://codereview.chromium.org/22859056/diff/9001/base/barrier_closure.h>
> File base/barrier_closure.h (right):
>
> https://codereview.chromium.**org/22859056/diff/9001/base/**
>
barrier_closure.h#newcode21<https://codereview.chromium.org/22859056/diff/9001/base/barrier_closure.h#newcode21>
> base/barrier_closure.h:21: // be Reset() after that.
> I would say
>
> |done_closure| is also Reset() on the final calling thread but due to
> the refcounted nature of callback, it is hard to know thread resources
> will be released on.
>
>
https://codereview.chromium.**org/22859056/<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.

[willchan no longer on Chromium, 2013-08-23 00:19:52]

I'm running off right now but I just wanted to say that I'm fine with
adding a BarrierClosure and am fine having Albert do the detailed review
here.


On Fri, Aug 23, 2013 at 8:17 AM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> I'
>
>
> On Fri, Aug 23, 2013 at 6:05 AM, <ajwong@chromium.org> wrote:
>
>>
>> https://codereview.chromium.**org/22859056/diff/9001/base/**
>>
barrier_closure.cc<https://codereview.chromium.org/22859056/diff/9001/base/barrier_closure.cc>
>> File base/barrier_closure.cc (right):
>>
>> https://codereview.chromium.**org/22859056/diff/9001/base/**
>>
barrier_closure.cc#newcode52<https://codereview.chromium.org/22859056/diff/9001/base/barrier_closure.cc#newcode52>
>> base/barrier_closure.cc:52: }  // base
>> // namespace base
>>
>> https://codereview.chromium.**org/22859056/diff/9001/base/**
>>
barrier_closure.h<https://codereview.chromium.org/22859056/diff/9001/base/barrier_closure.h>
>> File base/barrier_closure.h (right):
>>
>> https://codereview.chromium.**org/22859056/diff/9001/base/**
>>
barrier_closure.h#newcode21<https://codereview.chromium.org/22859056/diff/9001/base/barrier_closure.h#newcode21>
>> base/barrier_closure.h:21: // be Reset() after that.
>> I would say
>>
>> |done_closure| is also Reset() on the final calling thread but due to
>> the refcounted nature of callback, it is hard to know thread resources
>> will be released on.
>>
>>
https://codereview.chromium.**org/22859056/<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.

[groby-ooo-7-16, 2013-08-24 00:27:25]

https://codereview.chromium.org/22859056/diff/9001/base/barrier_closure.cc
File base/barrier_closure.cc (right):

https://codereview.chromium.org/22859056/diff/9001/base/barrier_closure.cc#ne...
base/barrier_closure.cc:52: }  // base
On 2013/08/22 22:05:16, awong wrote:
> // namespace base

Done.

https://codereview.chromium.org/22859056/diff/9001/base/barrier_closure.h
File base/barrier_closure.h (right):

https://codereview.chromium.org/22859056/diff/9001/base/barrier_closure.h#new...
base/barrier_closure.h:21: // be Reset() after that.
On 2013/08/22 22:05:16, awong wrote:
> I would say
> 
> |done_closure| is also Reset() on the final calling thread but due to the
> refcounted nature of callback, it is hard to know thread resources will be
> released on.

Done.

[awong, 2013-08-24 00:29:01]

LGTM

[groby-ooo-7-16, 2013-08-24 00:30:44]

TBR'd willchan, gbillock

[commit-bot: I haz the power, 2013-08-24 00:31:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/groby@chromium.org/22859056/16001

[commit-bot: I haz the power, 2013-08-24 15:00:37]

Change committed as 219446