[webkit-reviews] review denied: [Bug 102775] Loader: Store WeakRef CachedResourceHandle in CachedResourceLoader : [Attachment 190896] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied pdeng6 <pan.deng at intel.com>'s request
for review:
Bug 102775: Loader: Store WeakRef CachedResourceHandle in CachedResourceLoader
https://bugs.webkit.org/show_bug.cgi?id=102775

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=190896&action=review


This patch makes the lifetime of CachedResources too complicated.

WeakPtr is a new concept in WebCore.  One of the risks of introducing WeakPtr.h
into the project is that folks will use it to build complex ownership
relationships.	We avoided adding it for a long to for just that reason.  Now
that we have it, we're planning to use it sparingly, at first only for
cross-thread communication where RefPtr and OwnPtr are either inappropriate or
lead to worse problems.

Overall, this patch feels like a band-aid that layers on more complexity
instead of rationalizing the ownership semantics of CachedResources.

> Source/WTF/wtf/WeakPtr.h:72
> +    void reset(T* ptr)
> +    {
> +#ifndef NDEBUG
> +	   m_boundThread = currentThread();
> +#endif
> +	   m_ptr = ptr;
> +    }

If we're going to write m_ptr, we need to assert that we're on the bound
thread.

> Source/WTF/wtf/WeakPtr.h:120
> +	   m_ref = WeakReference<T>::create(0);

ditto