[webkit-reviews] review denied: [Bug 52162] Implement CSS3 Images cross-fade() image function : [Attachment 115027] Render Cross-Fade (plus build fix for non-debug)

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Tim Horton
<timothy_horton at apple.com>'s request for review:
Bug 52162: Implement CSS3 Images cross-fade() image function
https://bugs.webkit.org/show_bug.cgi?id=52162

Attachment 115027: Render Cross-Fade (plus build fix for non-debug)
https://bugs.webkit.org/attachment.cgi?id=115027&action=review

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


This is a good start! I'd like to see a test case with a tiled cross-fade
image.

> LayoutTests/css3/images/cross-fade-invalidation.html:16
> +    if(window.layoutTestController)
> +	layoutTestController.notifyDone();

Need to indent there

> Source/WebCore/css/CSSCrossfadeValue.cpp:71
> +    if (value->isImageValue())
> +	   return
static_cast<CSSImageValue*>(value.get())->cachedOrPendingImage()->isPendingImag
e();
> +
> +    if (value->isImageGeneratorValue())
> +	   return
static_cast<CSSImageGeneratorValue*>(value.get())->isPending();

These two bare functions look like candidates for a virtual method.

> Source/WebCore/css/CSSCrossfadeValue.cpp:117
> +    CachedImage* fromImage, * toImage;

We don't declare multiple variables on one line like this.

> Source/WebCore/css/CSSCrossfadeValue.cpp:120
> +    fromImage = cachedImageForCSSValue(m_fromImage, renderer, size);
> +    toImage = cachedImageForCSSValue(m_toImage, renderer, size);

Initialize at declaration!

> Source/WebCore/css/CSSCrossfadeValue.h:42
> +    static PassRefPtr<CSSCrossfadeValue> create(PassRefPtr<CSSValue>
fromImage, PassRefPtr<CSSValue> toImage) { return adoptRef(new
CSSCrossfadeValue(fromImage, toImage)); }

Break this onto 3 lines.

> Source/WebCore/css/CSSCrossfadeValue.h:43
> +    ~CSSCrossfadeValue() { }

You can remove this.

> Source/WebCore/css/CSSImageGeneratorValue.cpp:117
> +    if (isPending())
> +	   m_image = StylePendingImage::create(this).get();

It seems odd that calling this method has the side-effect of changing m_image.

> Source/WebCore/css/CSSImageGeneratorValue.cpp:198
> +    case CrossfadeClass:
> +	   return static_cast<const CSSCrossfadeValue*>(this)->isPending();
> +    case CanvasClass:
> +	   return static_cast<const CSSCanvasValue*>(this)->isPending();
> +    case LinearGradientClass:
> +	   return static_cast<const
CSSLinearGradientValue*>(this)->isPending();
> +    case RadialGradientClass:
> +	   return static_cast<const
CSSRadialGradientValue*>(this)->isPending();
> +    default:

Erm, why not use virtual methods?

> Source/WebCore/platform/graphics/CrossfadeGeneratedImage.cpp:45
> +    m_size = size;

Does the superclass not have a ctor that sets the size?

> Source/WebCore/platform/graphics/CrossfadeGeneratedImage.cpp:64
> +    float inversePercentage = 1.0f - m_percentage;

Use 1, not 1.0f

> Source/WebCore/platform/graphics/CrossfadeGeneratedImage.cpp:69
> +    fromImage = m_fromImage->image();
> +    fromImageSize = fromImage->size();
> +    toImage = m_toImage->image();
> +    toImageSize = toImage->size();

Declare at first use. This is C++, not C :)

> Source/WebCore/platform/graphics/CrossfadeGeneratedImage.cpp:76
> +    crossfadeImageSize = IntSize(fromImageSize.width() * inversePercentage +
toImageSize.width() * m_percentage,
> +				    fromImageSize.height() * inversePercentage
+ toImageSize.height() * m_percentage);

Does the spec say anything about interpolating sizes? I'm not sure it's correct
to animate the size too.

> Source/WebCore/platform/graphics/CrossfadeGeneratedImage.h:73
> +    : m_ownerValue(ownerValue)
> +    , m_ready(false) { }

These should be indented.

> Source/WebCore/platform/graphics/CrossfadeGeneratedImage.h:76
> +    virtual void imageChanged(CachedImage* image, const IntRect* rect = 0)
OVERRIDE {

{ on new line.