[webkit-reviews] review requested: [Bug 61875] Fonts returned by FontCache::getFontDataForCharacters() are never released : [Attachment 96057] Updated patch to address comments 15 & 16

[bugzilla-daemon at webkit.org]

Michael Saboff <msaboff at apple.com> has asked  for review:
Bug 61875: Fonts returned by FontCache::getFontDataForCharacters() are never
released
https://bugs.webkit.org/show_bug.cgi?id=61875

Attachment 96057: Updated patch to address comments 15 & 16
https://bugs.webkit.org/attachment.cgi?id=96057&action=review

------- Additional Comments from Michael Saboff <msaboff at apple.com>
(In reply to comment #15)

Made changes in response to ell comments except:

> > Source/WebCore/platform/graphics/FontCache.cpp:-300
> > -	     if (gInactiveFontData->size() > cMaxInactiveFontData)
> > -		 purgeInactiveFontData(gInactiveFontData->size() -
cTargetInactiveFontData);
> 
> Without a change log comment, I have no idea why you didn’t replace this with
a call to checkForFontsToPurge()

Check needed to be moved outside of getFontData since since almost all calls to
getCachedFontData() now happen when purging is not allowed.  Comment to this
effect added to ChangeLog.

> > Source/WebCore/platform/graphics/GlyphPageTreeNode.cpp:383
> > -	 if (child == m_children.end()) {
> > -	     // If there is no level-1 node for fontData, then there is no
deeper node for it in this tree.
> > -	     if (!level)
> > -		 return;
> > -	 } else {
> > +	 if (child != m_children.end()) {
> 
> It is sad to lose this optimization, which still applies to non-fallback
fonts. If this function took a parameter telling it whether the pruning is for
a system fallback font or not, it could still apply the optimization in the
common case.

Unfortunately we don't have data keeping track of this. From what I saw during
testing, a font often serve as a non-fallback and fallback font at the same
time.  Given that font purging is relatively rare, I don't think there is much
benefit to this.

> > Source/WebCore/rendering/RenderListBox.cpp:365
> > +	 FontCachePurgePreventer fontCachePurgePreventer;
> > +	 
> 
> It’s not clear to me when this can get called outside of
FrameView::paintContents()

This and other paths that are protected became apparent when running the webkit
tests with a debug build via the ASSERT in
FontCache::releasePurgeProtectedFont.  Most of these non-frameView cases are
needed for editing code paths.