Bug 33784

Summary: [Qt] Modifying SVG path dumping to equal to other ports
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: SVGAssignee: QtWebKit Unassigned <webkit-qt-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abecsi, kent.hansen, kling, krit, tonikitoo, webkit.review.bot, zimmermann
Priority: P3 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Attachments:
Description Flags
make path dump closer to Mac none

Description Csaba Osztrogonác 2010-01-17 23:41:34 PST
example: http://build.webkit.org/results/Qt%20Linux%20Release/r53390%20%286095%29/svg/custom/transformedMaskFails-pretty-diff.html
...
on Mac: [data="M0.00,0.00 L150.00,0.00 L150.00,150.00 L0.00,150.00 Z"]
on Qt:  [data="M 0 0L 150 0L 150 150L 0 150L 0 0"]
...

We should remove space after M and L, use fixed point real numbers with 2 decimal, use Z to close path instead of L to starting point.

I have a preliminary patch to fix it except closing path. I try to fix it and then we should update many expected files at the same time.

But unfortunately we have to use platform dependent expected files until fix.
Comment 1 Nikolas Zimmermann 2010-01-18 04:54:25 PST
Hi Csaba,

thanks for the bug report. We should really rewrite to have a cross-platform Path::debugString() implementation, by supplying a path-applier function, that's used to iterate over all path segments (using Path::apply()) in order to dump them.

Shouldn't be much work, does anyone volunteer? :-)
Comment 2 Dirk Schulze 2010-01-18 09:10:30 PST
(In reply to comment #1)
> Hi Csaba,
> 
> thanks for the bug report. We should really rewrite to have a cross-platform
> Path::debugString() implementation, by supplying a path-applier function,
> that's used to iterate over all path segments (using Path::apply()) in order to
> dump them.
> 
> Shouldn't be much work, does anyone volunteer? :-)

Also it is not possible for qt to match the LayoutTest results. Qt closes paths, once they cross each other, while SVG doesn't. Not a problem for the visible result, but gives wrong output for DRT.
Qt also closes path's automaticly, if start and ending point match each other. This causes one of the W3C tests to fail.
Altogether, it's realy better to follow Nikos advice, even if I still did not understand how he wants to do it ;-)
Comment 3 Csaba Osztrogonác 2010-02-05 09:31:46 PST
Created attachment 48236 [details]
make path dump closer to Mac

Now I can't produce a cross-platform Path::debugString() implementation, 
but I would like to make our expected files more similar to Mac.

Proposed patch to modify path dumping and update expected files attached.
Comment 4 WebKit Review Bot 2010-02-05 09:40:22 PST
Attachment 48236 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/qt/PathQt.cpp:314:  Missing space after ,  [whitespace/comma] [3]
WebCore/platform/graphics/qt/PathQt.cpp:317:  Missing space after ,  [whitespace/comma] [3]
WebCore/platform/graphics/qt/PathQt.cpp:327:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 3


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Dirk Schulze 2010-02-05 09:57:57 PST
Comment on attachment 48236 [details]
make path dump closer to Mac

r=me, but please add spaces after the commas in arg(...)
Comment 6 Csaba Osztrogonác 2010-02-05 10:09:35 PST
Comment on attachment 48236 [details]
make path dump closer to Mac

Style fixed, long line splitted as Kenneth asked on #qtwebkit, 
and then patch landed in http://trac.webkit.org/changeset/54429
Comment 7 Dirk Schulze 2010-02-05 23:17:13 PST
Can we close the bug now?
Comment 8 Csaba Osztrogonác 2010-02-09 16:24:45 PST
(In reply to comment #7)
> Can we close the bug now?

I think we shouldn't close this bug. Not yet. Because we have some more little differences between Mac and Qt SVG path dumping results. Additionally we have a strange problem because 0.00 isn't equal to -0.00. It needs more investigation.
Comment 9 Andreas Kling 2010-10-09 13:29:04 PDT
This was fixed by <http://trac.webkit.org/changeset/69386> since we no longer dump platform-specific data for RenderSVGPath.