[webkit-reviews] review granted: [Bug 106854] Generalize DocumentWeakReference into WTF::WeakPtr : [Attachment 182701] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Adam Barth <abarth at webkit.org>'s
request for review:
Bug 106854: Generalize DocumentWeakReference into WTF::WeakPtr
https://bugs.webkit.org/show_bug.cgi?id=106854

Attachment 182701: Patch
https://bugs.webkit.org/attachment.cgi?id=182701&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.