[webkit-reviews] review denied: [Bug 132948] [CG] strokeRect does not honor lineJoin : [Attachment 235498] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Nikos Andronikos
<nikos.andronikos-webkit at cisra.canon.com.au>'s request for review:
Bug 132948: [CG] strokeRect does not honor lineJoin
https://bugs.webkit.org/show_bug.cgi?id=132948

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=235498&action=review


Thanks for tackling this.

Test case is failing, can’t land this until that’s resolved.

> Source/WebCore/ChangeLog:3
> +	   [CANVAS] strokeRect does not honor lineJoin

Bug naming is wrong here. We don’t include "[CANVAS]" in the title of a bug
just because it's a canvas bug fix.

> Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp:1170
> +    // Using CGContextAddRect and CGContextStrokePath to stroke rect rather
than 
> +    // convenience functions (CGContextStrokeRect and
CGContextStrokeRectWithWidth).
> +    // The convenience functions fail to apply some attributes of the
graphics state
> +    // in certain cases (e.g. See
https://bugs.webkit.org/show_bug.cgi?id=132948)
> +    CGContextAddRect(context, rect);
> +    CGContextStrokePath(context);

The comment says that there is a bug in CGContextStrokeRect and
CGContextStrokeRectWithWidth “in certain cases” and provides a link to this
bug. But nothing in this bug documents the problem with these functions, nor do
we have a report to the team at Apple that handles CG describing the bug. I’d
like to be clearer on what the bug is before we work around it. The fact that
the tests are failing also makes me suspect that we don’t yet know the whole
story.

This code as is cannot be correct, because it ignores the passed-in line width.
We at least need CGContextSaveGState, setStrokeThickness, and
CGContextRestoreGState calls in addition to the calls here, as in the
non-shadow gradient code path above.

To test this, I presume we need test cases with different line widths and we
should see failures unless we set the line width. It’s possible this breaks the
use of strokeRect in RenderEmbeddedObject and RenderSVGRect, so we may need
test cases that cover those uses too.