[Webkit-unassigned] [Bug 122371] Move paint() to RenderElement

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=122371


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #213435|review?                     |review+
               Flag|                            |




--- Comment #4 from Darin Adler <darin at apple.com>  2013-10-05 00:35:06 PST ---
(From update of attachment 213435)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.