[Webkit-unassigned] [Bug 5968] [CG] gradients do not support spreadMethod

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=5968

Tim Horton <thorton at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |thorton at apple.com

--- Comment #10 from Tim Horton <thorton at apple.com> ---
Comment on attachment 349371
  --> https://bugs.webkit.org/attachment.cgi?id=349371
Patch

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

> Source/WebCore/ChangeLog:7
> +        Add support for spreadMethod=repeat and reflect. Also, the opacity of a gradient is now
> +        be the result of multiplying stop-opacity with the opacity of the color.

"is now be" is not quite proper grammar.

> Source/WebCore/platform/graphics/cg/GradientCG.cpp:116
> +                FloatPoint gradientVectorNorm = data.point1 + -data.point0;

Is there a reason you can't just subtract and instead add a negative? This looks super weird to me.

> Source/WebCore/platform/graphics/cg/GradientCG.cpp:129
> +                CGFloat pixelSize = abs(CGContextConvertSizeToUserSpace(platformContext, CGSizeMake(1, 1)).width);

s/abs/CGFAbs/, I think that will fix the build (at least one of the build problems)

> Source/WebCore/platform/graphics/cg/GradientCG.cpp:137
> +                        CGContextDrawLinearGradient(platformContext, gradient, CGPointMake(flip? end : end - width, 0), CGPointMake(flip? end - width : end, 0), 0);

Definitely should have spaces before the ternary operator's question marks. Also this (and the one below) are quite long lines, maybe pull the points out into locals? I'm not sure.

> Source/WebCore/platform/graphics/cg/GradientCG.cpp:148
> +                    CGContextRestoreGState(platformContext);

The weird "where's the save that corresponds to this restore" thought that popped into my head illustrates precisely why we like RAII objects for things like this :) Please use GraphicsContextStateSaver or CGContextStateSaver instead of this manual goop.

> Source/WebCore/platform/graphics/cg/GradientCG.cpp:153
> +                U_FALLTHROUGH;

just FALLTHROUGH;, no U_

> Source/WebCore/platform/graphics/cg/GradientCG.cpp:160
>              bool needScaling = data.aspectRatio != 1;

Should you use the already-computed-and-stored gradient instead of calling platformGradient() again below? (I think so!)

> Source/WebCore/svg/SVGStopElement.cpp:102
> +    return colorWithOverrideAlpha(svgStyle.stopColor().rgb(), svgStyle.stopColor().alpha() / 255.0 * svgStyle.stopOpacity());

Might want a fastDivideBy255? (Also not totally sure what's happening here).

> LayoutTests/svg/gradients/spreadMethodAlpha-expected.svg:5
> +    <rect x="10" y="120" width="115" height="55" fill="rgba(255,0,0,0.5)"/>

We try not to use red in "good" results (I understand that the W3C test above does, but ... don't follow them :) )

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180911/07fa3555/attachment.html>