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

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.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 107032: Propsed patch
https://bugs.webkit.org/attachment.cgi?id=107032&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=107032&action=review


You're making good progress, still lots of comments from a first quick round of
review. Still didn't check everything in detail.

> LayoutTests/svg/custom/pointer-events-on-rounded-rect-expected.txt:1
> +PASSED: fallbackRect had pointer

No ChangeLog for LayoutTests included?

> LayoutTests/svg/custom/pointer-events-on-svg-with-pointer.xhtml:6
> +<defs>

What's this change about? Needs explanation in the ChangeLog.

> Source/WebCore/ChangeLog:6
> +	   Adding a new framework to support the rendering of SVG shapes.

I'd propose: Adding a new framework to support more efficient rendering of SVG
shapes.

> Source/WebCore/ChangeLog:8
> +	   The main logic takes place in RenderSVGShape class and every
renderer will
> +	   inherit from it. Since its code base is mostly moved from
RenderSVGPath it's suitable to

Well this is not correct. RenderSVGEllipse/Rect etc will inherit from it, but
not RenderSVGText etc. It's worded a bit unfortunate, please fix it :-)

> Source/WebCore/ChangeLog:10
> +

In general this ChangeLog should be more descriptive, this is a heavy change
and thus deserves a good explaination.

> Source/WebCore/platform/graphics/FloatRect.h:149
> +    bool comprises(const FloatPoint&) const;

Interssting, never heard the term "comprises" before. The only difference to
contains is < -> <= right?
Couldn't this be an enum passed to contains() named "ContainsMode"?

> Source/WebCore/rendering/RenderObject.h:359
> +    virtual bool isSVGLine() const { return false; }

Line? This is not yet in, is it?

> Source/WebCore/rendering/svg/RenderSVGPath.h:35
> +// FIXME: RenderSVGPath will be removed when all SVG shapes will be rendered
via RenderSVGShape.
> +class RenderSVGPath : public RenderSVGShape {

Huh, really? I thought it would still stay. It would be confusing if
RenderSVGShape would be the main renderer for <path>, and RenderSVGRect
inheriting from RenderSVGShape for <rect>.
I think we should only let "leaf" classes be used for specific svg elements
like <circle>, <rect>, <path>.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:49
> +    // Clear m_boundingBox.

Does this serve a comment? It's not really helpful, it doesn't state a reason.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:56
> +    bool hasRx = rect->hasAttribute(SVGNames::rxAttr);
> +    bool hasRy = rect->hasAttribute(SVGNames::ryAttr);
> +    // Fallback to RenderSVGShape if rect has special features.
> +    if (hasRx || hasRy) {

I'd rather see the fallback code decision in a seperated function:
bool shouldFallbackToPathRendering().

Hm, I thought Dirk mentioned more corner cases, you're not handling them here?

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:86
> +    if (isFillFallback() &&
!style()->svgStyle()->strokeDashArray().isEmpty())

Why that extra condition? If the strokeDashArray is empty and isFillFallback is
true, shouldn't RenderSVGshape be asked for its stroke bbox??

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

s/this->//

> Source/WebCore/rendering/svg/RenderSVGRect.h:46
> +    virtual bool isEmpty() const { return hasPath() ?
RenderSVGShape::isEmpty() : m_boundingBox.isEmpty(); };

Here hasPath() is used not isFillFallback(), is that on purpose?
You should make this a multi-line function, and/or move in the cpp file.

> Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp:191
> +    if (/*path && */!(resourceMode & ApplyToTextMode)) {

You can remove the comment.

> Source/WebCore/rendering/svg/RenderSVGResourceSolidColor.cpp:83
> +    if (!(resourceMode & ApplyToTextMode)) {

We could add an early return here.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:139
> +	   if (!m_path.get())

The .get() is superfluous.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:140
> +	       RenderSVGShape::createShape();

This should be encapsulated in a descriptive named function, otherwhise we'll
wonder why only these conditions are checked.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:141
> +	   return RenderSVGShape::shapeDependentStrokeContains(point);

You want to avoid calling the shapeDependent function from the derived class,
thus that RenderSVGShape:: prefix here?
This is easy to misread.
The whole stuff should be commented more, hard to follow at the moment.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:156
> +	       filter->postApplyResource(static_cast<RenderSVGShape*>(object),
paintInfo.context, ApplyToDefaultMode, 0);

That's not a good idea, to blindly cast here!
This is also used for eg. RenderSVGContainer or Text who call
finish/preparerenderSVGContent.
How is this supposed to work?