[webkit-reviews] review granted: [Bug 81104] REGRESSION: getBBox returns incorrect results with empty containers : [Attachment 133663] Asking reviewers what they think about a different approach

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has granted 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 133663: Asking reviewers what they think about a different approach
https://bugs.webkit.org/attachment.cgi?id=133663&action=review

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


That approach is much less intrusive, and I think its the best mid-term
solution. If we ever encounter non-SVG code suffering from similar problems, we
can always come back and introduce FloatRect::invalid().
I'll r+ this as-is, if someone dislikes this concept, feel free to r- with
reasons :-)

> Source/WebCore/ChangeLog:20
> +	   Reviewed by NOBODY (OOPS!).

This line should be on top.

> Source/WebCore/rendering/svg/RenderSVGContainer.h:45
> +    bool isObjectBoundingBoxValid() { return m_objectBoundingBoxValid; }

Make it const.

> Source/WebCore/rendering/svg/RenderSVGContainer.h:80
> +    bool m_objectBoundingBoxValid;

You could move this down and mark as : 1, as its done for
m_needsBoundariesUpdate, which in theory saves space.

> Source/WebCore/rendering/svg/RenderSVGRoot.h:107
> +    bool m_objectBoundingBoxValid;

This can also be moved down and marked as :1.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:88
> +// Update an object's bounding box taking into account the validity of each
object bounding box.

typo: objects

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:92
> +    if (otherValid) {

Early exit if !otherValid.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:94
> +	   if (objectBoundingBoxValid)
> +	       objectBoundingBox.uniteEvenIfEmpty(otherBoundingBox);

Ditto. Early exit instead of using an else branch.