Bug 78184 – Allow per-script font settings to be specified in layout tests

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Matt Falkenhagen]

Add support for per-script font settings in overridePreference for DumpRenderTree so layout tests can be written for per-script font selection (bug 10874). Bug 71110 added it but only for Chromium port.

[Kenichi Ishibashi]

You may want to use InternalSettings class which lives in Source/WebCore/testing.

[Matt Falkenhagen]

bashi, thanks for the pointer. It looks like InternalSettings is like an alternative to overridePreference. Do you know what are the reasons to use one or the other? It seems great if it works on all ports automatically. Then the DRT for each port won't have to implement per-script font settings.

[Kenichi Ishibashi]

(In reply to comment #2)
> bashi, thanks for the pointer. It looks like InternalSettings is like an alternative to overridePreference. Do you know what are the reasons to use one or the other? It seems great if it works on all ports automatically. Then the DRT for each port won't have to implement per-script font settings.

If my understanding is correct, it would be better to use InternalSettings than DRT. It should work all ports. Morrita-san, please correct me if I'm wrong.

[Matt Falkenhagen]

Created attachment 128384 [details]
Patch

[Matt Falkenhagen]

Hi Morrita-san, what do you think of this approach? It's not ready to commit yet... I get a linker error when I try to build on Mac and still have to update the ChangeLog.

[Philippe Normand]

Comment on attachment 128384 [details]
Patch

Attachment 128384 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11602093

[Kenichi Ishibashi]

Comment on attachment 128384 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128384&action=review

> Source/WebCore/ChangeLog:3
> +        [DRT] Support per-script font settings in overridePreference

nit: you should remove [DRT] prefix because there is no DRT related stuff.

Please add description of this change.

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

Please explain why tests are not necessary.

> LayoutTests/ChangeLog:5
> +

Please add description.

[Kenichi Ishibashi]

(In reply to comment #5)
> Hi Morrita-san, what do you think of this approach? It's not ready to commit yet... I get a linker error when I try to build on Mac and still have to update the ChangeLog.

You may need to add symbols to WebCore.exp.in and symbols.filter like http://trac.webkit.org/changeset/106146

[Matt Falkenhagen]

Created attachment 128404 [details]
Patch

[Matt Falkenhagen]

bashi@ thanks for the comments. It builds on Mac now.

[Philippe Normand]

Comment on attachment 128404 [details]
Patch

Attachment 128404 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11610117

[Kenichi Ishibashi]

(In reply to comment #11)
> (From update of attachment 128404 [details])
> Attachment 128404 [details] did not pass gtk-ews (gtk):
> Output: http://queues.webkit.org/results/11610117

Looks like you may also need to export Settings::setXXXFontFamily() functions.

./.libs/libWebCoreInternals.a(./.libs/../Source/WebCore/testing/.libs/libWebCoreInternals_la-InternalSettings.o):InternalSettings.cpp:function WebCore::setStandardFontFamilyWrapper(WebCore::Settings*, WTF::String const&, UScriptCode): error: undefined reference to 'WebCore::Settings::setStandardFontFamily(WTF::AtomicString const&, UScriptCode)'

[Matt Falkenhagen]

Could a GTK expert please advise me on what symbols to add to WebCore.exp.in or symbols.filter? I'm having trouble setting up an environment to build WebKitGTK+ myself.

Morrita-san, what do you think the overall approach of adding this to InternalSettings?

[Kenichi Ishibashi]

(In reply to comment #13)
> Could a GTK expert please advise me on what symbols to add to WebCore.exp.in or symbols.filter? I'm having trouble setting up an environment to build WebKitGTK+ myself.
> 
> Morrita-san, what do you think the overall approach of adding this to InternalSettings?

I'm not really sure about it, but looks like these symbols should be added in symbols.filter.

_ZNK7WebCore8Settings15fixedFontFamilyE11UScriptCode
_ZNK7WebCore8Settings15serifFontFamilyE11UScriptCode
_ZNK7WebCore8Settings17cursiveFontFamilyE11UScriptCode
_ZNK7WebCore8Settings17fantasyFontFamilyE11UScriptCode
_ZNK7WebCore8Settings18standardFontFamilyE11UScriptCode
_ZNK7WebCore8Settings19sansSerifFontFamilyE11UScriptCode
_ZNK7WebCore8Settings20pictographFontFamilyE11UScriptCode

[Kenichi Ishibashi]

(In reply to comment #14)
> _ZNK7WebCore8Settings15fixedFontFamilyE11UScriptCode
> _ZNK7WebCore8Settings15serifFontFamilyE11UScriptCode
> _ZNK7WebCore8Settings17cursiveFontFamilyE11UScriptCode
> _ZNK7WebCore8Settings17fantasyFontFamilyE11UScriptCode
> _ZNK7WebCore8Settings18standardFontFamilyE11UScriptCode
> _ZNK7WebCore8Settings19sansSerifFontFamilyE11UScriptCode
> _ZNK7WebCore8Settings20pictographFontFamilyE11UScriptCode

I made mistake. These should be:

_ZN7WebCore8Settings18setFixedFontFamilyERKN3WTF12AtomicStringE11UScriptCode
_ZN7WebCore8Settings18setSerifFontFamilyERKN3WTF12AtomicStringE11UScriptCode
_ZN7WebCore8Settings20setCursiveFontFamilyERKN3WTF12AtomicStringE11UScriptCode
_ZN7WebCore8Settings20setFantasyFontFamilyERKN3WTF12AtomicStringE11UScriptCode
_ZN7WebCore8Settings21setStandardFontFamilyERKN3WTF12AtomicStringE11UScriptCode
_ZN7WebCore8Settings22setSansSerifFontFamilyERKN3WTF12AtomicStringE11UScriptCode
_ZN7WebCore8Settings23setPictographFontFamilyERKN3WTF12AtomicStringE11UScriptCode

[Matt Falkenhagen]

Updating summary to what the patch is trying to do.

[Matt Falkenhagen]

Created attachment 130930 [details]
Patch

[Matt Falkenhagen]

Created attachment 131303 [details]
remove expectation pngs

[Build Bot]

Comment on attachment 131303 [details]
remove expectation pngs

Attachment 131303 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11940796

[Matt Falkenhagen]

Created attachment 131551 [details]
add WebCore.exp.in back in

[Hajime Morrita]

Comment on attachment 131551 [details]
add WebCore.exp.in back in

View in context: https://bugs.webkit.org/attachment.cgi?id=131551&action=review

The approach looks good. added some nitpicky comments.

> Source/WebCore/testing/InternalSettings.cpp:287
> +        (*setter)(settings(), family, code);

I think we can use a member function pointer here.
http://stackoverflow.com/questions/5499155/c-member-function-pointer

> Source/WebCore/testing/InternalSettings.h:33
> +#include <wtf/unicode/Unicode.h>

Could you get rid of this include? I want the dependency to be as small as possible.

> Source/WebCore/testing/InternalSettings.h:85
> +    void setFontFamily(const String& family, const String& script, SetFontFamilyWrapper setter);

For that purpose, this could be be s static function.

[Matt Falkenhagen]

Created attachment 131802 [details]
Patch

[Matt Falkenhagen]

Comment on attachment 131551 [details]
add WebCore.exp.in back in

View in context: https://bugs.webkit.org/attachment.cgi?id=131551&action=review

Thanks for the review. I've uploaded a new patch.

>> Source/WebCore/testing/InternalSettings.cpp:287
>> +        (*setter)(settings(), family, code);
> 
> I think we can use a member function pointer here.
> http://stackoverflow.com/questions/5499155/c-member-function-pointer

Done.

>> Source/WebCore/testing/InternalSettings.h:33
>> +#include <wtf/unicode/Unicode.h>
> 
> Could you get rid of this include? I want the dependency to be as small as possible.

Done.

>> Source/WebCore/testing/InternalSettings.h:85
>> +    void setFontFamily(const String& family, const String& script, SetFontFamilyWrapper setter);
> 
> For that purpose, this could be be s static function.

Done.

[Hajime Morrita]

Comment on attachment 131802 [details]
Patch

Looks good. Please wait until bots become green.

[WebKit Review Bot]

Comment on attachment 131802 [details]
Patch

Rejecting attachment 131802 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Review Bot]

Comment on attachment 131802 [details]
Patch

Clearing flags on attachment: 131802

Committed r110808: <http://trac.webkit.org/changeset/110808>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.