Bug 73074 - StyleGeneratedImage should ref CSSImageGeneratorValue
Summary: StyleGeneratedImage should ref CSSImageGeneratorValue
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-24 02:26 PST by Antti Koivisto
Modified: 2011-11-25 10:12 PST (History)
6 users (show)

See Also:


Attachments
patch (31.65 KB, patch)
2011-11-24 04:28 PST, Antti Koivisto
zimmermann: review-
Details | Formatted Diff | Diff
patch2 (18.71 KB, patch)
2011-11-25 05:30 PST, Antti Koivisto
gustavo: commit-queue-
Details | Formatted Diff | Diff
patch3 (18.66 KB, patch)
2011-11-25 05:37 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch4 (21.49 KB, patch)
2011-11-25 09:08 PST, Antti Koivisto
kling: review+
kling: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2011-11-24 02:26:25 PST
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.
Comment 1 Antti Koivisto 2011-11-24 04:28:27 PST
Created attachment 116497 [details]
patch
Comment 2 WebKit Review Bot 2011-11-24 04:29:51 PST
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.
Comment 3 Nikolas Zimmermann 2011-11-24 06:13:30 PST
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.
Comment 4 Nikolas Zimmermann 2011-11-24 06:27:57 PST
(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.
Comment 5 Antti Koivisto 2011-11-25 05:14:07 PST
(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.
Comment 6 Antti Koivisto 2011-11-25 05:30:57 PST
Created attachment 116604 [details]
patch2
Comment 7 Gustavo Noronha (kov) 2011-11-25 05:36:11 PST
Comment on attachment 116604 [details]
patch2

Attachment 116604 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10642368
Comment 8 Antti Koivisto 2011-11-25 05:37:11 PST
Created attachment 116606 [details]
patch3
Comment 9 Antti Koivisto 2011-11-25 09:08:48 PST
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.
Comment 10 Antti Koivisto 2011-11-25 09:27:33 PST
I mean StyleGeneratedImage constructor.
Comment 11 Andreas Kling 2011-11-25 09:57:19 PST
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?
Comment 12 Antti Koivisto 2011-11-25 10:09:16 PST
http://trac.webkit.org/changeset/101177
Comment 13 Antti Koivisto 2011-11-25 10:12:41 PST
(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.