Bug 38841 – Ignore invalid values for various CanvasRenderingContext2D properties

• Status: CLOSED
• Resolution: FIXED
• Component: WebCore Misc.

[Andreas Kling]

(HTML5 spec 4.8.10.1) The following properties should ignore invalid values:
lineWidth, miterLimit, shadowOffsetX, shadowOffsetY, shadowBlur

[Andreas Kling]

Created attachment 55546 [details]
Proposed patch

[Darin Adler]

Comment on attachment 55546 [details]
Proposed patch

> -    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.

[Andreas Kling]

Created attachment 55582 [details]
Proposed patch v2

Updated patch, all comments addressed. Thanks for the review :)

[Darin Adler]

Comment on attachment 55582 [details]
Proposed patch v2

> +PASS ctx.miterLimit is 1.5
> +PASS ctx.miterLimit is 1.5
> +PASS ctx.miterLimit is 1.5
> +PASS ctx.miterLimit is 1.5
> +PASS ctx.miterLimit is 1.5
> +PASS ctx.miterLimit is 1.5
> +PASS ctx.lineWidth is 1.5
> +PASS ctx.lineWidth is 1.5
> +PASS ctx.lineWidth is 1.5
> +PASS ctx.lineWidth is 1.5
> +PASS ctx.lineWidth is 1.5
> +PASS ctx.lineWidth is 1.5
> +PASS ctx.shadowOffsetX is 1
> +PASS ctx.shadowOffsetY is 2
> +PASS ctx.shadowOffsetX is 1
> +PASS ctx.shadowOffsetY is 2
> +PASS ctx.shadowOffsetX is 1
> +PASS ctx.shadowOffsetY is 2
> +PASS ctx.shadowBlur is 1
> +PASS ctx.shadowBlur is 1
> +PASS ctx.shadowBlur is 1
> +PASS ctx.shadowBlur is 1
> +PASS successfullyParsed is true

This is the old expected file. You need to regenerate this file.

You should upload a new patch, because if we land this one all the bots will fail!

Otherwise, r=me

[Andreas Kling]

Created attachment 55634 [details]
Proposed patch v3

... duh. Updated patch. Thanks Darin!

[WebKit Commit Bot]

Comment on attachment 55634 [details]
Proposed patch v3

Clearing flags on attachment: 55634

Committed r59447: <http://trac.webkit.org/changeset/59447>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

Revision r59447 cherry-picked into qtwebkit-2.0 with commit 038ed8d8dcdc8a63673b602fca7c0601a7988e2c