Bug 38841

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.

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