Generalize DocumentWeakReference into WTF::WeakPtr

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

Generalize DocumentWeakReference into WTF::WeakPtr

[Adam Barth]

Created attachment 182679 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 182679 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182679&action=review

Looks reasonable to me.  You should probably wait for others to wake and comment since we're adding something to WTF and thus might want a bit broader consensus. :)

> Source/WebCore/dom/Document.cpp:4786
> -    RefPtr<DocumentWeakReference> documentReference;
> +    WeakPtr<Document> documentRef;

I'm not sure why you changed the variable name.

[Adam Barth]

Created attachment 182701 [details]
Patch

[Adam Barth]

This updated version is basically the same, but with some ASSERTs to make sure that the functions are called on the right thread.  (I've also improved some of the names slightly.)

[Alexey Proskuryakov]

> You should probably wait for others to wake and comment

Some of the people likely to comment read webkit-dev more frequently than bugmail, so e-mailing the list could be helpful.

[Darin Adler]

Comment on attachment 182701 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182701&action=review

> Source/WTF/ChangeLog:37
> +        (WeakPtr):

prepare-ChangeLog puts these kinds of bogus lines in. I suggest you take them out.

> Source/WTF/wtf/WeakPtr.h:40
> +class WeakReference : public ThreadSafeRefCounted<WeakReference<T> > {

It seems a little inconsistent to use ThreadSafeRefCounted here when we use the non-thread-safe RefCounted class by default in so many other places in WebKit. It’s almost arbitrary that we chose the thread safe version here. Obviously, going forward it would be nice if we could use the thread-safe version more and maybe phase out the single-threaded-only version some day if we can deal with the performance issues that caused us to make that one. Maybe RefCounted needs to be renamed to something else and ThreadSafeRefCounted should get the short name.

> Source/WTF/wtf/WeakPtr.h:79
> +    ALWAYS_INLINE WeakPtr() : m_impl(0) { }

I don’t think this line of code is needed at all. The default constructor will already initialize m_impl to 0 and I am pretty sure this will get inlined. Normally we wait to use ALWAYS_INLINE and only use it if we find that some compiler isn’t inlining as desired.

> Source/WTF/wtf/WeakPtr.h:80
> +    explicit WeakPtr(PassRefPtr<Internal::WeakReference<T> > impl) : m_impl(impl) { }

Seems unnecessary to make this constructor explicit, harmless to do this any time as a type conversion.

> Source/WTF/wtf/WeakPtr.h:96
> +    explicit WeakPtrFactory(T* ptr) { m_impl = Internal::WeakReference<T>::create(ptr); }
> +    ~WeakPtrFactory() { m_impl->clear(); }
> +
> +    WeakPtr<T> createWeakPtr() { return WeakPtr<T>(m_impl); }

Some clients might want the version that creates the WeakReference the first time createWeakPtr is called instead of in the constructor.

> Source/WTF/wtf/WeakPtr.h:102
> +template<typename T, typename U> inline bool operator==(const WeakPtr<T>& a, const WeakPtr<U>& b)

Reading this makes me realize our existing strategy on our smart pointer types and == is kind of weak. Lots of people end up doing get() when they want to do == or != even though they don’t have to. And since WeakPtr can be combined with OwnPtr or RefPtr, we don’t have the == and != operators that allow mixing the different pointer types. I think we could consider not bothering with the overloading and just letting people do get() when they want to compare the pointers. Or find a more complete way to do the overloading that works with various combinations of smart pointers.

[Adam Barth]

(In reply to comment #6)
> (From update of attachment 182701 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=182701&action=review
> 
> > Source/WTF/ChangeLog:37
> > +        (WeakPtr):
> 
> prepare-ChangeLog puts these kinds of bogus lines in. I suggest you take them out.

Fixed.

> > Source/WTF/wtf/WeakPtr.h:40
> > +class WeakReference : public ThreadSafeRefCounted<WeakReference<T> > {
> 
> It seems a little inconsistent to use ThreadSafeRefCounted here when we use the non-thread-safe RefCounted class by default in so many other places in WebKit. It’s almost arbitrary that we chose the thread safe version here. Obviously, going forward it would be nice if we could use the thread-safe version more and maybe phase out the single-threaded-only version some day if we can deal with the performance issues that caused us to make that one. Maybe RefCounted needs to be renamed to something else and ThreadSafeRefCounted should get the short name.

The main use case we have for this class is cross-thread messaging where you want to send a message to an object on another thread but that object might be destroyed before the message arrives.  (That's what DocumentWeakReference was used for prior to this patch.)

> > Source/WTF/wtf/WeakPtr.h:79
> > +    ALWAYS_INLINE WeakPtr() : m_impl(0) { }
> 
> I don’t think this line of code is needed at all. The default constructor will already initialize m_impl to 0 and I am pretty sure this will get inlined. Normally we wait to use ALWAYS_INLINE and only use it if we find that some compiler isn’t inlining as desired.

Fixed.

> > Source/WTF/wtf/WeakPtr.h:80
> > +    explicit WeakPtr(PassRefPtr<Internal::WeakReference<T> > impl) : m_impl(impl) { }
> 
> Seems unnecessary to make this constructor explicit, harmless to do this any time as a type conversion.

Fixed.

> > Source/WTF/wtf/WeakPtr.h:96
> > +    explicit WeakPtrFactory(T* ptr) { m_impl = Internal::WeakReference<T>::create(ptr); }
> > +    ~WeakPtrFactory() { m_impl->clear(); }
> > +
> > +    WeakPtr<T> createWeakPtr() { return WeakPtr<T>(m_impl); }
> 
> Some clients might want the version that creates the WeakReference the first time createWeakPtr is called instead of in the constructor.

Yes, that's true.  I'll add a comment to that effect so that future contributors can add that the first time its needed.

> > Source/WTF/wtf/WeakPtr.h:102
> > +template<typename T, typename U> inline bool operator==(const WeakPtr<T>& a, const WeakPtr<U>& b)
> 
> Reading this makes me realize our existing strategy on our smart pointer types and == is kind of weak. Lots of people end up doing get() when they want to do == or != even though they don’t have to. And since WeakPtr can be combined with OwnPtr or RefPtr, we don’t have the == and != operators that allow mixing the different pointer types. I think we could consider not bothering with the overloading and just letting people do get() when they want to compare the pointers. Or find a more complete way to do the overloading that works with various combinations of smart pointers.

Ok.  These aren't used anyway, so I've removed them.

[Adam Barth]

Created attachment 182835 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 182835 [details]
Patch for landing

Clearing flags on attachment: 182835

Committed r139780: <http://trac.webkit.org/changeset/139780>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Antonio Gomes]

Thanks so much for working on this, Adam!