[webkit-reviews] review granted: [Bug 121822] Move more style change code from RenderObject to RenderElement : [Attachment 212424] patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Antti Koivisto <koivisto at iki.fi>'s
request for review:
Bug 121822: Move more style change code from RenderObject to RenderElement
https://bugs.webkit.org/show_bug.cgi?id=121822

Attachment 212424: patch
https://bugs.webkit.org/attachment.cgi?id=212424&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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?