Bug 112668 - [Qt] New fast/text/word-space-with-kerning-3.html fails on Qt.
Summary: [Qt] New fast/text/word-space-with-kerning-3.html fails on Qt.
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Allan Sandfeld Jensen
Depends on: 112795 113179
Blocks: 87008
  Show dependency treegraph
Reported: 2013-03-19 01:14 PDT by Ádám Kallai
Modified: 2013-03-25 02:37 PDT (History)
9 users (show)

See Also:

Patch (2.91 KB, patch)
2013-03-20 04:27 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (3.04 KB, patch)
2013-03-20 07:15 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (6.06 KB, patch)
2013-03-20 09:22 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (6.51 KB, patch)
2013-03-20 11:01 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ádám Kallai 2013-03-19 01:14:00 PDT
The new layout test fast/text/word-space-with-kerning-3.html is failing after http://trac.webkit.org/changeset/146087.



I will skip it.
Comment 2 Dean Jackson 2013-03-19 12:01:32 PDT
This seems like an actual bug in the layout system. Is kerning enabled on QT?

If so, then there is definitely a bug. The two pieces of content should be identical.

If not, then I'm not sure how they could be different :)
Comment 3 Allan Sandfeld Jensen 2013-03-20 04:15:10 PDT
I think kerning might be always on in the complex font path in Qt. I have a patch to fix that I will see if that fixes it.
Comment 4 Allan Sandfeld Jensen 2013-03-20 04:27:12 PDT
Created attachment 194018 [details]

Make complex font path respect WebCore kerning setting
Comment 5 Allan Sandfeld Jensen 2013-03-20 04:56:17 PDT
Committed r146331: <http://trac.webkit.org/changeset/146331>
Comment 6 WebKit Review Bot 2013-03-20 06:03:45 PDT
Re-opened since this is blocked by bug 112795
Comment 7 Allan Sandfeld Jensen 2013-03-20 06:08:30 PDT
The test-case works with Qt 5.1 because both cases goes through the fast font path. There does however seem to be a problem with word-spacing working differently in fast and complex path. Possibly we need to fix the same bug in the Qt complex path that this test was introduced to test in the fast path.
Comment 8 Allan Sandfeld Jensen 2013-03-20 07:15:29 PDT
Created attachment 194048 [details]

Second try, this time verified it works when the complex font path is used.
Comment 9 Allan Sandfeld Jensen 2013-03-20 09:22:54 PDT
Created attachment 194076 [details]
Comment 10 Jocelyn Turcotte 2013-03-20 10:02:44 PDT
Comment on attachment 194076 [details]

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

> Source/WebCore/platform/graphics/qt/FontQt.cpp:238
> -    initFormatForTextLayout(&layout);
> +    initFormatForTextLayout(&layout, run);
>      QTextLine line = setupLayout(&layout, run);

Maybe you can do this in setupLayout instead? You don't seem to be using any Font member for this logic, and initFormatForTextLayout isn't a fitting name anymore with this.

> Source/WebCore/platform/graphics/qt/FontQt.cpp:260
> +    if (!range.length)
> +        return;

Maybe this is the reason why, but I'm not quite well following that part.
Comment 11 Allan Sandfeld Jensen 2013-03-20 11:01:53 PDT
Created attachment 194088 [details]

Slight clean-up
Comment 12 Pierre Rossi 2013-03-22 09:26:29 PDT
Comment on attachment 194088 [details]

Me gusta !
Comment 13 Allan Sandfeld Jensen 2013-03-22 10:53:35 PDT
Comment on attachment 194088 [details]

Clearing flags on attachment: 194088

Committed r146630: <http://trac.webkit.org/changeset/146630>
Comment 14 Allan Sandfeld Jensen 2013-03-22 10:53:40 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Zoltan Arvai 2013-03-25 02:37:15 PDT
After the patch 8 test started asserting on Qt debug bots.