Bug 78184 - Allow per-script font settings to be specified in layout tests
Summary: Allow per-script font settings to be specified in layout tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Matt Falkenhagen
URL:
Keywords:
Depends on:
Blocks: 10874
  Show dependency treegraph
 
Reported: 2012-02-08 17:05 PST by Matt Falkenhagen
Modified: 2012-03-14 19:12 PDT (History)
7 users (show)

See Also:


Attachments
Patch (13.38 KB, patch)
2012-02-22 21:02 PST, Matt Falkenhagen
no flags Details | Formatted Diff | Diff
Patch (16.22 KB, patch)
2012-02-22 23:18 PST, Matt Falkenhagen
no flags Details | Formatted Diff | Diff
Patch (29.00 KB, patch)
2012-03-08 16:05 PST, Matt Falkenhagen
no flags Details | Formatted Diff | Diff
remove expectation pngs (29.81 KB, patch)
2012-03-12 02:38 PDT, Matt Falkenhagen
no flags Details | Formatted Diff | Diff
add WebCore.exp.in back in (30.52 KB, patch)
2012-03-12 22:55 PDT, Matt Falkenhagen
no flags Details | Formatted Diff | Diff
Patch (28.76 KB, patch)
2012-03-14 01:03 PDT, Matt Falkenhagen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Falkenhagen 2012-02-08 17:05:44 PST
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.
Comment 1 Kenichi Ishibashi 2012-02-08 17:17:50 PST
You may want to use InternalSettings class which lives in Source/WebCore/testing.
Comment 2 Matt Falkenhagen 2012-02-20 22:06:46 PST
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.
Comment 3 Kenichi Ishibashi 2012-02-20 22:15:45 PST
(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.
Comment 4 Matt Falkenhagen 2012-02-22 21:02:13 PST
Created attachment 128384 [details]
Patch
Comment 5 Matt Falkenhagen 2012-02-22 21:04:29 PST
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.
Comment 6 Philippe Normand 2012-02-22 21:08:43 PST
Comment on attachment 128384 [details]
Patch

Attachment 128384 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11602093
Comment 7 Kenichi Ishibashi 2012-02-22 21:17:27 PST
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.
Comment 8 Kenichi Ishibashi 2012-02-22 21:23:18 PST
(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
Comment 9 Matt Falkenhagen 2012-02-22 23:18:51 PST
Created attachment 128404 [details]
Patch
Comment 10 Matt Falkenhagen 2012-02-22 23:19:54 PST
bashi@ thanks for the comments. It builds on Mac now.
Comment 11 Philippe Normand 2012-02-22 23:23:24 PST
Comment on attachment 128404 [details]
Patch

Attachment 128404 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11610117
Comment 12 Kenichi Ishibashi 2012-02-22 23:29:32 PST
(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)'
Comment 13 Matt Falkenhagen 2012-03-07 15:09:59 PST
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?
Comment 14 Kenichi Ishibashi 2012-03-07 16:29:37 PST
(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
Comment 15 Kenichi Ishibashi 2012-03-07 16:33:32 PST
(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
Comment 16 Matt Falkenhagen 2012-03-08 15:50:04 PST
Updating summary to what the patch is trying to do.
Comment 17 Matt Falkenhagen 2012-03-08 16:05:06 PST
Created attachment 130930 [details]
Patch
Comment 18 Matt Falkenhagen 2012-03-12 02:38:54 PDT
Created attachment 131303 [details]
remove expectation pngs
Comment 19 Build Bot 2012-03-12 03:08:29 PDT
Comment on attachment 131303 [details]
remove expectation pngs

Attachment 131303 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11940796
Comment 20 Matt Falkenhagen 2012-03-12 22:55:30 PDT
Created attachment 131551 [details]
add WebCore.exp.in back in
Comment 21 Hajime Morrita 2012-03-13 19:18:34 PDT
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.
Comment 22 Matt Falkenhagen 2012-03-14 01:03:19 PDT
Created attachment 131802 [details]
Patch
Comment 23 Matt Falkenhagen 2012-03-14 01:06:04 PDT
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.
Comment 24 Hajime Morrita 2012-03-14 01:12:30 PDT
Comment on attachment 131802 [details]
Patch

Looks good. Please wait until bots become green.
Comment 25 WebKit Review Bot 2012-03-14 17:58:43 PDT
Comment on attachment 131802 [details]
Patch

Rejecting attachment 131802 [details] from commit-queue.

falken@chromium.org 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.
Comment 26 WebKit Review Bot 2012-03-14 19:12:45 PDT
Comment on attachment 131802 [details]
Patch

Clearing flags on attachment: 131802

Committed r110808: <http://trac.webkit.org/changeset/110808>
Comment 27 WebKit Review Bot 2012-03-14 19:12:52 PDT
All reviewed patches have been landed.  Closing bug.