Bug 112668

Summary: [Qt] New fast/text/word-space-with-kerning-3.html fails on Qt.
Product: WebKit Reporter: Ádám Kallai <kadam>
Component: Tools / TestsAssignee: Allan Sandfeld Jensen <allan.jensen>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, dino, enrica, jturcotte, noam, ossy, szledan, webkit.review.bot, zarvai
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 112795, 113179    
Bug Blocks: 87008    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Ádám Kallai
Reported 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. diff: http://build.webkit.sed.hu/results/x86-64%20Linux%20Qt%20Release/r146183%20(49292)/fast/text/word-space-with-kerning-3-diffs.html I will skip it.
Attachments
Patch (2.91 KB, patch)
2013-03-20 04:27 PDT, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Patch (3.04 KB, patch)
2013-03-20 07:15 PDT, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Patch (6.06 KB, patch)
2013-03-20 09:22 PDT, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Patch (6.51 KB, patch)
2013-03-20 11:01 PDT, Allan Sandfeld Jensen 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Dean Jackson
Comment 2 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 :)
Allan Sandfeld Jensen
Comment 3 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.
Allan Sandfeld Jensen
Comment 4 2013-03-20 04:27:12 PDT
Created attachment 194018 [details] Patch Make complex font path respect WebCore kerning setting
Allan Sandfeld Jensen
Comment 5 2013-03-20 04:56:17 PDT
Committed r146331: <http://trac.webkit.org/changeset/146331>
WebKit Review Bot
Comment 6 2013-03-20 06:03:45 PDT
Re-opened since this is blocked by bug 112795
Allan Sandfeld Jensen
Comment 7 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.
Allan Sandfeld Jensen
Comment 8 2013-03-20 07:15:29 PDT
Created attachment 194048 [details] Patch Second try, this time verified it works when the complex font path is used.
Allan Sandfeld Jensen
Comment 9 2013-03-20 09:22:54 PDT
Created attachment 194076 [details] Patch
Jocelyn Turcotte
Comment 10 2013-03-20 10:02:44 PDT
Comment on attachment 194076 [details] Patch 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.
Allan Sandfeld Jensen
Comment 11 2013-03-20 11:01:53 PDT
Created attachment 194088 [details] Patch Slight clean-up
Pierre Rossi
Comment 12 2013-03-22 09:26:29 PDT
Comment on attachment 194088 [details] Patch Me gusta !
Allan Sandfeld Jensen
Comment 13 2013-03-22 10:53:35 PDT
Comment on attachment 194088 [details] Patch Clearing flags on attachment: 194088 Committed r146630: <http://trac.webkit.org/changeset/146630>
Allan Sandfeld Jensen
Comment 14 2013-03-22 10:53:40 PDT
All reviewed patches have been landed. Closing bug.
Zoltan Arvai
Comment 15 2013-03-25 02:37:15 PDT
After the patch 8 test started asserting on Qt debug bots. https://bugs.webkit.org/show_bug.cgi?id=113179
Note You need to log in before you can comment on or make changes to this bug.