Bug 122371 – Move paint() to RenderElement

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

[Antti Koivisto]

RenderText does not paint itself.

[Antti Koivisto]

Created attachment 213435 [details]
patch

[EFL EWS Bot]

Comment on attachment 213435 [details]
patch

Attachment 213435 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/3335002

[EFL EWS Bot]

Comment on attachment 213435 [details]
patch

Attachment 213435 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/3339031

[Darin Adler]

Comment on attachment 213435 [details]
patch

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

> Source/WebCore/rendering/RenderBox.cpp:1105
> +        if (!child->isRenderElement())
> +            continue;

Why is this check needed? We didn’t add a check in RenderFrameSet::paint, but we did here. I think the old code would have asserted if it ever called paint on a text renderer. And toRenderElement will assert. Is adding the check fixing some kind of bug?

> Source/WebCore/rendering/RenderElement.h:90
> +    virtual void paint(PaintInfo&, const LayoutPoint&) { }

Could this be pure virtual instead of empty? Are there any concrete element classes that actually paint nothing?

> Source/WebCore/rendering/RenderObject.cpp:1241
>  
> -void RenderObject::paint(PaintInfo&, const LayoutPoint&)
> -{
> -}
>  

Should remove one of the blank lines too.

> Source/WebCore/rendering/svg/RenderSVGContainer.cpp:144
> +                if (!child->isRenderElement())
> +                    continue;

Why is this check needed? We didn’t add a check in RenderFrameSet::paint, but we did here. I think the old code would have asserted if it ever called paint on a text renderer. And toRenderElement will assert. Is adding the check fixing some kind of bug?

> Source/WebCore/rendering/svg/RenderSVGResourceMasker.cpp:121
> +    auto svgChildren = childrenOfType<SVGElement>(&maskElement());

I think it would be nicer to call this just children, even if it’s limited to the SVG children. I just like words better than acronym-word combinations. Same applies to the other copies of this code in other files.

Looks like some non-Mac builds require an ElementChildIterator.h include in this file. I think that’s why the EWS bots are red.

> Source/WebCore/rendering/svg/RenderSVGResourceMasker.cpp:124
> +        SVGElement& child = *it;
> +        RenderElement* renderer = child.renderer();

I would have written:

    auto renderer = it->renderer();

No reason to have to repeat the type SVGElement nor the type RenderElement* and it would be nice to get a more specific type automatically in the future if possible.

Same applies to the other copies of this code in other files.

> Source/WebCore/rendering/svg/SVGRenderingContext.h:83
> +    static void renderSubtreeToImageBuffer(ImageBuffer*, RenderElement*, const AffineTransform&);

Could change this to RenderElement&; all the callers are already null checking it.

[Antti Koivisto]

(In reply to comment #4)
> Why is this check needed? We didn’t add a check in RenderFrameSet::paint, but we did here. I think the old code would have asserted if it ever called paint on a text renderer. And toRenderElement will assert. Is adding the check fixing some kind of bug?

It can be easily shown that RenderFrameSet can only have RenderElement children. This is definitely not true for all RenderBoxes. Almost everyone has their specialised paint but at least RenderSVGRoot calls to RenderBox::paint(). I'm not sure it can't have RenderSVGInlineText children.

The right move would probably be to get rid of RenderBox::paint completely.

> > Source/WebCore/rendering/RenderElement.h:90
> > +    virtual void paint(PaintInfo&, const LayoutPoint&) { }
> 
> Could this be pure virtual instead of empty? Are there any concrete element classes that actually paint nothing?

Just RenderLineBreak. I'll add empty paint there and make this pure virtual.

> Why is this check needed? We didn’t add a check in RenderFrameSet::paint, but we did here. I think the old code would have asserted if it ever called paint on a text renderer. And toRenderElement will assert. Is adding the check fixing some kind of bug?

Same as RenderBox. I can't easily prove that this can't happen. Old code would still be ok in release build. With new code we would have unsafe cast.

[Antti Koivisto]

Created attachment 213448 [details]
for bots

[Antti Koivisto]

https://trac.webkit.org/r156952

[Alexey Proskuryakov]

This made debug bots crash with an assertion, and Antti is not on IRC, so I'm going to roll out.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 122381

[Alexey Proskuryakov]

Rolled out in <https://trac.webkit.org/r156954>.

Crash log is here:

http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK2%20(Tests)/r156952%20(13137)/svg/custom/large-image-pattern-crash-crash-log.txt

[Antti Koivisto]

That assert is an existing bug revealed by switching to element iterators.

[Antti Koivisto]

Relanded in https://trac.webkit.org/r157011