Bug 23287 – Avoid using FontPlatformData outside the FontCache

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Julien Chaffraix]

Currently, we are using FontPlatformData outside the FontCache which should be avoided. Instead SimpleFontData, which wraps the FontPlatformData, should be used instead.

Patch forthcoming.

[Julien Chaffraix]

Created attachment 26662 [details]
Step 1: Make getCachedFontPlatformData private and export an equivalent method that return a SimpleFontData

[Eric Seidel (no email)]

Comment on attachment 26662 [details]
Step 1: Make getCachedFontPlatformData private and export an equivalent method that return a SimpleFontData

It seems we might as well go whole-hog and remove all uses of FontPlatformData outside of the font cache if we're going to remove any.  They're aren't that many as far as I can tell.

[Julien Chaffraix]

(In reply to comment #2)
> (From update of attachment 26662 [details] [review])
> It seems we might as well go whole-hog and remove all uses of FontPlatformData
> outside of the font cache if we're going to remove any.  They're aren't that
> many as far as I can tell.
> 

I wanted to keep the changes simple so that we could easily pinpoint a regression. The aim was to remove them all so I would happily squash the 2 or 3 patches I have if you are willing to take a look at the updated patch :-)

[Eric Seidel (no email)]

OK, all I need is a "sounds sane" from Hyatt, and I'm ready to r+.

[Eric Seidel (no email)]

Comment on attachment 26662 [details]
Step 1: Make getCachedFontPlatformData private and export an equivalent method that return a SimpleFontData

I think the code will be easier to review if we skip this intermediary step and just make FontPlatformData private to begin with.  Right now you're adding extra code in CSSFontFaceSource which just makes this patch more confusing.

[Julien Chaffraix]

Created attachment 45134 [details]
FontPlatformData removal, take 2 - left FontCached to keep the changes manageable

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45134 [details] without any errors.

[Eric Seidel (no email)]

Comment on attachment 45134 [details]
FontPlatformData removal, take 2 - left FontCached to keep the changes manageable

This would break builders.  Otherwise looks sane.  r-.

[Julien Chaffraix]

Created attachment 47501 [details]
FontPlatformData removal, take 2 - merged with ToT

This is the merged version of the previous page. Not setting the r? until the early warning system has checked it.

[Julien Chaffraix]

Comment on attachment 47501 [details]
FontPlatformData removal, take 2 - merged with ToT

I forgot that the EWS will not run if the patch is not r?. So setting this flag now.

[Eric Seidel (no email)]

Comment on attachment 47501 [details]
FontPlatformData removal, take 2 - merged with ToT

Looks fine.

[Eric Seidel (no email)]

Attachment 47501 [details] was posted by a committer and has review+, assigning to Julien Chaffraix for commit.

[Julien Chaffraix]

Landed the patch in r54601 and a follow-up build fix in r54602.

Sorry for taking so long to land it but I needed some time aside to deal with the likely build failure.