[webkit-reviews] review denied: [Bug 21725] Should be able to do transitions of background-image, including gradients : [Attachment 133776] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Shane Stephens
<shanestephens at google.com>'s request for review:
Bug 21725: Should be able to do transitions of background-image, including
gradients
https://bugs.webkit.org/show_bug.cgi?id=21725

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=133776&action=review


> Source/WebCore/css/CSSGradientValue.cpp:579
> +float CSSLinearGradientValue::getAngle()

Should be const.

We don't normally use 'get' in method names. It should also be clear what the
units of the return value are.

Shouldn't we implement the new gradient spec (with the reverse angle behavior)
before implementing animation, since all these angles might change?

> Source/WebCore/css/CSSGradientValue.cpp:581
> +    float angle = -1;

-1 seems like a bizarre default. It would be clearer to track the "has not been
set yet" state via a separate bool.

> Source/WebCore/css/CSSGradientValue.cpp:585
> +    if (m_firstX.get()) {

Blank line above this one please.

> Source/WebCore/css/CSSGradientValue.h:57
> +    virtual bool canAnimateWith(PassRefPtr<CSSGradientValue> other) const

PassRefPtr<CSSGradientValue> is for transfer of ownership, which is not
happening here. Should be a const CSSGradientValue& or const CSSGradientValue*

'canAnimateWith' is a slightly odd name for this. I think we have other code
like this which uses the 'blend' terminology.

> Source/WebCore/css/CSSInterpolatedLinearGradientValue.cpp:41
> +static inline float blendFunc(const AnimationBase*, float from, float to,
double progress)
> +{
> +    return narrowPrecisionToFloat(from + (to - from) * progress);
> +}

No need for this. Use WebCore::blend().

> Source/WebCore/css/CSSInterpolatedLinearGradientValue.cpp:56
> +    float angle = blendFunc(0, fromAngle, toAngle, amount);

Just use blend() here.

> Source/WebCore/css/CSSInterpolatedLinearGradientValue.cpp:65
> +    // create result gradient from calculated interpolated angle, to
correctly set up end points.

Comments should have sentence case.

> Source/WebCore/css/CSSInterpolatedLinearGradientValue.cpp:81
> +	       blendFunc(0, fromStop.stop, toStop.stop, m_amount),
> +	       blendFunc(0, fromStop.red, toStop.red, m_amount),
> +	       blendFunc(0, fromStop.green, toStop.green, m_amount),
> +	       blendFunc(0, fromStop.blue, toStop.blue, m_amount),
> +	       blendFunc(0, fromStop.alpha, toStop.alpha, m_amount)));

Can't you call blend() directly?

So here you're interpolating stops after the length units have been resolved. I
think this is wrong; i think you should interpolate CSS Lengths as we do
elsewhere (which requires the two gradients to use matching units).

> Source/WebCore/css/CSSInterpolatedRadialGradientValue.cpp:41
> +static inline float blendFunc(const AnimationBase*, float from, float to,
double progress)
> +{
> +    return narrowPrecisionToFloat(from + (to - from) * progress);
> +}

Ditto.

> Source/WebCore/css/CSSInterpolatedRadialGradientValue.cpp:85
> +	   result->addColorStop(Gradient::ColorStop(blendFunc(0, fromStop.stop,
toStop.stop, m_amount),
> +						    blendFunc(0, fromStop.red,
toStop.red, m_amount),
> +						    blendFunc(0,
fromStop.green, toStop.green, m_amount),
> +						    blendFunc(0, fromStop.blue,
toStop.blue, m_amount),
> +						    blendFunc(0,
fromStop.alpha, toStop.alpha, m_amount)));
> +    }

Ditto.