Make Callback.Reset safe against deleting itself.

Make Callback.Reset safe against deleting itself.

BUG=99159
TEST=CallbackTest.ResetRefCountedOwnerOfCallback


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

[mattm, 2012-06-08 03:59:50]

Forgot about this CL.  I guess you're back now, can you take a look?  thanks.

[awong, 2012-06-08 17:33:15]

oops...thanks for reminding me. Just a few style nits.

Thanks for fixing this!

http://codereview.chromium.org/10388060/diff/1/base/callback_unittest.cc
File base/callback_unittest.cc (right):

http://codereview.chromium.org/10388060/diff/1/base/callback_unittest.cc#newc...
base/callback_unittest.cc:151: CallbackOwner(bool *deleted) {
* should associate with bool to stay consistent with the rest of the callback
code.

http://codereview.chromium.org/10388060/diff/1/base/callback_unittest.cc#newc...
base/callback_unittest.cc:166: ASSERT_TRUE(false);
How about:

   FAIL() << "Should never be called.";

instead?

http://codereview.chromium.org/10388060/diff/1/base/callback_unittest.cc#newc...
base/callback_unittest.cc:169: Callback<void(void)> callback_;
Use Closure instead of Callback<void(void)>

http://codereview.chromium.org/10388060/diff/1/base/callback_unittest.cc#newc...
base/callback_unittest.cc:173: TEST_F(CallbackTest,
ResetRefCountedOwnerOfCallback) {
The name of the test doesn't quite express the high level intent.  How about

CallbackHasLastRefOnContainingObject

?

[mattm, 2012-06-09 02:10:56]

http://codereview.chromium.org/10388060/diff/1/base/callback_unittest.cc
File base/callback_unittest.cc (right):

http://codereview.chromium.org/10388060/diff/1/base/callback_unittest.cc#newc...
base/callback_unittest.cc:151: CallbackOwner(bool *deleted) {
On 2012/06/08 17:33:15, awong wrote:
> * should associate with bool to stay consistent with the rest of the callback
> code.

Done.

http://codereview.chromium.org/10388060/diff/1/base/callback_unittest.cc#newc...
base/callback_unittest.cc:166: ASSERT_TRUE(false);
On 2012/06/08 17:33:15, awong wrote:
> How about:
> 
>    FAIL() << "Should never be called.";
> 
> instead?

Done.

http://codereview.chromium.org/10388060/diff/1/base/callback_unittest.cc#newc...
base/callback_unittest.cc:169: Callback<void(void)> callback_;
On 2012/06/08 17:33:15, awong wrote:
> Use Closure instead of Callback<void(void)>

Done.

http://codereview.chromium.org/10388060/diff/1/base/callback_unittest.cc#newc...
base/callback_unittest.cc:173: TEST_F(CallbackTest,
ResetRefCountedOwnerOfCallback) {
On 2012/06/08 17:33:15, awong wrote:
> The name of the test doesn't quite express the high level intent.  How about
> 
> CallbackHasLastRefOnContainingObject
> 
> ?

Done.

[awong, 2012-06-09 02:40:27]

LGTM!

On Fri, Jun 8, 2012 at 7:10 PM, <mattm@chromium.org> wrote:

>
> http://codereview.chromium.**org/10388060/diff/1/base/**
>
callback_unittest.cc<http://codereview.chromium.org/10388060/diff/1/base/callback_unittest.cc>
> File base/callback_unittest.cc (right):
>
> http://codereview.chromium.**org/10388060/diff/1/base/**
>
callback_unittest.cc#**newcode151<http://codereview.chromium.org/10388060/diff/1/base/callback_unittest.cc#newcode151>
> base/callback_unittest.cc:151: CallbackOwner(bool *deleted) {
> On 2012/06/08 17:33:15, awong wrote:
>
>> * should associate with bool to stay consistent with the rest of the
>>
> callback
>
>> code.
>>
>
> Done.
>
>
> http://codereview.chromium.**org/10388060/diff/1/base/**
>
callback_unittest.cc#**newcode166<http://codereview.chromium.org/10388060/diff/1/base/callback_unittest.cc#newcode166>
> base/callback_unittest.cc:166: ASSERT_TRUE(false);
> On 2012/06/08 17:33:15, awong wrote:
>
>> How about:
>>
>
>     FAIL() << "Should never be called.";
>>
>
>  instead?
>>
>
> Done.
>
>
> http://codereview.chromium.**org/10388060/diff/1/base/**
>
callback_unittest.cc#**newcode169<http://codereview.chromium.org/10388060/diff/1/base/callback_unittest.cc#newcode169>
> base/callback_unittest.cc:169: Callback<void(void)> callback_;
> On 2012/06/08 17:33:15, awong wrote:
>
>> Use Closure instead of Callback<void(void)>
>>
>
> Done.
>
>
> http://codereview.chromium.**org/10388060/diff/1/base/**
>
callback_unittest.cc#**newcode173<http://codereview.chromium.org/10388060/diff/1/base/callback_unittest.cc#newcode173>
> base/callback_unittest.cc:173: TEST_F(CallbackTest,
> ResetRefCountedOwnerOfCallback**) {
> On 2012/06/08 17:33:15, awong wrote:
>
>> The name of the test doesn't quite express the high level intent.  How
>>
> about
>
>  CallbackHasLastRefOnContaining**Object
>>
>
>  ?
>>
>
> Done.
>
>
http://codereview.chromium.**org/10388060/<http://codereview.chromium.org/103...
>

[commit-bot: I haz the power, 2012-06-11 22:45:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/10388060/10002

[commit-bot: I haz the power, 2012-06-11 22:45:44]

Presubmit check for 10388060-10002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    base

[mattm, 2012-06-11 22:46:50]

+willchan for OWNERS

[willchan no longer on Chromium, 2012-06-13 20:21:48]

lgtm

[commit-bot: I haz the power, 2012-06-13 21:18:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/10388060/10002

[commit-bot: I haz the power, 2012-06-13 23:15:32]

Change committed as 142011