[webkit-reviews] review granted: [Bug 122371] Move paint() to RenderElement : [Attachment 213435] 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 122371: Move paint() to RenderElement
https://bugs.webkit.org/show_bug.cgi?id=122371

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

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