Bug 43837 – [Qt] PathQt should give back pointAtLength and length

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Dirk Schulze]

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().

[Ariya Hidayat]

Adding myself.

[Jan Erik Hanssen]

Created attachment 66332 [details]
Proposed patch

[Jan Erik Hanssen]

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.

[Dirk Schulze]

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?

[Jan Erik Hanssen]

(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.

[Jan Erik Hanssen]

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.

[Dirk Schulze]

Why did you remove normalAngleAtLength from your first patch. Doen't http://doc.trolltech.com/4.6/qpainterpath.html#angleAtPercent make what you want?

[Jan Erik Hanssen]

(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).

[Dirk Schulze]

(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

[Jan Erik Hanssen]

(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.

[Jan Erik Hanssen]

Created attachment 66854 [details]
Proposed patch v3

[Dirk Schulze]

Comment on attachment 66854 [details]
Proposed patch v3

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

LGTM. r=me.

[WebKit Commit Bot]

Comment on attachment 66854 [details]
Proposed patch v3

Clearing flags on attachment: 66854

Committed r66979: <http://trac.webkit.org/changeset/66979>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.