Bug 75702

Summary: [Chromium] Use SkFontHost::GetUnitsPerEm instead of advanced type metrics for Android in FontPlatformData
Product: WebKit Reporter: Peter Beverloo <peter>
Component: New BugsAssignee: Peter Beverloo <peter>
Status: RESOLVED FIXED    
Severity: Normal CC: jrg, reed, senorblanco, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 66689    
Attachments:
Description Flags
Patch
senorblanco: review+
Patch for landing none

Peter Beverloo
Reported 2012-01-06 07:31:54 PST
[Chromium] Use SkFontHost::GetUnitsPerEm instead of advanced type metrics for Android in FontPlatformData
Attachments
Patch (2.25 KB, patch)
2012-01-06 07:34 PST, Peter Beverloo 		senorblanco: review+
DetailsFormatted DiffDiff
Patch for landing (2.22 KB, patch)
2012-01-10 07:35 PST, Peter Beverloo 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Peter Beverloo
Comment 1 2012-01-06 07:34:59 PST
Created attachment 121431 [details] Patch
Peter Beverloo
Comment 2 2012-01-06 07:43:29 PST
Hi Tony, could you take a look please?
Tony Chang
Comment 3 2012-01-06 10:08:28 PST
Comment on attachment 121431 [details] Patch What's the difference between getAdvancedTypefaceMetrics and GetUnitsPerEm? Is one faster? On platforms other than Android, do they return the same value? If so, should we use GetUnitsPerEm on all platforms?
Tony Chang
Comment 4 2012-01-06 10:09:03 PST
Also, feel free to loop in someone who works on skia to review.
Peter Beverloo
Comment 5 2012-01-09 03:32:38 PST
(In reply to comment #3) > (From update of attachment 121431 [details]) > What's the difference between getAdvancedTypefaceMetrics and GetUnitsPerEm? Is one faster? On platforms other than Android, do they return the same value? If so, should we use GetUnitsPerEm on all platforms? It's an Android-specific function which solely retrieves the em size from the font header, whereas getAdvancedTypefaceMetrics retrieves other information (such as the name, font type and italic angle) as well. The latter invokes Freetype's FT_Get_PS_Font_Info function which is defined in the "type1" extension, which is not available in Android. +senorblanco
Tony Chang
Comment 6 2012-01-09 10:12:41 PST
(In reply to comment #5) > It's an Android-specific function which solely retrieves the em size from the font header, whereas getAdvancedTypefaceMetrics retrieves other information (such as the name, font type and italic angle) as well. The latter invokes Freetype's FT_Get_PS_Font_Info function which is defined in the "type1" extension, which is not available in Android. Should we implement GetUnitsPerEm on all platforms so we don't have to have an #if here?
Peter Beverloo
Comment 7 2012-01-10 06:28:53 PST
Stephen, could you comment on that option? Removing the #if-guards in Skia code and getting that rolled into WebKit would be an option as well.
Stephen White
Comment 8 2012-01-10 06:57:29 PST
+reed, who knows Skia text code far better than me. Mike, any thoughts on this? Should we fold this into Skia, or leave it here in WebKit?
Mike Reed
Comment 9 2012-01-10 07:10:15 PST
UnitsPerEm should be an API on SkTypeface (we shouldn't be making calls on SkFontHost). For now do whatever is easier I guess, until the new API arrives. http://code.google.com/p/skia/issues/detail?id=441
Stephen White
Comment 10 2012-01-10 07:19:08 PST
Comment on attachment 121431 [details] Patch In that case, I'm fine with going ahead with this. You might want to add a comment referencing the skia bug Mike just created. r=me
Peter Beverloo
Comment 11 2012-01-10 07:28:36 PST
I prefer to keep references local to WebKit, so I filed bug 75961. Since it involves the current Android-hack, I'll take care of that cleanup.
Peter Beverloo
Comment 12 2012-01-10 07:35:14 PST
Created attachment 121845 [details] Patch for landing Patch for landing
WebKit Review Bot
Comment 13 2012-01-10 08:03:59 PST
Comment on attachment 121845 [details] Patch for landing Clearing flags on attachment: 121845 Committed r104578: <http://trac.webkit.org/changeset/104578>
WebKit Review Bot
Comment 14 2012-01-10 08:04:04 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.