[webkit-reviews] review granted: [Bug 119955] Support interpolation between cross-fade() images : [Attachment 210459] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Dirk Schulze <krit at webkit.org>'s
request for review:
Bug 119955: Support interpolation between cross-fade() images
https://bugs.webkit.org/show_bug.cgi?id=119955

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

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


> LayoutTests/ChangeLog:10
> +	   * animations/cross-fade-background-image.html:

Can we change this into a reference test instead of a pixel test?

Also, we normally try to avoid having any red in expected results from tests,
but I see red in this one.

> Source/WebCore/css/CSSCrossfadeValue.h:72
> +    bool equalInputImages(const CSSCrossfadeValue&) const;

Naming here is a bit awkward. The function name “equal input images” sounds
like a function that returns two equal images, not a function that answers the
question, do these fades have input images that are equal.

> Source/WebCore/page/animation/CSSPropertyAnimation.cpp:332
> +	       CSSCrossfadeValue* fromCrossfade =
toCSSCrossfadeValue(fromGenerated);
> +	       CSSCrossfadeValue* toCrossfade =
toCSSCrossfadeValue(toGenerated);
> +	       if (fromCrossfade->equalInputImages(*toCrossfade)) {
> +		   RefPtr<CSSCrossfadeValue> result =
toCrossfade->blend(*fromCrossfade, progress);
> +		   return StyleGeneratedImage::create(result.get());
> +	       }

I recommend writing this with references instead of pointers and eliminating
the local variable for tighter code:

    if (fromGenerated->isCrossfadeValue() && toGenerated->isCrossfadeValue()) {

	CSSCrossfadeValue& fromCrossfade = *toCSSCrossfadeValue(fromGenerated);

	CSSCrossfadeValue& toCrossFade = *toCSSCrossfadeValue(toGenerated);
	if (fromCrossfade.equalInputImages(toCrossfade))
	    return StyleGeneratedImage::create(toCrossfade.blend(fromCrossfade,
progress).get());
    }

Might even want to use shorter variable names since these have tiny scope.