Fixes up scoped_nsobject so that you don't accidentally release the object...

Forces user to use ScopedNSAutoreleasePool instead of using 
NSAutoreleasePool with scoped_nsobject.

BUG=none
TEST=none


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

[dmac, 2009-10-19 00:45:28]

See what you think of this. Should help prevent stupid future errors/misuses of
scoped_nsobject.

[Mark Mentovai, 2009-10-19 01:17:17]

http://codereview.chromium.org/292011/diff/1/2
File base/scoped_nsobject.h (right):

http://codereview.chromium.org/292011/diff/1/2#newcode22
Line 22: // scoped_nsobject<> is not to be used for NSAutoReleasePools. For
Nit: NSAutoreleasePool, lowercase R.  Same a couple more times in this comment.

http://codereview.chromium.org/292011/diff/1/2#newcode41
Line 41: if (object != object_) {
Ah, naïve!

That's OK, this is tricky stuff.

We used to do this, but it was wrong.  See r12301 and
http://codereview.chromium.org/52018.

The idea here is that the caller must already have an ownership claim over
whatever it gives to scoped_nsobject and scoped_cftyperef, whether it's in the
constructor or in a call to reset().  In either case, it relinquishes that claim
and the scoper assumes it.  This is explicitly documented above.

Whenever you give something to scoped_nsobject by calling reset(), it's because
you own a reference, and you're giving that reference to scoped_nsobject.  If it
happens to be the same object that scoped_nsobject already owns, there needs to
be a release to get rid of that reference.

Prior to r12301, this was a source of leaks.

http://codereview.chromium.org/292011/diff/1/2#newcode99
Line 99: if (object != object_) {
Same.

http://codereview.chromium.org/292011/diff/1/2#newcode142
Line 142: // Do not use scoped_nsobject for NSAutoreleasePools, use
I like this specialization.

[dmac, 2009-10-19 04:19:58]

> http://codereview.chromium.org/292011/diff/1/2#newcode22
> Line 22: // scoped_nsobject<> is not to be used for NSAutoReleasePools. For
> Nit: NSAutoreleasePool, lowercase R.  Same a couple more times in this
comment.

Fixed.

> http://codereview.chromium.org/292011/diff/1/2#newcode41
> Line 41: if (object != object_) {
> Ah, naïve!
> 
> That's OK, this is tricky stuff.

D'oh... should've seen that. Sorry. It's just such a standard cocoa pattern to
check before releasing. Comment put there so some other noob doesn't screw it
up.

New version up.

[Mark Mentovai, 2009-10-19 13:17:01]

LGTM.  Also, fix the change description.

No biggie on the equality check.  The revision history shows that I fixed this
bug six months ago.  What it doesn't show is that I also introduced it when I
first wrote these classes.  Like I said, tricky stuff...

http://codereview.chromium.org/292011/diff/4001/4002
File base/scoped_nsobject.h (right):

http://codereview.chromium.org/292011/diff/4001/4002#newcode41
Line 41: // We intentionally do not check that object != object_ as the caller
must
Nit: I still hate comments that use "we," just start the sentence at "Do not
check."

http://codereview.chromium.org/292011/diff/4001/4002#newcode102
Line 102: // We intentionally do not check that object != object_ as the caller
must
"

[pink (ping after 24hrs), 2009-10-19 14:24:56]

LGTM