[webkit-reviews] review granted: [Bug 189329] Add Support for Conic Gradients : [Attachment 348983] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Megan Gardner
<megan_gardner at apple.com>'s request for review:
Bug 189329: Add Support for Conic Gradients
https://bugs.webkit.org/show_bug.cgi?id=189329

Attachment 348983: Patch

https://bugs.webkit.org/attachment.cgi?id=348983&action=review




--- Comment #3 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 348983
  --> https://bugs.webkit.org/attachment.cgi?id=348983
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348983&action=review

> Source/WebCore/ChangeLog:9
> +	   Add support to draw conic gradients, as specified in the CSS source.

"in the CSS source" is weird. Maybe just "Add support for rendering CSS conic
gradients".

> Source/WebCore/ChangeLog:16
> +	   Tests: fast/gradients/conic-repeating.html
> +		  fast/gradients/conic.html

I think you should add the following tests in this patch:

* off-center
* center outside the box
* pie-wedge gradient (not filling the full 360deg)
* gradient with first stop < 0deg, last stop > 360deg, and non-opaque colors.

> Source/WebCore/css/CSSGradientValue.cpp:1431
> +    FloatPoint firstPoint = computeEndPoint(m_firstX.get(), m_firstY.get(),
conversionData, size);

A comment to explain that m_firstX  and m_firstY are for the center point would
help here.

> Source/WebCore/platform/graphics/cg/GradientCG.cpp:136
> +	       CGContextTranslateCTM(platformContext, data.point0.x(),
data.point0.y());
> +	       CGContextRotateCTM(platformContext, (CGFloat)-M_PI_2);
> +	       CGContextTranslateCTM(platformContext, -data.point0.x(),
-data.point0.y());

I guess we disagree with CG about where the zero angle is?

> LayoutTests/fast/gradients/conic-repeating.html:10
> +	       background: repeating-conic-gradient(
> +		     black 0deg, black 90deg,
> +		     red 0deg, red 180deg
> +		 );

You should avoid anything red in a passing test case. Also,  don't use the same
color in more than one sector, otherwise your test might pass with a 180deg
flip bug.

> LayoutTests/fast/gradients/conic.html:7
> +	       background: conic-gradient(#f06 0deg, #f06 180deg, gold 180deg,
gold 360deg);

Not really a fan of the gold and #f06 colors. Choose blue and orange, say.