[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 #19 from Brady Eidson <beidson at apple.com>  2012-05-24 09:51:00 PST ---
(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > (In reply to comment #15)
> > > > (From update of attachment 143386 [details] [details] [details] [details])
> > > > LGTM.  Please add some explanation to the top of CachedResource.h as well as CachedResourceHandle.h to explain when one is supposed to use a CachedResourceHandle, vs. a RefPtr.  You can do that in a separate patch, but I believe it's important to document this distinction in the code now that it's possible to confuse these two.
> > > 
> > > If this is a concern, then why are we doing this bizarre halfway step?
> > > 
> > > Understanding the MemoryCache is already difficult enough.  Making it even more confusing (but relying on comments for clarity) doesn't seem worthwhile.
> > > 
> > > Why can't we replace all uses of CachedResourceHandle with RefPtrs, or go 100% CachedResourceHandle with no raw pointers?
> > > 
> > > Wouldn't either of those be better than "confusing"?
> > > 
> > > (full disclosure: have not looked at the patch, have just been paying attention to the worrisome comments)
> > 
> > Seems worthwhile to remove cq+ for the moment.
> > 
> > As mentioned above, I'd be inclined to replace CachedResourceHandle with RefPtr, but I need to figure the right interactions between CachedResource and its clients in CachedResource::switchClientsToRevalidatedResource(). At the very least, I should probably include a FIXME to that effect when submitting this.
> 
> It appears to be pretty complicated to do away with CachedResourceHandle entirely (at least, not something I could throw together in a day). I'm inclined to set a rule of: RefPtrs are an implementation detail of the cache and parts of the loader below it (namely SubresourceLoader), CachedResourceHandles are the public tool for keeping CachedResources alive. Is that clear/simple enough that it might work in practice?

It seems reasonable.  If completely replacing CachedResourceHandle really is a much more monumental task, then it's fine to take the intermediate step.

But it really would be great to get rid of it altogether sooner rather than later.

Don't let my principled objections hold this up any longer.  :)

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