[webkit-reviews] review canceled: [Bug 106188] Proposal: Add support for even-odd fill and clip to Canvas : [Attachment 181471] Patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has canceled Rik Cabanier <cabanier at adobe.com>'s
request for review:
Bug 106188: Proposal: Add support for even-odd fill and clip to Canvas
https://bugs.webkit.org/show_bug.cgi?id=106188

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

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=181471&action=review


You can not ask to stop the review process for discussion and put an own patch
online that does what you want (without keeping the thread on webkit-dev in the
loop). It is everything else then obvious what eoFill() and eoClip() mean. I
strongly suggest another name, better another way to do it. Do not increase the
number of functions on Canvas unnecessarily. I am unsetting the review flag for
now. This should be discussed on the mailing list further.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.h:154
> +    bool isPointInPath(const float x, const float y, const bool isEvenOdd);

Use enumeration here. We have an enumeration for winding rules in WebCore
already. It should be exposed to the API as well.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:134
>      boolean isPointInPath(in [Optional=DefaultIsUndefined] float x,
> -			     in [Optional=DefaultIsUndefined] float y);
> +			     in [Optional=DefaultIsUndefined] float y,
> +			     in [Optional=DefaultIsUndefined] boolean
isEvenOdd);

don't use an boolean for something that isn't so obvious for people who are not
familiar with winding rules.

> LayoutTests/ChangeLog:8
> +	   Add tests to verify that the winding rule work as expected

s/Add tests to verify that the winding rule work as expected/Add tests to
verify that the winding rule work as expected on HTML Canvas./

The tests look fine for me.

> LayoutTests/fast/canvas/script-tests/canvas-clip-rule.js:54
> +prepareTestScenario(5);

Didn't know this function. What is the test scenario 5?