[webkit-reviews] review requested: [Bug 65769] New renderer for SVGRectElement : [Attachment 105641] Proposed 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 105641: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=105641&action=review

------- Additional Comments from Renata Hodovan <reni at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=105479&action=review


>> Source/WebCore/rendering/svg/RenderSVGPath.h:34
>> +class RenderSVGPath : public RenderSVGShape {
> 
> Add a FIXME that you plan to remove RenderSVGPath with all the changes in DRT
afterwards in a followup.

Done.

>> Source/WebCore/rendering/svg/RenderSVGRect.cpp:51
>> +	SVGRectElement* rect = static_cast<SVGRectElement*>(node());
> 
> Can you add an assert here for rect please?

Done.

>> Source/WebCore/rendering/svg/RenderSVGRect.cpp:55
>> +	if (m_hasRx || m_hasRy ||
!style()->svgStyle()->strokeDashArray().isEmpty()) {
> 
> just create the path for rects with rounded corners. More later on this
review...

Done.

>> Source/WebCore/rendering/svg/RenderSVGRect.cpp:96
>> +	    context->fillRect(m_rect);
> 
> even if we have to create paths for pointer events, it could be faster to use
context->fillRect instead of filling the path (of course just if fillRect
respects dashArray and different styles; you have to check this first). We just
need path for rects with rounded corners.

Okay, fillRect() can handle these styles. Updated.

>> Source/WebCore/rendering/svg/RenderSVGRect.cpp:105
>> +	context->strokeRect(m_rect, this->strokeWidth());
> 
> Ditto

Done.

>> Source/WebCore/rendering/svg/RenderSVGRect.cpp:108
>> +bool RenderSVGRect::strokeContainsSlowCase(const FloatPoint& point)
> 
> what is the fast case?

This virtual function will contain the shape specific calculation of
strokeContains() operation.

>> Source/WebCore/rendering/svg/RenderSVGRect.cpp:110
>> +	if (hasPath())
> 
> You just created a path for dash arrays. But there is more stuff to respect:
lineCap, mitterLimit. The W3C test suite has more examples. This is not needed
for stroking or filling a rect, since the graphic libraries support it with
rects automatically. But you have to think about this on pointer events.
> 
> I'd just create the path, for _pointer events_ if we have either have a
dahsarray, some special settings for mitterlimit or lineCap, and not on every
layout call. Use the stored rects as much as possible. Just fallback to the
path if there is no other way to determine something. Would it make sense to
store a boolean for isPath() in RenderShape instead of checking the pointer?
Not sure if we loose time on the check.

Done.

>> Source/WebCore/rendering/svg/RenderSVGRect.cpp:121
>> +		&& determinePointLocation(point, outerStrokeRect) !=
OutsideShape) ?  true : false;
> 
> you should store inner and outer rect. But don't forget to clear the rects on
style changes. This could be

I'm not sure it's worth to store them constatly, so I left out this
modification fo far. But if you are sure, I will do it ofc. Btw where should I
catch the style change in this case?

>> Source/WebCore/rendering/svg/RenderSVGRect.cpp:126
>> +	return (determinePointLocation(point, m_rect) != OutsideShape) ? true :
false;
> 
> isn't m_rect.contains(point.x(), point.y()) enough here? Also, you are
missing the rounded corner case.
> 
> And I'd like to see a test case for pointer events with rounded corners, if
you didn't see regressions.

It's enuogh. I just wanted to use a uniform way to determine the location of
the points. But determainPointLocation() is fade into the past, so this
solution is a trash too.

>> Source/WebCore/rendering/svg/RenderSVGRect.cpp:134
>> +	return (rect.x() < point.x() && rect.maxX() > point.x() && rect.y() <
point.y() && rect.maxY() > point.y()) ? InsideShape : OnStroke;
> 
> Can we add a comprises() method in FloatRect, that does not include the
boundingBox of the rect? So we would not need this method here.

Done.

>> Source/WebCore/rendering/svg/RenderSVGResourceSolidColor.cpp:83
>> +	if (/*path && */!(resourceMode & ApplyToTextMode)) {
> 
> can you remove this comment?

Done.

>> Source/WebCore/rendering/svg/RenderSVGShape.cpp:75
>> +	return path().isEmpty();
> 
> why not just m_path->isEmpty();?

I do already :P

>> Source/WebCore/rendering/svg/RenderSVGShape.cpp:91
>> +	return path().strokeBoundingRect(&strokeStyle);
> 
> m_path->stroke...

Done.

>> Source/WebCore/rendering/svg/RenderSVGShape.cpp:102
>> +	return path().strokeContains(&applier, point);
> 
> m_path->strokeC...

Done.

>> Source/WebCore/rendering/svg/RenderSVGShape.cpp:107
>> +	return path().contains(point, fillRule);
> 
> m_path->contains(...

Done.

>> Source/WebCore/rendering/svg/RenderSVGShape.h:108
>> +protected:
> 
> The order should be public, protected, private sections

Done.

>> Source/WebCore/rendering/svg/RenderSVGShape.h:127
>> +	};
> 
> Looks like you don't use it. Remove it please.

Done.