[webkit-reviews] review denied: [Bug 73074] StyleGeneratedImage should ref CSSImageGeneratorValue : [Attachment 116497] patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Antti Koivisto
<koivisto at iki.fi>'s request for review:
Bug 73074: StyleGeneratedImage should ref CSSImageGeneratorValue
https://bugs.webkit.org/show_bug.cgi?id=73074

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

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=116497&action=review


Much nicer approach! Though style bot & me still see some issues:

> Source/WebCore/css/CSSStyleApplyProperty.cpp:898
> -    setPropertyHandler(CSSPropertyBackgroundImage,
ApplyPropertyFillLayer<StyleImage*, CSSPropertyBackgroundImage,
BackgroundFillLayer, &RenderStyle::accessBackgroundLayers,
&RenderStyle::backgroundLayers,
> +    setPropertyHandler(CSSPropertyBackgroundImage,
ApplyPropertyFillLayer<StyleImage*, PassRefPtr<StyleImage>,
CSSPropertyBackgroundImage, BackgroundFillLayer,
&RenderStyle::accessBackgroundLayers, &RenderStyle::backgroundLayers,
>			  &FillLayer::isImageSet, &FillLayer::image,
&FillLayer::setImage, &FillLayer::clearImage, &FillLayer::initialFillImage,
&CSSStyleSelector::mapFillImage>::createHandler());
>  
> -    setPropertyHandler(CSSPropertyBackgroundOrigin,
ApplyPropertyFillLayer<EFillBox, CSSPropertyBackgroundOrigin,
BackgroundFillLayer, &RenderStyle::accessBackgroundLayers,
&RenderStyle::backgroundLayers,
> +    setPropertyHandler(CSSPropertyBackgroundOrigin,
ApplyPropertyFillLayer<EFillBox, EFillBox, CSSPropertyBackgroundOrigin,
BackgroundFillLayer, &RenderStyle::accessBackgroundLayers,
&RenderStyle::backgroundLayers,

I don't like this kind of mechanic change, instead you could use a "decorator"
which figures out the right type.
template<typename T>
struct Decorator {
     typedef typename T Setter;
     typedef typename T Getter;
};

template<>
struct Decorator<StyleImage> {
    typedef typename PassRefPtr<StyleImage> Setter;
    typedef typename StyleImage* Getter;
};

What do you think?

> Source/WebCore/css/CSSStyleSelector.cpp:5775
> +			   RefPtr<StyleImage> loadedImage =
loadPendingImage(static_cast<StylePendingImage*>(backgroundLayer->image()));
> +			   backgroundLayer->setImage(loadedImage.release());

I'd combine this to avoid a unncessary ref'ing.

> Source/WebCore/css/CSSStyleSelector.cpp:5787
> -			       StyleImage* loadedImage =
loadPendingImage(static_cast<StylePendingImage*>(image));
> +			       RefPtr<StyleImage> loadedImage =
loadPendingImage(static_cast<StylePendingImage*>(image));
>			       if (loadedImage)
> -				  
static_cast<ImageContentData*>(contentData)->setImage(loadedImage);
> +				  
static_cast<ImageContentData*>(contentData)->setImage(loadedImage.release());

If we handle null images in ImageContentData::setImage(), this could turn into
a simple
static_cast<ImageContentData*>(contentData)->setImage(loadPendingImage(...

> Source/WebCore/css/CSSStyleSelector.cpp:5800
> +				   RefPtr<StyleImage> loadedImage =
loadPendingImage(static_cast<StylePendingImage*>(image));
> +				  
currentCursor.setImage(loadedImage.release());

Could be combined into a single statement.

> Source/WebCore/css/CSSStyleSelector.cpp:5810
> +		       RefPtr<StyleImage> loadedImage =
loadPendingImage(static_cast<StylePendingImage*>(m_style->listStyleImage()));
> +		       m_style->setListStyleImage(loadedImage.release());

Ditto.

> Source/WebCore/css/CSSStyleSelector.cpp:5817
> +		       RefPtr<StyleImage> loadedImage =
loadPendingImage(static_cast<StylePendingImage*>(m_style->borderImageSource()))
;
> +		       m_style->setBorderImageSource(loadedImage.release());

Ditto.

> Source/WebCore/css/CSSStyleSelector.cpp:5826
> +			   RefPtr<StyleImage> loadedImage =
loadPendingImage(static_cast<StylePendingImage*>(maskImage.image()));
> +			  
reflection->setMask(NinePieceImage(loadedImage.release(),
maskImage.imageSlices(), maskImage.fill(), maskImage.borderSlices(),
maskImage.outset(), maskImage.horizontalRule(), maskImage.verticalRule()));

Ditto (though here it would really hurt the readability)

> Source/WebCore/css/CSSStyleSelector.cpp:5834
> +		       RefPtr<StyleImage> loadedImage =
loadPendingImage(static_cast<StylePendingImage*>(m_style->maskBoxImageSource())
);
> +		       m_style->setMaskBoxImageSource(loadedImage.release());

Ditto.

> Source/WebCore/css/CSSStyleSelector.cpp:5842
> +			   RefPtr<StyleImage> loadedImage =
loadPendingImage(static_cast<StylePendingImage*>(maskLayer->image()));
> +			   maskLayer->setImage(loadedImage.release());

Ditto.