Add a WeakPtr<T> class....

Add a WeakPtr<T> class.

This allows a class to hand out weak pointers to itself that will be nulled out
automatically when the class instance is destroyed.

I have provided two ways for a class to implement weak pointers.  It can either
subclass SupportsWeakPtr (like subclassing RefCounted) or it can be composed of
WeakPtrFactory (like having a ScopedRunnableMethodFactory member).

Eventually, I'd like to make it possible to pass a WeakPtr<T> as the first
parameter to NewRunnableMethod.  This will make ScopedRunnableMethodFactory
obsolete and should help cleanup some code.

One feature that makes the implementation here a bit more complicated is that
it is possible to pass a WeakPtr<U> to a method that takes a WeakPtr<T>
provided U "is a" T.  This proved useful in RenderView, which can then give
out weak references to both itself as well as to an interface it implements.
This informed the design of WeakPtr, causing it to have a T* ptr_ member
instead of stashing that pointer within the ref counted WeakReference object.

R=brettw
BUG=none
TEST=weak_ptr_unittest.cc


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

[brettw, 2009-09-01 17:23:49]

http://codereview.chromium.org/183026/diff/7001/7003
File base/weak_ptr.h (right):

http://codereview.chromium.org/183026/diff/7001/7003#newcode48
Line 48: // WARNING: weak pointers are not threadsafe!!!
Can you be more specific here? Like add "You can not use weak pointers on a
different thread than the object pointed to is on." or something.

http://codereview.chromium.org/183026/diff/7001/7003#newcode78
Line 78: class Flag : public RefCountedData<bool>, public NonThreadSafe {};
Can you write a comment for what this is doing?

http://codereview.chromium.org/183026/diff/7001/7003#newcode97
Line 97: // constructor by avoiding the need for a public WeakReference
accessor.
I'm still confused about what you're doing here. Can you add a bit more to the
comment about specifically what this gives you?

http://codereview.chromium.org/183026/diff/7001/7003#newcode152
Line 152: : WeakPtrBase(ref), ptr_(ptr) {}
I find this a bit hard to read. I'd put the } on the next line (like you did for
the empty constructor above).

http://codereview.chromium.org/183026/diff/7001/7003#newcode154
Line 154: T* ptr_;
Can you comment that this pointer's validity is indicated by ref_, and when ref_
isn't valid, the value here is undefined (as opposed to NULL)?

http://codereview.chromium.org/183026/diff/7001/7003#newcode170
Line 170: void InvalidateWeakPtrs() { owner_.Invalidate(); }
Why is this public? Do we expect external classes to call it? Does just the
derived class need to call it (in which case it should be protected).

http://codereview.chromium.org/183026/diff/7001/7003#newcode173
Line 173: internal::WeakReferenceOwner owner_;
Seems like a more unique name would be a good idea since random classes will be
deriving from this and will get this injected into their namespace (even if they
can't access it). I would probably use weak_reference_owner_.

[darin (slow to review), 2009-09-01 17:38:34]

http://codereview.chromium.org/183026/diff/7001/7003
File base/weak_ptr.h (right):

http://codereview.chromium.org/183026/diff/7001/7003#newcode48
Line 48: // WARNING: weak pointers are not threadsafe!!!
On 2009/09/01 17:23:49, brettw wrote:
> Can you be more specific here? Like add "You can not use weak pointers on a
> different thread than the object pointed to is on." or something.

Done.

http://codereview.chromium.org/183026/diff/7001/7003#newcode78
Line 78: class Flag : public RefCountedData<bool>, public NonThreadSafe {};
On 2009/09/01 17:23:49, brettw wrote:
> Can you write a comment for what this is doing?

Done.

http://codereview.chromium.org/183026/diff/7001/7003#newcode97
Line 97: // constructor by avoiding the need for a public WeakReference
accessor.
On 2009/09/01 17:23:49, brettw wrote:
> I'm still confused about what you're doing here. Can you add a bit more to the
> comment about specifically what this gives you?

Done.

http://codereview.chromium.org/183026/diff/7001/7003#newcode152
Line 152: : WeakPtrBase(ref), ptr_(ptr) {}
On 2009/09/01 17:23:49, brettw wrote:
> I find this a bit hard to read. I'd put the } on the next line (like you did
for
> the empty constructor above).

Done.

http://codereview.chromium.org/183026/diff/7001/7003#newcode154
Line 154: T* ptr_;
On 2009/09/01 17:23:49, brettw wrote:
> Can you comment that this pointer's validity is indicated by ref_, and when
ref_
> isn't valid, the value here is undefined (as opposed to NULL)?

Done.

http://codereview.chromium.org/183026/diff/7001/7003#newcode170
Line 170: void InvalidateWeakPtrs() { owner_.Invalidate(); }
On 2009/09/01 17:23:49, brettw wrote:
> Why is this public? Do we expect external classes to call it? Does just the
> derived class need to call it (in which case it should be protected).

Yeah, this could be used in places where we currently use
ScopedRunnableMethodFactory::RevokeAll.  I don't actually have need for this
yet, so I could also just leave it out, but I see a lot of calls to RevokeAll in
our codebase.

http://codereview.chromium.org/183026/diff/7001/7003#newcode173
Line 173: internal::WeakReferenceOwner owner_;
On 2009/09/01 17:23:49, brettw wrote:
> Seems like a more unique name would be a good idea since random classes will
be
> deriving from this and will get this injected into their namespace (even if
they
> can't access it). I would probably use weak_reference_owner_.

Good catch!  Done.

[brettw, 2009-09-01 17:49:58]

http://codereview.chromium.org/183026/diff/4006/4008
File base/weak_ptr.h (right):

http://codereview.chromium.org/183026/diff/4006/4008#newcode191
Line 191: void InvalidateWeakPtrs() { weak_reference_owner_.Invalidate(); }
I agree that there are a bunch of callers, but those are all for the scoped
factory, and the member for the factory is always inside some other class. In
this case, you're exposing RevokeAll to any users of the class, which I don't
think we have now, and I don't think we want.

[darin (slow to review), 2009-09-01 18:13:12]

http://codereview.chromium.org/183026/diff/4006/4008
File base/weak_ptr.h (right):

http://codereview.chromium.org/183026/diff/4006/4008#newcode191
Line 191: void InvalidateWeakPtrs() { weak_reference_owner_.Invalidate(); }
On 2009/09/01 17:49:58, brettw wrote:
> I agree that there are a bunch of callers, but those are all for the scoped
> factory, and the member for the factory is always inside some other class. In
> this case, you're exposing RevokeAll to any users of the class, which I don't
> think we have now, and I don't think we want.

So, you support the method for WeakPtrFactory but not for SupportsWeakPtr?  I'm
okay with that.

[brettw, 2009-09-01 18:32:37]

http://codereview.chromium.org/183026/diff/4006/4008
File base/weak_ptr.h (right):

http://codereview.chromium.org/183026/diff/4006/4008#newcode191
Line 191: void InvalidateWeakPtrs() { weak_reference_owner_.Invalidate(); }
On 2009/09/01 18:13:12, darin wrote:
> On 2009/09/01 17:49:58, brettw wrote:
> > I agree that there are a bunch of callers, but those are all for the scoped
> > factory, and the member for the factory is always inside some other class.
In
> > this case, you're exposing RevokeAll to any users of the class, which I
don't
> > think we have now, and I don't think we want.
> 
> So, you support the method for WeakPtrFactory but not for SupportsWeakPtr? 
I'm
> okay with that.

Yes. LGTM with this.