[Webkit-unassigned] [Bug 82287] MemoryCache should adopt our standard RefCounted model for object lifetime

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=82287





--- Comment #36 from Eric Seidel <eric at webkit.org>  2012-05-25 14:53:35 PST ---
(From update of attachment 144133)
View in context: https://bugs.webkit.org/attachment.cgi?id=144133&action=review

I really like the change, but I think the PassCachedResourceHandle bits is really a wholy separate change from the RefCounted bits.  I would have done the PassCachedResourceHande bit first, and then teh RefCounted part, were I writing this.  I'm happy to discuss this change at greater length with you over IRC or VC.  I may need a VC walkthrough to review the whole thing at once like this.

> Source/WebKit/mac/WebView/WebHTMLView.mm:535
> -    WebCore::CachedImage* promisedDragTIFFDataSource;
> +    WebCore::CachedResourceHandle<WebCore::CachedImage> promisedDragTIFFDataSource;

Would be nice to document the reasoning in the ChangeLog.

> Source/WebCore/ChangeLog:8
> +        No new tests, refactor only.

:(  Would be nice to explain the change in the ChangeLog.

> Source/WebCore/loader/cache/CachedResourceHandle.h:39
> +// CachedResourceHandle and PassCachedResourceHandle are like RefPtr and PassRefPtr, but specifically for CachedResources.
> +// If you are a CachedResourceClient, hold a CachedResourceHandle instead of a RefPtr, since a CachedResourceHandle will have its
> +// CachedResource pointer updated to the correct resource in the event of a 304, while a RefPtr will not.
> +// FIXME: It would be awesome if we could get rid of CachedResourceHandle and solve 304 handling with a different mechanism,
> +// so CachedResourceClients can just hold normal RefPtrs.`

Fantastic, thank you!

> Source/WebCore/loader/cache/CachedResourceHandle.h:46
> +    PassRefPtr<R> get() const { return m_resource; }

get() normally gives you a raw pointer.

> Source/WebCore/loader/cache/CachedResourceHandle.h:79
> +template <class R> class CachedResourceHandle : public CachedResourceHandleBase {

R?

> Source/WebCore/loader/cache/CachedResourceHandle.h:100
> +template <class R, class RR> bool operator==(const CachedResourceHandle<R>& h, const RR* res) 

I think we could have more descriptive names than R/RR?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.