[webkit-reviews] review granted: [Bug 52162] Implement CSS3 Images cross-fade() image function : [Attachment 115308] Render Cross-Fade (Windows build fix, hopefully)
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Nov 16 09:33:52 PST 2011
Simon Fraser (smfr) <simon.fraser at apple.com> has granted 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 115308: Render Cross-Fade (Windows build fix, hopefully)
https://bugs.webkit.org/attachment.cgi?id=115308&action=review
------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=115308&action=review
> LayoutTests/css3/images/cross-fade-tiled.html:9
> + -webkit-transform:translateZ(0);
Why the translateZ()?
> Source/WebCore/css/CSSCrossfadeValue.cpp:42
> +CachedImage* cachedImageForCSSValue(PassRefPtr<CSSValue>, const
RenderObject*, const IntSize&);
> +void loadSubimage(PassRefPtr<CSSValue>, CachedResourceLoader*);
> +bool subimageIsPending(PassRefPtr<CSSValue>);
You could just move the functions to the stop of the file and make them static.
> Source/WebCore/css/CSSCrossfadeValue.cpp:62
> + IntSize size;
> +
> + CachedImage* fromImage = cachedImageForCSSValue(m_fromImage, renderer,
size);
> + CachedImage* toImage = cachedImageForCSSValue(m_toImage, renderer,
size);
Seems weird to send in an empty size here.
> Source/WebCore/css/CSSCrossfadeValue.cpp:85
> +bool subimageIsPending(PassRefPtr<CSSValue> value)
You should only use PassRefPtr<CSSValue> if you're passing ownership, which
doesn't seem to be the case here. Can you pass a const CSSValue*?
> Source/WebCore/css/CSSCrossfadeValue.cpp:98
> +void loadSubimage(PassRefPtr<CSSValue> value, CachedResourceLoader*
cachedResourceLoader)
Ditto.
> Source/WebCore/css/CSSCrossfadeValue.cpp:113
> +CachedImage* cachedImageForCSSValue(PassRefPtr<CSSValue> value, const
RenderObject* renderer, const IntSize& size)
Ditto.
> Source/WebCore/css/CSSCrossfadeValue.cpp:115
> UNUSED_PARAM(size);
Are you going to use size?
> Source/WebCore/css/CSSCrossfadeValue.h:65
> + // NOTE: We put the ImageObserver in a member instead of inheriting from
it
No need for "NOTE:".
> Source/WebCore/platform/graphics/CrossfadeGeneratedImage.cpp:76
> + context->beginTransparencyLayer(1.0);
1.0 -> 1
> Source/WebCore/platform/graphics/CrossfadeGeneratedImage.h:62
> + CachedImage* m_fromImage;
> + CachedImage* m_toImage;
I'd like to see a comment about who owns these CachedImages.
More information about the webkit-reviews
mailing list