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.
Created attachment 116497 [details] patch
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 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.
(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.
(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.
Created attachment 116604 [details] patch2
Comment on attachment 116604 [details] patch2 Attachment 116604 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10642368
Created attachment 116606 [details] patch3
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.
I mean StyleGeneratedImage constructor.
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?
http://trac.webkit.org/changeset/101177
(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.