Bug 121822 – Move more style change code from RenderObject to RenderElement

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Antti Koivisto]

Very little of it is needed for RenderText.

[Antti Koivisto]

Created attachment 212424 [details]
patch

[Darin Adler]

Comment on attachment 212424 [details]
patch

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

> Source/WebCore/ChangeLog:11
> +            RenderTexts are no longer registered as image clients. They don't need 

This sentence ends right in the middle of

> Source/WebCore/rendering/RenderElement.h:36
> +    virtual void setStyle(PassRefPtr<RenderStyle>);

I think you should mark this OVERRIDE, even though it’s "= 0" in the base class. Still useful to get an error if the function signatures don’t match. Like when we change from PassRefPtr to just RefPtr some day, if we do it wrong.

> Source/WebCore/rendering/RenderElement.h:82
> +    virtual void styleWillChange(StyleDifference, const RenderStyle*);
> +    virtual void styleDidChange(StyleDifference, const RenderStyle*);

If not OVERRIDE then does this still need to be virtual? Do derived classes override this?

> Source/WebCore/rendering/RenderElement.h:106
> +    void updateFillImages(const FillLayer*, const FillLayer*);
> +    void updateImage(StyleImage*, StyleImage*);
> +#if ENABLE(CSS_SHAPES)
> +    void updateShapeImage(const ShapeValue*, const ShapeValue*);
> +#endif
> +    StyleDifference adjustStyleDifference(StyleDifference, unsigned contextSensitiveProperties) const;

I’d like this better with a blank line after the #endif.

> Source/WebCore/rendering/RenderText.cpp:203
> +void RenderText::setStyle(PassRefPtr<RenderStyle> style)

Since RenderText objects inherit their style from their parents, isn’t there something even more efficient we can do?

> Source/WebCore/rendering/RenderText.cpp:210
> +    if (oldStyle.get())

Should not need a .get() here.

> Source/WebCore/rendering/RenderText.h:154
> +    virtual void styleDidChange(StyleDifference, const RenderStyle* oldStyle);

Why is this still virtual, if it’s not an override? Are there classes derived from RenderText that override this?

[Antti Koivisto]

> This sentence ends right in the middle of

That's because I

> I think you should mark this OVERRIDE, even though it’s "= 0" in the base class. Still useful to get an error if the function signatures don’t match. Like when we change from PassRefPtr to just RefPtr some day, if we do it wrong.

Yeah.

> 
> > Source/WebCore/rendering/RenderElement.h:82
> > +    virtual void styleWillChange(StyleDifference, const RenderStyle*);
> > +    virtual void styleDidChange(StyleDifference, const RenderStyle*);
> 
> If not OVERRIDE then does this still need to be virtual? Do derived classes override this?

Yes, they have overrides.

> Since RenderText objects inherit their style from their parents, isn’t there something even more efficient we can do?

The plan is to move m_style pointer to RenderElement and have RenderText just use the parent style. That will require a few more patches.

> > Source/WebCore/rendering/RenderText.h:154
> > +    virtual void styleDidChange(StyleDifference, const RenderStyle* oldStyle);
> 
> Why is this still virtual, if it’s not an override? Are there classes derived from RenderText that override this?

Surprisingly there are RenderText subclasses and there is an override for this.

[Antti Koivisto]

https://trac.webkit.org/r156325