Bug 38841 - Ignore invalid values for various CanvasRenderingContext2D properties
Summary: Ignore invalid values for various CanvasRenderingContext2D properties
Status: CLOSED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: HTML5
Depends on:
Blocks: 35784
  Show dependency treegraph
 
Reported: 2010-05-10 06:05 PDT by Andreas Kling
Modified: 2010-05-14 01:41 PDT (History)
2 users (show)

See Also:


Attachments
Proposed patch (7.22 KB, patch)
2010-05-10 06:12 PDT, Andreas Kling
darin: review-
Details | Formatted Diff | Diff
Proposed patch v2 (8.33 KB, patch)
2010-05-10 11:49 PDT, Andreas Kling
darin: review+
darin: commit-queue-
Details | Formatted Diff | Diff
Proposed patch v3 (9.53 KB, patch)
2010-05-10 18:24 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2010-05-10 06:05:33 PDT
(HTML5 spec 4.8.10.1) The following properties should ignore invalid values:
lineWidth, miterLimit, shadowOffsetX, shadowOffsetY, shadowBlur
Comment 1 Andreas Kling 2010-05-10 06:12:54 PDT
Created attachment 55546 [details]
Proposed patch
Comment 2 Darin Adler 2010-05-10 08:26:42 PDT
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.
Comment 3 Andreas Kling 2010-05-10 11:49:08 PDT
Created attachment 55582 [details]
Proposed patch v2

Updated patch, all comments addressed. Thanks for the review :)
Comment 4 Darin Adler 2010-05-10 18:18:59 PDT
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
Comment 5 Andreas Kling 2010-05-10 18:24:31 PDT
Created attachment 55634 [details]
Proposed patch v3

... duh. Updated patch. Thanks Darin!
Comment 6 WebKit Commit Bot 2010-05-14 00:43:52 PDT
Comment on attachment 55634 [details]
Proposed patch v3

Clearing flags on attachment: 55634

Committed r59447: <http://trac.webkit.org/changeset/59447>
Comment 7 WebKit Commit Bot 2010-05-14 00:43:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Simon Hausmann 2010-05-14 01:28:03 PDT
Revision r59447 cherry-picked into qtwebkit-2.0 with commit 038ed8d8dcdc8a63673b602fca7c0601a7988e2c