Bug 23287

Summary:

Avoid using FontPlatformData outside the FontCache

Product:

WebKit

Reporter:

Julien Chaffraix <jchaffraix>

Component:

WebCore Misc.

Assignee:

Julien Chaffraix <jchaffraix>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ddkilzer, eric, joepeck, mario.bensi, mitz, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Attachments:

Description	Flags
Step 1: Make getCachedFontPlatformData private and export an equivalent method that return a SimpleFontData	eric: review-
FontPlatformData removal, take 2 - left FontCached to keep the changes manageable	eric: review-
FontPlatformData removal, take 2 - merged with ToT	eric: review+, jchaffraix: commit-queue-

Julien Chaffraix

Reported 2009-01-13 01:51:05 PST

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

Attachments
Step 1: Make getCachedFontPlatformData private and export an equivalent method that return a SimpleFontData (5.29 KB, patch) 2009-01-13 02:21 PST, Julien Chaffraix	eric: review-	Details Formatted Diff Diff
FontPlatformData removal, take 2 - left FontCached to keep the changes manageable (20.28 KB, patch) 2009-12-18 02:51 PST, Julien Chaffraix	eric: review-	Details Formatted Diff Diff
FontPlatformData removal, take 2 - merged with ToT (21.25 KB, patch) 2010-01-26 23:16 PST, Julien Chaffraix	eric: review+ jchaffraix: commit-queue-	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Julien Chaffraix

Comment 1 2009-01-13 02:21:04 PST

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

Eric Seidel (no email)

Comment 2 2009-02-26 11:52:44 PST

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

Comment 3 2009-03-09 00:48:16 PDT

(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)

Comment 4 2009-03-09 09:16:15 PDT

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

Eric Seidel (no email)

Comment 5 2009-05-19 20:42:26 PDT

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

Comment 6 2009-12-18 02:51:06 PST

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

WebKit Review Bot

Comment 7 2009-12-18 02:53:31 PST

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

Eric Seidel (no email)

Comment 8 2010-01-26 14:19:26 PST

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

Comment 9 2010-01-26 23:16:14 PST

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 10 2010-01-27 06:57:33 PST

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 11 2010-02-01 15:18:19 PST

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

Eric Seidel (no email)

Comment 12 2010-02-01 16:11:20 PST

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

Julien Chaffraix

Comment 13 2010-02-10 09:09:54 PST

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.

Note You need to log in before you can comment on or make changes to this bug.