[webkit-reviews] review denied: [Bug 106188] Proposal: Add support for even-odd fill and clip to Canvas : [Attachment 183387] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Jan 18 07:42:47 PST 2013
Dirk Schulze <krit at webkit.org> has denied 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 183387: Patch
https://bugs.webkit.org/attachment.cgi?id=183387&action=review
------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=183387&action=review
Some snippets and questions.
> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1027
> +void CanvasRenderingContext2D::fill(const String& winding)
can you rename this please? To windingRuleString or windingRuleEnumeration? (at
this time it is the first, so maybe choose this one).
Since you pass a value, doesn't it make sense to raise exceptions now? I would
pass an EC object here and say that the passed string was invalid.
> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1083
> +void CanvasRenderingContext2D::clip(const String& winding)
Ditto.
> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1103
> +bool CanvasRenderingContext2D::isPointInPath(const float x, const float y,
const String& winding)
Ditto.
> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1161
> +bool CanvasRenderingContext2D::parseWinding(const String& winding, WindRule&
windRule)
Ditto. Can you make this an inline function? Doesn't look like it needs to be
in the class.
> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:26
> +enum CanvasWindingRule { "nonzero", "evenodd" };
This enumeration doesn't look like it is used. I guess you added it since it
should be used as WindingRule according to WebIDL and the Firefox
implementation. Can you add a comment or fixme that this should be used once we
support WebIDL completely?
> LayoutTests/fast/canvas/canvas-isPointInPath-winding-expected.txt:6
> +Testing NZO isPointInPath
please use the enumeration value. Don't be lazy :)
> LayoutTests/fast/canvas/canvas-isPointInPath-winding-expected.txt:9
> +Testing NZO isPointInPath
Ditto.
> LayoutTests/fast/canvas/canvas-isPointInPath-winding-expected.txt:10
> +PASS ctx.isPointInPath(50, 50, 'evenodd') is false
Is the above text correct for this test?
I would also like to see a bunch of negative tests in a separate file. At least
one test seems to be missing in this file: isPointInPath(50, 50, 'nonzero')
More information about the webkit-reviews
mailing list