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

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


Maybe Niko has more comments to RenderTreeAsText. r- because of missing changes
of kling and some more comments. The code looks great in general.

> Source/WebCore/ChangeLog:58
> +	   (WebCore::RenderSVGRect::shapeDependentStrokeContains):
> +	   (WebCore::RenderSVGRect::shapeDependentFillContains):

Niko is right. A line by line comment what the new functions do is not a bad
idea.

> Source/WebCore/platform/graphics/FloatRect.cpp:76
> +bool FloatRect::isInsideRect(const FloatPoint& point) const
> +{
> +    if (!contains(point))
> +	   return false;
> +    return x() < point.x() && maxX() > point.x() && y() < point.y() &&
maxY() > y();
> +}

Niko mentioned to add a new argument to contains(const FloatPoint& point, Enum
...). The default value could be the normal contains. (Even if I'm not sure if
that is really needed.)

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

Please add a new line before comments. Makes it easier to read the code. Please
consider to write 
if (rect->hasAttribute(SVGNames::rxAttr) ||
rect->hasAttribute(SVGNames::ryAttr)) instead like mentioned by kling.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:68
> +    m_boundingBox = FloatRect(FloatPoint(rect->x().value(rect),
rect->y().value(rect)), boundingBoxSize);
> +    // To decide if the stroke contains a point we create two rects which
represent the inner and

ditto.

> Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:273
> +	       else if (shape)
> +		   shape->isPaintingFallback() ?
context->fillPath(shape->path()) : shape->fillShape(context);
> +	   } else if (resourceMode & ApplyToStrokeMode) {
> +	       if (path)
> +		   context->strokePath(*path);
> +	       else if (shape)
> +		   shape->isPaintingFallback() ?
context->strokePath(shape->path()) : shape->strokeShape(context);

The code style rules mention not to use 'else if' is it needed here? can we
just use if here?

> Source/WebCore/rendering/svg/RenderSVGResourcePattern.cpp:200
> +	   if (path)
>	       context->strokePath(*path);
> +	   else if (shape)
> +	       shape->isPaintingFallback() ? context->strokePath(shape->path())
: shape->strokeShape(context);

Ditto.

> Source/WebCore/rendering/svg/RenderSVGResourceSolidColor.cpp:92
> +    if (resourceMode & ApplyToFillMode) {
> +	   if (path)
>	       context->fillPath(*path);
> -	   else if (resourceMode & ApplyToStrokeMode)
> +	   else if (shape)
> +	       shape->isPaintingFallback() ? context->fillPath(shape->path()) :
shape->fillShape(context);
> +    } else if (resourceMode & ApplyToStrokeMode) {
> +	   if (path)
>	       context->strokePath(*path);
> +	   else if (shape)
> +	       shape->isPaintingFallback() ? context->strokePath(shape->path())
: shape->strokeShape(context);

Ditto.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:175
> +	   setisPaintingFallback(false);

setIsPaintingFallback, is with big I.

> Source/WebCore/rendering/svg/RenderSVGShape.h:80
> +	   ASSERT(m_path.get());

Can the .get() removed here?