RenderText does not paint itself.
Created attachment 213435 [details] patch
Comment on attachment 213435 [details] patch Attachment 213435 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/3335002
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
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.
(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.
Created attachment 213448 [details] for bots
https://trac.webkit.org/r156952
This made debug bots crash with an assertion, and Antti is not on IRC, so I'm going to roll out.
Re-opened since this is blocked by bug 122381
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
That assert is an existing bug revealed by switching to element iterators.
Relanded in https://trac.webkit.org/r157011