[webkit-reviews] review denied: [Bug 63933] HTML canvas strokes with dash and dashOffset : [Attachment 108635] Patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Young Han Lee <joybro201 at gmail.com>'s
request for review:
Bug 63933: HTML canvas strokes with dash and dashOffset
https://bugs.webkit.org/show_bug.cgi?id=63933

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

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


I'd like to have simon or another reviewer to check the code as well. Just some
notes from me first.

The Changelog looks ok, haven't looked at the tests so far.

> Source/WebCore/bindings/js/JSCanvasRenderingContext2DCustom.cpp:182
> +	   if (!(elem > 0 && elem <= FLT_MAX))

is the if (elem <= 0) return; check not enough? Don't you want to raise a DOM
exception on false values? setDOMException(exec, ec);

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:526
> +    if (!(offset > 0 && offset <= FLT_MAX))

First, why is a negative offset not allowed? SVG spec allows that. 2nd: Please
use std::numeric_limits<float>::max() instead of FLT_MAX. IIRC you don't need
to check for FLT_MAX, but you need to check for infinite. Just use the same
code like for setLineWidth here.