[webkit-reviews] review denied: [Bug 102656] If maxWidth for displaying a text, is zero or negative then, return empty array : [Attachment 186533] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Feb 4 23:20:24 PST 2013


Benjamin Poulain <benjamin at webkit.org> has denied Rashmi Shyamasundar
<rashmi.s2 at samsung.com>'s request for review:
Bug 102656: If maxWidth for displaying a text, is zero or negative then, return
empty array
https://bugs.webkit.org/show_bug.cgi?id=102656

Attachment 186533: Patch
https://bugs.webkit.org/attachment.cgi?id=186533&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=186533&action=review


Nice catch, it is amazing this was not tested before... :)

Some comments:

> Source/WebCore/ChangeLog:9
> +	   The functions fillText()/strokeText() should not display anything
when 
> +	   maxWidth is less than or equal to zero.

I suggest referencing this in the ChangeLog:
http://www.w3.org/TR/2dcontext/#text-preparation-algorithm

> LayoutTests/ChangeLog:13
> +	   * fast/canvas/canvas-fillText-maxWidth-zero-expected.txt: Added.
> +	   * fast/canvas/canvas-fillText-maxWidth-zero.html: Added.
> +	   * fast/canvas/script-tests/canvas-fillText-maxWidth-zero.js: Added.

The name of this test is unfortunate:
1) You test more than fillText, you also check strokeText.
2) The value 0 is not particularly interesting. It is <= 0.

I suggest splitting the test in two (fillText and strokeText), and change
maxWidth-zero to invalid-maxWidth.

> LayoutTests/fast/canvas/script-tests/canvas-fillText-maxWidth-zero.js:17
> +debug("Test canvas.fillText() with maxWidth zero");
> +ctx.fillStyle = '#f00';
> +ctx.fillText("AA", 0, 1, 0);
> +
> +var imageData = ctx.getImageData(0, 0, 1, 1);
> +var imgdata = imageData.data;
> +shouldBe("imgdata[0]", "0");
> +shouldBe("imgdata[1]", "255");
> +shouldBe("imgdata[2]", "0");

This looks a little fragile. Does the test pass if you set maxWidth to 1?

Is it possible to make a less fragile test?


More information about the webkit-reviews mailing list