Bug 71110

Summary: Add per-script font preferences support in overridePreference
Product: WebKit Reporter: Matt Falkenhagen <falken>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bashi, jshin, tkent, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
patch for landing none

Matt Falkenhagen
Reported 2011-10-28 04:40:38 PDT
Add per-script font preferences support in overridePreference. The motivation is that a layout test for per-script fonts is difficult, since each platform sets its own default per-script fonts and these don't affect DumpRenderTree, anyway. If overridePreference supports per-script fonts settings, the layout test can do something like: // Set standard font for USCRIPT_ARABIC to Ahem. overridePreference("WebKitStandardFontMap", [ "2", "Ahem" ]); So a layout test like the one proposed in bug 67019 is simpler, since we don't have to be concerned with the default per-script fonts on each platform; we can just test that Ahem font is used, which is recommended for pixel tests.
Attachments
Patch (6.73 KB, patch)
2011-10-28 04:54 PDT, Matt Falkenhagen 		no flags
DetailsFormatted DiffDiff
Patch (10.64 KB, patch)
2011-11-01 01:14 PDT, Matt Falkenhagen 		no flags
DetailsFormatted DiffDiff
patch for landing (10.63 KB, patch)
2011-11-02 00:32 PDT, Matt Falkenhagen 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Matt Falkenhagen
Comment 1 2011-10-28 04:54:26 PDT
Created attachment 112849 [details] Patch
Matt Falkenhagen
Comment 2 2011-10-28 05:00:57 PDT
Hi Tony, Can you review this patch please? It should enable a layout test for per-script font selection, which I'll upload in a subsequent patch.
Tony Chang
Comment 3 2011-10-28 10:50:04 PDT
Comment on attachment 112849 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112849&action=review > Tools/ChangeLog:9 > + // Set standard font for USCRIPT_ARABIC to Ahem. > + overridePreference("WebKitStandardFontMap", [ "2", "Ahem" ]); I assume 2 is USCRIPT_ARABIC? It would be nice if we could pass in strings (e.g., "arabic") instead of magic numbers. For example, what if a port doesn't use ICU (it looks like wince and brewmp don't use icu)? > Tools/DumpRenderTree/chromium/WebPreferences.h:52 > + // Map of UScriptCode to font such as USCRIPT_ARABIC > + // to "My Arabic Font". > + WebKit::WebString standardFontMap[USCRIPT_CODE_LIMIT]; Can we use a HashMap instead of a statically allocated array?
Matt Falkenhagen
Comment 4 2011-11-01 01:14:47 PDT
Created attachment 113141 [details] Patch
Matt Falkenhagen
Comment 5 2011-11-01 01:20:30 PDT
(In reply to comment #3) Thanks for the review. > (From update of attachment 112849 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=112849&action=review > > > Tools/ChangeLog:9 > > + // Set standard font for USCRIPT_ARABIC to Ahem. > > + overridePreference("WebKitStandardFontMap", [ "2", "Ahem" ]); > > I assume 2 is USCRIPT_ARABIC? It would be nice if we could pass in strings (e.g., "arabic") instead of magic numbers. For example, what if a port doesn't use ICU (it looks like wince and brewmp don't use icu)? I've changed it to be a four-letter script name like "Arab", "Hans". So, the other ports will need a mapping of these script names to script code since they can't use ICU directly. Regardless of the port, WebKit settings uses UScriptCode from ICU for per-script fonts, so I think this makes sense for overridePreference. > > > Tools/DumpRenderTree/chromium/WebPreferences.h:52 > > + // Map of UScriptCode to font such as USCRIPT_ARABIC > > + // to "My Arabic Font". > > + WebKit::WebString standardFontMap[USCRIPT_CODE_LIMIT]; > > Can we use a HashMap instead of a statically allocated array? I've changed it to a HashMap.
Tony Chang
Comment 6 2011-11-01 10:02:14 PDT
Comment on attachment 113141 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113141&action=review > Tools/DumpRenderTree/chromium/LayoutTestController.h:527 > + Vector<WebKit::WebString> cppVariantToWebStringArray(const CppVariant& value); Nit: Remove |value| from the declaration.
Matt Falkenhagen
Comment 7 2011-11-02 00:32:19 PDT
Created attachment 113283 [details] patch for landing
Matt Falkenhagen
Comment 8 2011-11-02 00:33:42 PDT
(In reply to comment #6) Thanks! > (From update of attachment 113141 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=113141&action=review > > > Tools/DumpRenderTree/chromium/LayoutTestController.h:527 > > + Vector<WebKit::WebString> cppVariantToWebStringArray(const CppVariant& value); > > Nit: Remove |value| from the declaration. Done.
WebKit Review Bot
Comment 9 2011-11-02 09:41:05 PDT
Comment on attachment 113283 [details] patch for landing Clearing flags on attachment: 113283 Committed r99070: <http://trac.webkit.org/changeset/99070>
WebKit Review Bot
Comment 10 2011-11-02 09:41:10 PDT
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.