Bug 107744 – Add support for currentX and currentY to CanvasPathMethods

• Component: Canvas
• Status: RESOLVED LATER

[Rik Cabanier]

Define a couple of helper APIs:
- currentPath lets you set/get the path from the canvasContext
- currentX/currentY are readonly attributes on the CanvasPathMethods that return the current point. an exception is thrown if there is no current point.

[Rik Cabanier]

Created attachment 184349 [details]
Patch

[Rik Cabanier]

Created attachment 184359 [details]
Patch

[Dirk Schulze]

Comment on attachment 184349 [details]
Patch

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

It should definitely be reviewed by some of the JS guys as well olliej might be a good reviewer.

> Source/WebCore/ChangeLog:3
> +        Add support for currentPath to Canvas, currentX and currentY to CanvasPathMethods

These are two different features and should be separated.

You do not have one test for currentPath, even if that needs to be tested heavily.
We need JS tests on setting and getting the path for currentPath. With valid arguments and invalid arguments. Should currentPath return exceptions in case you try to set it with something invalid? I think so and it needs to be tested.
How do trasnformations of the context interact with the currentPath you get? I think this is obvious but needs testing (scale context, apply a rect, get currentPath and set it on the context with identity CTM), what happens with multiple transform-pathsegment-transfrom-calls? Needs to be tested as well.

For now it might be enough to just implement currentX/Y.

> Source/WebCore/ChangeLog:12
> +        currentX and currentY are 2 new getter on the CanvasPathAPI objects. They will return 

I am not convinced that it is obvious enough for authors what currentX/Y means. The origin of the context? A translation? The position of the canvas? The name should be self descriptive.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:376
> +    *static_cast<CanvasPathMethods*>(domPath.get()) = *this;

That looks weird. Why *this? What do you want to do here? Don't you want to create a DOMPath object that has a copy of currentPath? I would implement DOMPath::create(Path&) and create a copy of the current path.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:381
> +
> +void CanvasRenderingContext2D::setCurrentPath(const DOMPath* domPath)

This should return an exception if you do something weird.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:383
> +    *static_cast<CanvasPathMethods*>(this) = *domPath;

This looks weird as well. I am not a JS expert, but it doesn't look like what you want. You want to set the m_path in CanvasPathMethod.

> LayoutTests/fast/canvas/script-tests/canvas-path-current.js:26
> +shouldThrow("ctx.currentX", "'Error: InvalidStateError: DOM Exception 11'");
> +shouldThrow("ctx.currentY", "'Error: InvalidStateError: DOM Exception 11'");
> +ctx.moveTo(25, 35);
> +shouldBe("ctx.currentX", "25");
> +shouldBe("ctx.currentY", "35");
> +p = ctx.currentPath;
> +shouldBe("ctx.currentX", "p.currentX");
> +shouldBe("ctx.currentY", "p.currentY");
> +ctx.beginPath();
> +shouldThrow("ctx.currentX", "'Error: InvalidStateError: DOM Exception 11'");
> +shouldNotThrow("p.currentX", "'Error: InvalidStateError: DOM Exception 11'");
> +ctx.moveTo(50, 75);
> +shouldBe("ctx.currentX", "50");
> +shouldBe("ctx.currentY", "75");
> +shouldBe("p.currentX", "25");
> +shouldBe("p.currentY", "35");
> +ctx.currentPath = p;
> +shouldBe("ctx.currentX", "25");
> +shouldBe("ctx.currentY", "35");

I would like to see a separation of the testing on Path and on the context. Ideally in two different files, but at least separated locally. I don't think that the tests are sufficient enough. Please add tests for all path methods. It would also be interesting to have a combination of path methods with different transforms, to verify that the transformations affect the current point as well. Save and restore operations should be included as well, to test the behavior after restoring the context.

[Dirk Schulze]

Comment on attachment 184359 [details]
Patch

See previous comments.

[Early Warning System Bot]

Comment on attachment 184359 [details]
Patch

Attachment 184359 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16084389

[WebKit Review Bot]

Comment on attachment 184359 [details]
Patch

Attachment 184359 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16077443

[Build Bot]

Comment on attachment 184359 [details]
Patch

Attachment 184359 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16067650

[Peter Beverloo (cr-android ews)]

Comment on attachment 184359 [details]
Patch

Attachment 184359 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16078507

[Build Bot]

Comment on attachment 184359 [details]
Patch

Attachment 184359 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16072540

[Early Warning System Bot]

Comment on attachment 184359 [details]
Patch

Attachment 184359 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16074571

[Build Bot]

Comment on attachment 184359 [details]
Patch

Attachment 184359 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16083512

[EFL EWS Bot]

Comment on attachment 184359 [details]
Patch

Attachment 184359 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16065996

[Rik Cabanier]

Created attachment 184535 [details]
Patch

not for review

[Brent Fulgham]

Is any work being done on this bug? Should we just close it out?

[Dirk Schulze]

currentPath was rejected and currentX, currentY didn't make it into the spec yet. Not sure but I guess we can close it as LATER to track the patch still? But didn't look at the latest patch.