[webkit-reviews] review denied: [Bug 108763] Canvas GraphicsContext miterLimit initializes to 4 (SVG) instead of 10 : [Attachment 186469] Patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Jonathan Olson <olsonsjc at gmail.com>'s
request for review:
Bug 108763: Canvas GraphicsContext miterLimit initializes to 4 (SVG) instead of
10
https://bugs.webkit.org/show_bug.cgi?id=108763

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

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=186469&action=review


> Source/WebCore/ChangeLog:8
> +	   Canvas GraphicsContext miterLimit initializes to 4 (SVG) instead of
10
> +	   https://bugs.webkit.org/show_bug.cgi?id=108763
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Test: fast/canvas/canvas-miterLimit.html

Can you be a bit more explicit and add more description please? A changelog
should contain a description of the changes and why you changed it.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:139
> +    applyLineStyles(); // Ensure the GraphicsContext is initialized with
consistent line styles, as values like miterLimit may be different.

comments in the line before.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:599
> +void CanvasRenderingContext2D::applyLineStyles() const

It is not called anywhere else, an inline function where you pass the Graphics
context and state might be enough.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:608
> +    c->setStrokeThickness(state().m_lineWidth);
> +    c->setLineCap(state().m_lineCap);
> +    c->setLineJoin(state().m_lineJoin);
> +    c->setMiterLimit(state().m_miterLimit);
> +}

Are there problems with the other default value as well? If so, can you add
more tests to cover that please?

> LayoutTests/ChangeLog:4
> +	   Canvas GraphicsContext miterLimit initializes to 4 (SVG) instead of
10
> +	   https://bugs.webkit.org/show_bug.cgi?id=108763

Please add one or two lines of description.

> LayoutTests/ChangeLog:18
> +	   *
platform/chromium-android/fast/canvas/canvas-incremental-repaint-expected.png:
Removed.
> +	   *
platform/chromium-linux-x86/fast/canvas/canvas-incremental-repaint-expected.png
: Removed.
> +	   *
platform/chromium-linux/fast/canvas/canvas-incremental-repaint-expected.png:
Removed.
> +	   *
platform/chromium-mac-lion/fast/canvas/canvas-incremental-repaint-expected.png:
Removed.
> +	   *
platform/chromium-mac-snowleopard/fast/canvas/canvas-incremental-repaint-expect
ed.png: Removed.
> +	   *
platform/chromium-mac/fast/canvas/canvas-incremental-repaint-expected.png:
Removed.
> +	   *
platform/chromium-win-xp/fast/canvas/canvas-incremental-repaint-expected.png:
Removed.
> +	   *
platform/chromium-win/fast/canvas/canvas-incremental-repaint-expected.png:
Removed.
> +	   *
platform/mac-snowleopard/fast/canvas/canvas-incremental-repaint-expected.png:
Removed.

Is this cleanup? This should be mentioned in the change log as well.

> LayoutTests/fast/canvas/canvas-miterLimit-expected.txt:5
> +CONSOLE MESSAGE: line 19: The following alpha should be 255, since the unit
rectangle is completely in the mitered join bounds.
> +CONSOLE MESSAGE: line 19: Since the drawn angle is 13 degrees, a mitered
join will be drawn if miterLimit is above 8.833671471997569.
> +CONSOLE MESSAGE: line 19: The default CanvasRenderingContext2D miterLimit
value is 10, but some GraphicsContexts are created with a default of 4.
> +CONSOLE MESSAGE: line 19: If miterLimit is initialized incorrectly, this
will render as a bevel join instead and will be transparent (0).
> +CONSOLE MESSAGE: line 19: imageData.data[3] = 255

We don't use write() and test with console messages. Please look at other tests
in the folder like canvas-clearRect.