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.
Created attachment 112849 [details] Patch
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.
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?
Created attachment 113141 [details] Patch
(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.
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.
Created attachment 113283 [details] patch for landing
(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.
Comment on attachment 113283 [details] patch for landing Clearing flags on attachment: 113283 Committed r99070: <http://trac.webkit.org/changeset/99070>
All reviewed patches have been landed. Closing bug.