[webkit-reviews] review denied: [Bug 81104] REGRESSION: getBBox returns incorrect results with empty containers : [Attachment 133293] First pass

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Philip Rogers
<pdr at google.com>'s request for review:
Bug 81104: REGRESSION: getBBox returns incorrect results with empty containers
https://bugs.webkit.org/show_bug.cgi?id=81104

Attachment 133293: First pass
https://bugs.webkit.org/attachment.cgi?id=133293&action=review

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


Patch looks good, still some issues to consider though:

> Source/WebCore/platform/graphics/FloatRect.h:99
> +    // We define an invalid FloatRect as having negative width or height.

This is heavy, what I have in mind, is passing around a reference to an
internal static FloatRect, like:
static const FloatRect& invalidFloatRect()
{
    static const float kInvalidFloat = std::numeric_limits<float>::max() - 1;
    DEFINE_STATIC_LOCAL(FloatRect, invalidRect, (kInvalidFloat, kInvalidFloat,
kInvalidFloat, kInvalidFloat));
    return invalidRect;
}

FloatRect defaultBoundingBoxForContainer = invalidFloatRect();
You could make invalidFloatRect() a free-function, no need for a class-static,
but if you prefer it, you can keep it there.
You can also save the isValid() method completely, and use: someRect ==
invalidRect() to compare.

IMHO we should also use special values like flt_max-1, to distinguish from
negative values for the size, even if that could be used for that. To avoid
clashes, or problems like Dirk mentioned in his review, its safer to not reuse
negative values for the sake of determining the validity of a FloatRect.

> Source/WebCore/rendering/svg/RenderSVGContainer.cpp:153
>      m_strokeBoundingBox = FloatRect();

The stroke bounding box should suffer from the same problem no?
I could imagine it might make sense to keep the repaint bbox as-is, but I might
be wrong (didn't investigate)

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:105
> +		       isObjectBoundingBoxValid = false;

= true, no? :-) That should explain possible pixel test failures..

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:118
> +		       isObjectBoundingBoxValid = false;

Ditto.

I'm aware this is just a first-stab, but code like this should really be
shared, in a static inline helper method.