[webkit-reviews] review granted: [Bug 82560] Implement canvas v5 line dash feature : [Attachment 162301] Patch

[bugzilla-daemon at webkit.org]

Dean Jackson <dino at apple.com> has granted Justin Novosad <junov at google.com>'s
request for review:
Bug 82560: Implement canvas v5 line dash feature
https://bugs.webkit.org/show_bug.cgi?id=82560

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

------- Additional Comments from Dean Jackson <dino at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=162301&action=review


It's slightly weird that the lineDashOffset methods call from the webkit prefix
into the unprefixed, but the setLineDash method does it the other way around.
But that's not a big deal.

It's also a shame we can't copy the vector directly thanks to the CGFloat ->
double cast :(

I wonder if we should have image tests for this? It seems important.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:523
> +    int copyCount = 1 + dash.size() % 2;

I think this is more clear as int copyCount = (dash.size() % 2) ? 2 : 1; (but
I'm not asking to change)

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:533
> +    for (size_t i = 0; i < dash.size(); i++) {
> +	   if (!isfinite(dash[i]) || dash[i] < 0)
> +	       return;
> +	   convertedDashArray[i] = static_cast<DashArrayEntry>(dash[i]);
> +    }
> +    if (copyCount == 2) {
> +	   for (size_t src = 0, dst = dash.size(); src < dash.size(); ++src,
++dst)
> +	       convertedDashArray[dst] = convertedDashArray[src];
> +    }

I'm not sure it would actually be more readable, but you could merge the
content of the second loop into the first.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:547
> +    if (!isfinite(offset))
> +	   return;
> +    if (state().m_lineDashOffset == offset)
>	   return;

Might as well combine these into a single test.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:79
> +	   void setLineDash(in sequence<double> dash);
> +	   sequence<double> getLineDash();
> +	   attribute float lineDashOffset;

I wonder why the spec says the dash array is a list of double when the offset
is a float?

> LayoutTests/fast/canvas/script-tests/canvas-lineDash-invalid.js:10
> +    ctx.setLineDash([1.5, 2.5]);

Could you move all these lines into a resetLineDash() method? It could probably
debug() when it runs too, because I was looking at the expected output and
couldn't work out why these numbers were coming up.

> LayoutTests/fast/canvas/script-tests/canvas-lineDash-invalid.js:33
> +shouldBe("trySettingLineDash([1, 2, 3])", "[1, 2, 3, 1, 2, 3]");

Technically this one isn't invalid, yet it is in the invalid test.