[webkit-reviews] review granted: [Bug 134184] Elements with rendering disabled due to dimensions should not contribute to parent bounding box : [Attachment 234473] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Nikos Andronikos
<nikos.andronikos-webkit at cisra.canon.com.au>'s request for review:
Bug 134184: Elements with rendering disabled due to dimensions should not
contribute to parent bounding box
https://bugs.webkit.org/show_bug.cgi?id=134184

Attachment 234473: Patch
https://bugs.webkit.org/attachment.cgi?id=234473&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=234473&action=review


I’m going to say review+ because I think this code change is helpful, gets us
closer to a correct implementation, but I think there are mistakes, and tests
that cover edge cases would likely show us this.

> Source/WebCore/rendering/svg/RenderSVGEllipse.cpp:158
> +    return m_fillBoundingBox.isEmpty();

Likely the same problem with m_usePathFallback that I mention below in
RenderSVGRect::isRenderingDisabled.

> Source/WebCore/rendering/svg/RenderSVGEllipse.h:39
> +    virtual bool isRenderingDisabled() const override;

Our usual practice is to make such overrides private since we only intend to
call the functions polymorphically. If we called this directly it would
probably be a programming mistake.

> Source/WebCore/rendering/svg/RenderSVGPath.cpp:166
> +    // For a polygon, polyline and path, rendering is disabled if the d
> +    // or points attribute is missing or empty.
> +    return path().isEmpty();

This comment doesn’t match this code. It’s not good to have that comment before
code that is entirely different. If we keep the comment it needs to explain why
the code below is correct.

I don’t fully understand why the path will only be empty in the case mentioned
above. It might be true, but if it’s true, it’s definitely not obvious.

> Source/WebCore/rendering/svg/RenderSVGPath.h:38
> +    virtual bool isRenderingDisabled() const override;

Can make this private for same reason as above.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:157
> +    // Some combinations of attributes cause SVG elements
> +    // to not render.

I don’t think this comment clarifies; it’s kind of an oblique remark.

To give one example of how it’s misleading, to me it makes it sound like this
function prevents an SVG element from rendering. But it doesn’t. This function
is only used to decide whether this shape contributes to its parent’s bounding
box, and does not directly affect rendering.

It’s also confusing because this function doesn’t check the values of
attributes on an SVG element. Instead it checks a computed box that depends not
only on values of attributes, but also on other things such as the length
context and even on the vector effect (non-scaling stroke).

Besides being non-clear, the comment does not belong here. If it was important
to have this comment, we would not want it just on one particular override of
isRenderingDisabled since it applies equally to all isRenderingDisabled
functions. Maybe we’d put it at the call site or in the
RenderSVGShape::isRenderingDisabled function.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:159
> +    // For rects a negative dimension is an error and
> +    // a zero dimension disables rendering.

This comment is a bit mysterious (does not explain how the error relates to the
rendering disabled issue). But also, if a comment like this was needed, we’d
want one in RenderSVGEllipse::isRenderingDisabled too. I suggest just omitting
the comment. If we keep it, then we should make the comment more pertinent,
explaining exactly why this is the correct code.

> Source/WebCore/rendering/svg/RenderSVGRect.cpp:160
> +    return m_fillBoundingBox.isEmpty();

I think this is not quite right. The RenderSVGRect::updateShapeFromElement
function has a different set of conditions that determine wither rendering is
enabled and this doesn’t seem to quite match them. In particular, I think a
correct version of this function needs to look at m_usePathFallback, not just
m_fillBoundingBox. When m_usePathFallback is true, it’s possible that
m_fillBoundingBox contains an old, now-invalid bounding box that was computed
earlier. Some kind of check that involves path() is likely needed in that case.


> Source/WebCore/rendering/svg/RenderSVGRect.h:43
> +    virtual bool isRenderingDisabled() const override;

Can make this private for same reason as above.

> Source/WebCore/rendering/svg/RenderSVGShape.h:73
> +    virtual bool isRenderingDisabled() const
> +    {
> +	   // Returning false by default ensures backwards compatibility for
> +	   // elements that don't implement this method.
> +	   return false;
> +    }

The base implementation should not be put here in the class definition. I
suggest putting it in the .cpp file, since it won’t be inlined since it’s going
to be a virtual function call.

The comment does not have enough context. Today, the term “backwards
compatibility” makes sense to us; the compatibility is with how the code was
before this patch. But in the future it won’t make sense because nothing will
indicate that at one time this function didn’t exist. Instead the right way to
explain this is that any class that does not override this function will never
disable rendering.