ListRefPtr is used only by FontFamily class, besides it strongly depends on FontFamily class semantics which makes it non-generic and inappropriate for being present inside WTF.
Created attachment 200315 [details] WIP project files are not updated yet.
Nice! I seem to remember that ListRefPtr was added to avoid overflowing the stack when destroying AST Nodes in JSC when you had very deep syntax trees. I'm not sure how many nested FontFamily objects you could have linked together, so it might be OK to just have the RefPtr and not do any special processing in the destructor.
(In reply to comment #2) > Nice! > > I seem to remember that ListRefPtr was added to avoid overflowing the stack when destroying AST Nodes in JSC when you had very deep syntax trees. > > I'm not sure how many nested FontFamily objects you could have linked together, so it might be OK to just have the RefPtr and not do any special processing in the destructor. I would keep the existing behaviour so far, as I am not sure that having just RefPtr won't bring the regression, because at least in principal it is still possible to face the same issue(overflowing the stack) again.
Created attachment 200392 [details] patch
Comment on attachment 200392 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=200392&action=review No need to update the order files, they're re-generated by someone in the WebKit performance team every once in a while. > Source/WebCore/platform/graphics/FontFamily.h:74 > + RefPtr<SharedFontFamily> reaper = m_next.release(); > + while (reaper && reaper->hasOneRef()) > + reaper = reaper->releaseNext(); // implicitly protects reaper->next, then derefs reaper I'd add a comment stating that this initially came from ListRefPtr and that it can probably be a regular RefPtr.
Created attachment 200413 [details] patch v2 Took comments from Anders into consideration.
Created attachment 200422 [details] to be landed
Comment on attachment 200422 [details] to be landed Clearing flags on attachment: 200422 Committed r149523: <http://trac.webkit.org/changeset/149523>
All reviewed patches have been landed. Closing bug.
(In reply to comment #0) > it strongly depends on FontFamily class semantics How does it depend on that?
(In reply to comment #10) > (In reply to comment #0) > > it strongly depends on FontFamily class semantics > > How does it depend on that? ~ListRefPtr() { .... reaper = reaper->releaseNext(); // implicitly protects reaper->next, then derefs reaper }
(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #0) > > > it strongly depends on FontFamily class semantics > > > > How does it depend on that? > > ~ListRefPtr() > { > .... > reaper = reaper->releaseNext(); // implicitly protects reaper->next, then derefs reaper > } Yes, that’s the requirement to be a list in the ListRefPtr sense--a releaseNext function. What was missing was documentation of that in the ListRefPtr header. The reason I know this is not dependent on “FontFamily class semantics” is that it wasn’t even first created for FontFamily! It was created for JavaScriptCore. So I guess I am OK with removing this class, but I don’t think the remarks “strongly dependent on class semantics” and “non-generic” are fair.