Adding the possibility to build a closure from an ObjectiveC block.

Adding the possibility to build a closure from an ObjectiveC block.

This lead to more readeable ObjectiveC code as the bound code is right next to where it is scheduled as opposed to be outside the class–usually in a separate function in an anonymous namespace.

BUG=None
TEST=None


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

[noyau (Ping after 24h), 2012-07-24 18:00:16]

Stuart for initial review.

[stuartmorgan, 2012-07-25 12:45:26]

LGTM with nit. +Mark

https://chromiumcodereview.appspot.com/10818034/diff/3001/base/mac/bind_objc_...
File base/mac/bind_objc_block.mm (right):

https://chromiumcodereview.appspot.com/10818034/diff/3001/base/mac/bind_objc_...
base/mac/bind_objc_block.mm:16: }
Add a  // namespace

[noyau (Ping after 24h), 2012-07-25 12:56:34]

https://chromiumcodereview.appspot.com/10818034/diff/3001/base/mac/bind_objc_...
File base/mac/bind_objc_block.mm (right):

https://chromiumcodereview.appspot.com/10818034/diff/3001/base/mac/bind_objc_...
base/mac/bind_objc_block.mm:16: }
On 2012/07/25 12:45:26, stuartmorgan wrote:
> Add a  // namespace

Done.

[Mark Mentovai, 2012-07-25 14:06:30]

https://chromiumcodereview.appspot.com/10818034/diff/10001/base/mac/bind_objc...
File base/mac/bind_objc_block.mm (right):

https://chromiumcodereview.appspot.com/10818034/diff/10001/base/mac/bind_objc...
base/mac/bind_objc_block.mm:10: namespace {
Blank line after this one, and another blank line before line 16, the “}  //
namespace” line. Namespace formatting is covered at
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...
. You did the right thing for the base namespace below.

https://chromiumcodereview.appspot.com/10818034/diff/10001/base/mac/bind_objc...
base/mac/bind_objc_block.mm:12: void RunBlock(scoped_nsobject<id> block) {
If RunBlock is never called, block is never freed. Can that happen? Does that
happen? Is this OK?

ajwong may have suggestions.

https://chromiumcodereview.appspot.com/10818034/diff/10001/base/mac/bind_objc...
base/mac/bind_objc_block.mm:16: }  // namespace
Blank line before this one.

[noyau (Ping after 24h), 2012-07-25 14:27:58]

https://chromiumcodereview.appspot.com/10818034/diff/10001/base/mac/bind_objc...
File base/mac/bind_objc_block.mm (right):

https://chromiumcodereview.appspot.com/10818034/diff/10001/base/mac/bind_objc...
base/mac/bind_objc_block.mm:10: namespace {
On 2012/07/25 14:06:30, Mark Mentovai wrote:
> Blank line after this one, and another blank line before line 16, the “}  //
> namespace” line. Namespace formatting is covered at
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...
> . You did the right thing for the base namespace below.
Done. I would point out however that the extra lines are not explicitly required
anywhere that I could find and that the last example on the style guide do not
include them either.

https://chromiumcodereview.appspot.com/10818034/diff/10001/base/mac/bind_objc...
base/mac/bind_objc_block.mm:12: void RunBlock(scoped_nsobject<id> block) {
On 2012/07/25 14:06:30, Mark Mentovai wrote:
> If RunBlock is never called, block is never freed. Can that happen? Does that
> happen? Is this OK?
> 
I am not sure I follow. The block is retained by the Callback returned by Bind,
and is released when the Callback is freed, not when RunBlock is invoked.
RunBlock can be invoked multiple times if needed.

https://chromiumcodereview.appspot.com/10818034/diff/10001/base/mac/bind_objc...
base/mac/bind_objc_block.mm:16: }  // namespace
On 2012/07/25 14:06:30, Mark Mentovai wrote:
> Blank line before this one.

Done.

[Mark Mentovai, 2012-07-25 15:40:19]

https://chromiumcodereview.appspot.com/10818034/diff/5003/base/mac/bind_objc_...
File base/mac/bind_objc_block.mm (right):

https://chromiumcodereview.appspot.com/10818034/diff/5003/base/mac/bind_objc_...
base/mac/bind_objc_block.mm:16: }
Won’t the scoped_nsobject<> destructor run here, doing [block release]?

[stuartmorgan, 2012-07-25 15:56:31]

https://chromiumcodereview.appspot.com/10818034/diff/5003/base/mac/bind_objc_...
File base/mac/bind_objc_block.mm (right):

https://chromiumcodereview.appspot.com/10818034/diff/5003/base/mac/bind_objc_...
base/mac/bind_objc_block.mm:16: }
On 2012/07/25 15:40:19, Mark Mentovai wrote:
> Won’t the scoped_nsobject<> destructor run here, doing [block release]?

Bind is hard for mortals to understand, but AFAICT what happens in that the
closure will have a copy of the scoped_nsobject (and thus a retain), and then
every call to RunBlock will (since it's pass-by-value) copy the scoped_nsobject,
thus creating a retain at the start of the function call, and that's the one
that's being released conceptually at this point.

When nothing owns the closure any more, *that's* when the [block copy] below is
balanced with a release.

[Mark Mentovai, 2012-07-25 16:02:16]

Oh, right, I forgot that we added a copy constructor to scoped_nsobject<> in
r121215, http://codereview.chromium.org/9349016.

LGTM

[commit-bot: I haz the power, 2012-07-25 16:18:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/noyau@chromium.org/10818034/5003

[commit-bot: I haz the power, 2012-07-25 17:48:03]

Change committed as 148357