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

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


It definitely looks a lot better and you're nearly done! But still some more
comments.

> LayoutTests/svg/custom/pointer-events-on-rounded-rect.xhtml:14
> +	stroke-dasharray: 50 15;

se later comment.

> LayoutTests/svg/custom/pointer-events-with-linecaps-and-miterlimits.xhtml:14
> +	stroke-dasharray: 20 50;

lion changed the behavior of dash array. Not sure if we can influence it in
GraphicsContextCG (just noticed it on running pixel tests). Maybe we should
leave out dash array and check just the corner (11, 11). Not that we have to
roll out the patch because lion comments about failures.

> Source/WebCore/ChangeLog:9
> +	   handle the fallback cases (e.g.: rounded rects, special strokes,
pointer events, etc.)

Missing dot at the end of the sentence :)

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:52
> +    if (style()->svgStyle()->hasMarkers()) {
>	   FloatRect markerBounds = calculateMarkerBoundsIfNeeded();
>	   if (!markerBounds.isEmpty())
> -	       m_strokeAndMarkerBoundingBox.unite(markerBounds);
> +	       strokeBoundingBox().unite(markerBounds);

Oh that looks wrong! That makes no sense. You get a FloatRect from
strokeBoundingBox() and make a union with markerBounds but neither return it
nor store it? What about m_strokeAndMarkerBoundingBox? Why not store the values
here?

Doesn't it fire on a test? maybe on dynamically removing the markers of a path?
Is so, I'd like to see another test.

> Source/WebCore/rendering/svg/RenderSVGPath.h:44
> +    void inflateWithMarkerBounds();

put a virtual in front of this.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:67
> +    float widthValue = rect->width().value(rect);
> +    if (widthValue <= 0)
> +	   return;
> +
> +    float heightValue = rect->height().value(rect);
> +    if (heightValue <= 0)
> +	   return;

What about:

FloatSize boundingBoxSize(rect->width().value(rect),
rect->height().value(rect));
if (boundingBoxSize.isEmpty())
    return;

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:71
> +    float xValue = rect->x().value(rect);
> +    float yValue = rect->y().value(rect);
> +    m_boundingBox = FloatRect(xValue, yValue, widthValue, heightValue);

m_boundingBox = FloatRect(FloatPoint(rect->x().value(rect),
rect->y().value(rect)), boundingBoxSize);

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:392
> +void RenderSVGShape::inflateWithMarkerBounds()
> +{
> +}

remove it here.

> Source/WebCore/rendering/svg/RenderSVGShape.h:112
> +    virtual void inflateWithMarkerBounds();

empty braces behind the function.