[webkit-reviews] review denied: [Bug 65769] New renderer for SVGRectElement : [Attachment 103073] Proposed patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Renata Hodovan <reni at webkit.org>'s
request for review:
Bug 65769: New renderer for SVGRectElement
https://bugs.webkit.org/show_bug.cgi?id=65769

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

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=103073&action=review


We need performance tests for this. If we have stupid graphic libs that
recreate a path internally for every fillRect or strokeRect operation, this
would be significant slower! That's why I'd begin with circles/ellipses, there
is a very good Test to check the performance for circles, but it can indeed be
adapted to rects. http://themaninblue.com/experiment/AnimationBenchmark/svg

But again, we need to test this on all platforms! I'll be willing to run the
tests on mac as well, if that helps. I can try CG but maybe Skia as well.

The following comment are not a complete review. I have more comments later, we
just need to clarify some fact first. 

A lot of code looks like code duplication, can't you just call shapeSpecific
functions in RenderSVGBasicShape? Do we need all these paint, layout,
fillAndStroke, nodeAtPoint functions again?

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:189
> +    if (RenderSVGResource* fillPaintingResource =
RenderSVGResource::fillPaintingResource(this, style, fallbackColor)) {

Is it really a fallback _color_ ? Can't a gradient or pattern be a fallback as
well? I have to look for that myself first. Seems not to be your problem anyway
here.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:196
> +		   Path path;
> +		   path.addRoundedRect(m_rect, FloatSize(m_rxValue,
m_ryValue));
> +		   context->fillPath(path);

That is a bad idea. On every Paint operation we have to recreate the path to
draw it. This is significant slower than the current behavior, where we store
the path. We should fallback to BasicShape in this situation. Or you could
store a RefPtr<Path> like we do in BasicShape as well.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:207
> +		       Path path;
> +		       path.addRoundedRect(m_rect, FloatSize(m_rxValue,
m_ryValue));
> +		       context->fillPath(path);

ditto.