Bug 71110 – Add per-script font preferences support in overridePreference

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Matt Falkenhagen]

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.

[Matt Falkenhagen]

Created attachment 112849 [details]
Patch

[Matt Falkenhagen]

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 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]

Created attachment 113141 [details]
Patch

[Matt Falkenhagen]

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

Created attachment 113283 [details]
patch for landing

[Matt Falkenhagen]

(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 on attachment 113283 [details]
patch for landing

Clearing flags on attachment: 113283

Committed r99070: <http://trac.webkit.org/changeset/99070>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.