[webkit-reviews] review denied: [Bug 38841] Ignore invalid values for various CanvasRenderingContext2D properties : [Attachment 55546] Proposed patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon May 10 08:26:41 PDT 2010


Darin Adler <darin at apple.com> has denied Andreas Kling
<andreas.kling at nokia.com>'s request for review:
Bug 38841: Ignore invalid values for various CanvasRenderingContext2D
properties
https://bugs.webkit.org/show_bug.cgi?id=38841

Attachment 55546: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=55546&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> -    if (!(width > 0))
> +    if (!(width > 0) || isnan(width) || isinf(width))
>	   return;

This expression should already work properly for NAN. The expression "width >
0" returns false for NAN and so the function returns. So adding the isnan check
is not helpful. Unless perhaps we are working around a bug on some platform.

We normally use isfinite for checks like this to check both NAN and infinity at
once. I think the two best ways to write this are:

    if (!(isfinite(width) && width > 0))

or

    if (!(width > 0) || isinf(width))

I prefer the first.

> -    if (!(limit > 0))
> +    if (!(limit > 0) || isnan(limit) || isinf(limit))
>	   return;

Same comment.

> +    if (isnan(x) || isinf(x))
> +	   return;

    if (!isfinite(x))
	return;

> +    if (isnan(y) || isinf(y))
> +	   return;

Ditto.

> +    if (!(blur >= 0) || isnan(blur) || isinf(blur))
> +	   return;

Ditto.

I'd prefer not to land the redundant isnan checks, so review-.

As far as the test cases are concerned, it's typically better to write shouldBe
tests so you can see what's being tested. So I like to make functions that take
arguments that are the special values rather than just having repeated results.
Here's an example:

    function trySettingMiterLimit(value)
    {
	context.miterLimit = 1.5;
	context.miterLimit = value;
	return context.miterLimit;
    }


    shouldBe("trySettingMiterLimit(1)", "1");
    shouldBe("trySettingMiterLimit(0)", "1.5");
    shouldBe("trySettingMiterLimit(-1)", "1.5");
    shouldBe("trySettingMiterLimit(Infinity)", "1.5");
    shouldBe("trySettingMiterLimit(NaN)", "1.5");
    shouldBe("trySettingMiterLimit('string')", "1.5");
    shouldBe("trySettingMiterLimit(true)", "1");
    shouldBe("trySettingMiterLimit(false)", "0");

This way the test output easier to understand, and can even encourage adding
more test cases like the ones I added for strings and booleans.

Also, please run the test before making any code change. Doing that would have
shown you that NaN was already correctly handled.


More information about the webkit-reviews mailing list