Bug 107744 - Add support for currentX and currentY to CanvasPathMethods
Summary: Add support for currentX and currentY to CanvasPathMethods
Status: RESOLVED LATER
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dirk Schulze
URL:
Keywords:
Depends on: 97333
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-23 15:04 PST by Rik Cabanier
Modified: 2014-04-03 14:08 PDT (History)
12 users (show)

See Also:


Attachments
Patch (11.59 KB, patch)
2013-01-23 16:52 PST, Rik Cabanier
no flags Details | Formatted Diff | Diff
Patch (12.46 KB, text/plain)
2013-01-23 17:05 PST, Rik Cabanier
webkit-ews: commit-queue-
Details
Patch (12.53 KB, patch)
2013-01-24 11:08 PST, Rik Cabanier
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rik Cabanier 2013-01-23 15:04:11 PST
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.
Comment 1 Rik Cabanier 2013-01-23 16:52:37 PST
Created attachment 184349 [details]
Patch
Comment 2 Rik Cabanier 2013-01-23 17:05:01 PST
Created attachment 184359 [details]
Patch
Comment 3 Dirk Schulze 2013-01-23 17:24:42 PST
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.
Comment 4 Dirk Schulze 2013-01-23 17:25:23 PST
Comment on attachment 184359 [details]
Patch

See previous comments.
Comment 5 Early Warning System Bot 2013-01-23 17:29:18 PST
Comment on attachment 184359 [details]
Patch

Attachment 184359 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16084389
Comment 6 WebKit Review Bot 2013-01-23 17:50:06 PST
Comment on attachment 184359 [details]
Patch

Attachment 184359 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16077443
Comment 7 Build Bot 2013-01-23 19:51:35 PST
Comment on attachment 184359 [details]
Patch

Attachment 184359 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16067650
Comment 8 Peter Beverloo (cr-android ews) 2013-01-23 20:08:30 PST
Comment on attachment 184359 [details]
Patch

Attachment 184359 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/16078507
Comment 9 Build Bot 2013-01-23 20:32:26 PST
Comment on attachment 184359 [details]
Patch

Attachment 184359 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16072540
Comment 10 Early Warning System Bot 2013-01-23 21:13:55 PST
Comment on attachment 184359 [details]
Patch

Attachment 184359 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16074571
Comment 11 Build Bot 2013-01-23 22:24:14 PST
Comment on attachment 184359 [details]
Patch

Attachment 184359 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16083512
Comment 12 EFL EWS Bot 2013-01-24 03:03:17 PST
Comment on attachment 184359 [details]
Patch

Attachment 184359 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16065996
Comment 13 Rik Cabanier 2013-01-24 11:08:59 PST
Created attachment 184535 [details]
Patch

not for review
Comment 14 Brent Fulgham 2014-04-02 11:21:59 PDT
Is any work being done on this bug? Should we just close it out?
Comment 15 Dirk Schulze 2014-04-03 01:15:16 PDT
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.