[webkit-reviews] review requested: [Bug 65769] New renderer for SVGRectElement : [Attachment 104684] Draft patch

[bugzilla-daemon at webkit.org]

Renata Hodovan <reni at webkit.org> has asked  for review:
Bug 65769: New renderer for SVGRectElement
https://bugs.webkit.org/show_bug.cgi?id=65769

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

------- Additional Comments from Renata Hodovan <reni at webkit.org>
It's still a draft patch (without changelog and layout test updates).

>> Source/WebCore/rendering/svg/RenderSVGRect.cpp:68
>> +	   RenderSVGShape::createShape();
> 
> The fallback path should be described with a comment.
> Is this enough? What about combinations of stroke-linecap="join" and
different miter limits? I'm sure Dirk can come up with more cases.

I'm waiting for the other fallback ideas :)

>> Source/WebCore/rendering/svg/RenderSVGRect.cpp:80
>> +	float yValue = rect->y().value(rect);
> 
> Side note: This code is derived from SVGRectElement::toPathData.
> It should be possible to eliminate this method as well now - aka. move it
into RenderSVGRect, and use it from RenderSVGShape when path fallback mode is
enabled.
> Needs a bit more investigation, but I wanted to mention it so we won't
forget.

I agree with this but I think it should be made in a follow-up patch (I should
do the same with the toPathData() funtions of the other SVG elements, which are
not touched by this patch)

>> Source/WebCore/rendering/svg/RenderSVGShape.cpp:226
>> +	tempPath = path();
> 
> tempPath = m_path is faster.

m_path is an OwnPtr, so tempPath = m_path doesn't work. I could change it to
tempPath = *m_path; if you like it better.

>> Source/WebCore/rendering/svg/RenderSVGShape.cpp:268
>> +	    usePath = pathPtr();
> 
> usePath = &m_path is faster.

The same problem here. Maybe: usePath = m_path.get();  ?

>> Source/WebCore/rendering/svg/RenderSVGShape.cpp:272
>> +	    usePath = pathPtr();
> 
> Ditto.

The same again...

>> Source/WebCore/rendering/svg/RenderSVGShape.cpp:394
>> +	m_fillBoundingBox = shapeBoundingRect();
> 
> Maybe RenderSVGRects shapeBoundingRect should calculate the x/y/w/h FloatRect
here, instead of returning m_rect.
> This way we could save the additional m_rect in RenderSVGRect, maybe?

I think we need to store this because m_fillBoundingBox is used at several
places.

>> Source/WebCore/rendering/svg/SVGRenderTreeAsText.cpp:293
>>	    const RenderSVGPath& path = static_cast<const
RenderSVGPath&>(object);
> 
> Be careful here! this is wrong and crashy. Needs investigation. I'd
immediately think you want to have two isSVGPath/isSVGRect branches
> and then share code for in "writeSVGShape" that are common to path and rect.

This part really was a bit messy. I refactored it.

All the other suggestions are fixed. Besides I made a little refatoring around
the fallback of strokeContains.