Bug 73074 – StyleGeneratedImage should ref CSSImageGeneratorValue

• Component: CSS
• Status: RESOLVED FIXED

[Antti Koivisto]

RenderStyle owns StyleImage objects. However StyleGeneratedImage does not ref the CSSImageGeneratorValue it holds. It should do this to guarantee that the CSSImageGeneratorValue does not die before the RenderStyle does.

[Antti Koivisto]

Created attachment 116497 [details]
patch

[WebKit Review Bot]

Attachment 116497 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/css/CSSStyleSelector.h:346:  The return type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Source/WebCore/css/CSSStyleApplyProperty.cpp:889:  Missing space after ,  [whitespace/comma] [3]
Source/WebCore/css/CSSStyleSelector.cpp:5743:  The return type should use PassRefPtr instead of RefPtr.  [readability/pass_ptr] [5]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikolas Zimmermann]

Comment on attachment 116497 [details]
patch

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.

[Nikolas Zimmermann]

(In reply to comment #3)
> I'd combine this to avoid a unncessary ref'ing.
To correct myself: unnecessary conversion from PassRefPtr to RefPtr and then again to PassRefPtr, where we could instead just pass on the PassRefPtr.

[Antti Koivisto]

(In reply to comment #3)

> 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?

Good idea. Did this.

> 
> > 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.

I don't think there is unnecessary reffing as release() is used consistently. However you are right that the lines can be combined. Did some of this.

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

I don't think that's a good idea. Setters that silently ignore some values (null in this case) are confusing. It might be a behavior change too.

[Antti Koivisto]

Created attachment 116604 [details]
patch2

[Gustavo Noronha (kov)]

Comment on attachment 116604 [details]
patch2

Attachment 116604 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10642368

[Antti Koivisto]

Created attachment 116606 [details]
patch3

[Antti Koivisto]

Created attachment 116631 [details]
patch4

Got rid of the CSSImageGeneratorValue::generatedOrPendingImage() and CSSImageGeneratorValue::generatedImage() as those are now rather pointless. Also cleaned up the CSSImageGeneratorValue constructor.

[Antti Koivisto]

I mean StyleGeneratedImage constructor.

[Andreas Kling]

Comment on attachment 116631 [details]
patch4

View in context: https://bugs.webkit.org/attachment.cgi?id=116631&action=review

r=me. Patch looks great, and I believe you've addressed the issues Nikolas brought up.

> Source/WebCore/css/CSSImageGeneratorValue.cpp:40
>  CSSImageGeneratorValue::CSSImageGeneratorValue(ClassType classType)

Unrelated to this patch: CSSImageGeneratorValue's ctor and dtor could be inlined.

> Source/WebCore/css/CSSStyleApplyProperty.cpp:394
> +    typedef T Setter;
> +    typedef T Getter;

I think Setter/Getter are poor names here. SetterType/GetterType perhaps?

[Antti Koivisto]

http://trac.webkit.org/changeset/101177

[Antti Koivisto]

(In reply to comment #11)
> > Source/WebCore/css/CSSStyleApplyProperty.cpp:394
> > +    typedef T Setter;
> > +    typedef T Getter;
> 
> I think Setter/Getter are poor names here. SetterType/GetterType perhaps?

As discussed, I kept these as is as the names are in the FillLayerAccessorTypes namespace.