[webkit-reviews] review denied: [Bug 107744] Add support for currentPath to Canvas, currentX and currentY to CanvasPathMethods : [Attachment 184349] Patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied  review:
Bug 107744: Add support for currentPath to Canvas, currentX and currentY to
CanvasPathMethods
https://bugs.webkit.org/show_bug.cgi?id=107744

Attachment 184349: Patch
https://bugs.webkit.org/attachment.cgi?id=184349&action=review

------- Additional Comments from Dirk Schulze <krit at webkit.org>
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.