| Summary: | [Qt] PathQt should give back pointAtLength and length | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Dirk Schulze <krit> | ||||||||
| Component: | WebKit Qt | Assignee: | Nobody <webkit-unassigned> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | ariya.hidayat, commit-queue, hausmann, jhanssen, krit, zimmermann | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | PC | ||||||||||
| OS: | OS X 10.5 | ||||||||||
| Attachments: |
|
||||||||||
Adding myself. Created attachment 66332 [details]
Proposed patch
Comment on attachment 66332 [details]
Proposed patch
The test case for path-totalLength is wrong, it passes for Qt but not any of the other platforms. Will upload a new patch later.
Sorry, I thougt I commented on this bug. Iam not sure about pointAtLength. IIRC we give back the last possible point if the given length is larger than the totalLength of the path. Can you check this first please? (In reply to comment #4) > Sorry, I thougt I commented on this bug. Iam not sure about pointAtLength. IIRC we give back the last possible point if the given length is larger than the totalLength of the path. Can you check this first please? I'll have a look before uploading a revised patch. Thanks. Created attachment 66847 [details]
Proposed patch v2
Adding a test for Path::pointAtLength() for the case where the length argument is beyond the length of the path. Path::length() is already tested.
Why did you remove normalAngleAtLength from your first patch. Doen't http://doc.trolltech.com/4.6/qpainterpath.html#angleAtPercent make what you want? (In reply to comment #7) > Why did you remove normalAngleAtLength from your first patch. Doen't http://doc.trolltech.com/4.6/qpainterpath.html#angleAtPercent make what you want? It probably does. Path::normalAngleAtLength() not directly exposed from JS though, making it a bit more tricky to write a test case for that. Probably need to trigger a test via some SVG animation if that's possible (I haven't looked extensively at this yet). (In reply to comment #8) > (In reply to comment #7) > > Why did you remove normalAngleAtLength from your first patch. Doen't http://doc.trolltech.com/4.6/qpainterpath.html#angleAtPercent make what you want? > > It probably does. Path::normalAngleAtLength() not directly exposed from JS though, making it a bit more tricky to write a test case for that. Probably need to trigger a test via some SVG animation if that's possible (I haven't looked extensively at this yet). It is used for text on path in SVG. So if it doesn't work you break some SVG tests. IIRC Qt has pixel tests, so you should recognize it. If you want to check it your own, visit http://dev.w3.org/SVG/profiles/1.1F2/test/harness/htmlObject/text-path-01-b.html (In reply to comment #9) > It is used for text on path in SVG. So if it doesn't work you break some SVG tests. IIRC Qt has pixel tests, so you should recognize it. If you want to check it your own, visit http://dev.w3.org/SVG/profiles/1.1F2/test/harness/htmlObject/text-path-01-b.html You're right, LayoutTests/svg/text/text-hkern.svg fails if Path::normalAngleAtLength() returns incorrect results. That should be sufficient for testing this, I'll attach a new patch. Created attachment 66854 [details]
Proposed patch v3
Comment on attachment 66854 [details] Proposed patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=66854&action=prettypatch LGTM. r=me. Comment on attachment 66854 [details] Proposed patch v3 Clearing flags on attachment: 66854 Committed r66979: <http://trac.webkit.org/changeset/66979> All reviewed patches have been landed. Closing bug. |
PathQt should give back pointAtLength and length itself. At the moment Qt is using PathTraversalState in Path.cpp, like most other platforms. But I could be a performance win to use qreal percentAtLength ( qreal len ) const QPointF pointAtPercent for pointAtLength and qreal length () const for length().