Summary: | Ignore invalid values for various CanvasRenderingContext2D properties | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andreas Kling <kling> | ||||||||
Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | CLOSED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, hausmann | ||||||||
Priority: | P2 | Keywords: | HTML5 | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 35784 | ||||||||||
Attachments: |
|
Description
Andreas Kling
2010-05-10 06:05:33 PDT
Created attachment 55546 [details]
Proposed patch
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. Created attachment 55582 [details]
Proposed patch v2
Updated patch, all comments addressed. Thanks for the review :)
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 Created attachment 55634 [details]
Proposed patch v3
... duh. Updated patch. Thanks Darin!
Comment on attachment 55634 [details] Proposed patch v3 Clearing flags on attachment: 55634 Committed r59447: <http://trac.webkit.org/changeset/59447> All reviewed patches have been landed. Closing bug. |