[Webkit-unassigned] [Bug 73074] StyleGeneratedImage should ref CSSImageGeneratorValue

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=73074


Nikolas Zimmermann <zimmermann at kde.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #116497|review?                     |review-
               Flag|                            |




--- Comment #3 from Nikolas Zimmermann <zimmermann at kde.org>  2011-11-24 06:13:30 PST ---
(From update of attachment 116497)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.