Currently, we are using FontPlatformData outside the FontCache which should be avoided. Instead SimpleFontData, which wraps the FontPlatformData, should be used instead. Patch forthcoming.
Created attachment 26662 [details] Step 1: Make getCachedFontPlatformData private and export an equivalent method that return a SimpleFontData
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.
(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 :-)
OK, all I need is a "sounds sane" from Hyatt, and I'm ready to r+.
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.
Created attachment 45134 [details] FontPlatformData removal, take 2 - left FontCached to keep the changes manageable
style-queue ran check-webkit-style on attachment 45134 [details] without any errors.
Comment on attachment 45134 [details] FontPlatformData removal, take 2 - left FontCached to keep the changes manageable This would break builders. Otherwise looks sane. r-.
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.
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.
Comment on attachment 47501 [details] FontPlatformData removal, take 2 - merged with ToT Looks fine.
Attachment 47501 [details] was posted by a committer and has review+, assigning to Julien Chaffraix for commit.
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.