Bug 69817 – [skia] Implement Path.currentPoint for skia

• Component: New Bugs
• Status: RESOLVED FIXED

[Ben Wells]

[skia] Implement Path.currentPoint for skia

[Ben Wells]

Created attachment 110476 [details]
Patch

[Ben Wells]

I implemented this function while working another bug but it wasn't needed. It might come in handy in future for someone else or prevent cross platform problems creeping in if this function is used in future by developers of other ports.

It is used in the canvas code to work out if a line, arc etc. will be zero length and therefore whether it should be added or not. The zero length segments get filtered out later so there should be no impact of the change. Tests all pass with the change.

[Mike Reed]

Looks fine. You could also just call skiaPath.getLastPt(), but you'll still need the check for count==0. The next rev. of getLastPt returns a bool which indicates if the lastPt is valid. This would simplify the impl as well when that lands...

[Kenneth Russell]

Comment on attachment 110476 [details]
Patch

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

Looks fine but there's a problem with the ChangeLog that will cause it to be rejected from the commit queue.

> Source/WebCore/ChangeLog:10
> +        No new tests. (OOPS!)

You'll need to remove this OOPS line. Also ideally there would be a test case.

[Ben Wells]

Created attachment 110633 [details]
Patch

[Ben Wells]

I'm not sure how to test this with a new layout test. Any advice?

[Kenneth Russell]

(In reply to comment #6)
> I'm not sure how to test this with a new layout test. Any advice?

Did you search the code base for uses of Path.currentPoint() and see if there's any way to get it called from JavaScript, for example via a series of Canvas calls?

[Kenneth Russell]

Comment on attachment 110633 [details]
Patch

Looks fine. r=me

[Ben Wells]

(In reply to comment #7)
> (In reply to comment #6)
> > I'm not sure how to test this with a new layout test. Any advice?
> 
> Did you search the code base for uses of Path.currentPoint() and see if there's any way to get it called from JavaScript, for example via a series of Canvas calls?

Yes I searched for where currentPoint is used when making the change, its not exposed anywhere with easy access like that.

[WebKit Review Bot]

Comment on attachment 110633 [details]
Patch

Rejecting attachment 110633 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Review Bot]

Comment on attachment 110633 [details]
Patch

Clearing flags on attachment: 110633

Committed r97319: <http://trac.webkit.org/changeset/97319>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.