Bug 185806 – SVG2 Implement getBBox dictionary argument

• Status: NEW
• Component: SVG

[Dirk Schulze]

getBBox takes an dictionary argument in SVG2 to describe which box the user requests: https://svgwg.org/svg2-draft/types.html#__svg__SVGGraphicsElement__getBBox

[Dirk Schulze]

Created attachment 382013 [details]
Patch

[Dirk Schulze]

Turns out that getBBox seems to be broken for foreignObject. I am not taking that on with the patch above but will create a new bug report.

[Nikolas Zimmermann]

Comment on attachment 382013 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=382013&action=review

Nice patch, I like this particular new SVG2 feature! Here's a first unofficial review:

> Source/WebCore/ChangeLog:21
> +                need the heuristics here to be as compatible with other implementations

other impls? Can you elaborate on this?

> Source/WebCore/ChangeLog:22
> +                as possible. (Algorithm is specified by CSS Masking)

Concrete link would be helpful.

> Source/WebCore/rendering/RenderObject.cpp:1805
> +FloatRect RenderObject::strokeBoundingBoxDOM() const

This name is confusing and not self-explanatory.

> Source/WebCore/rendering/RenderObject.cpp:1811
> +FloatRect RenderObject::getBBox(const SVGBoundingBoxFlags) const

Confusion++ - we should try hard to avoid adding new virtual functions to RenderObject.
The naming follows the SVG2 spec but we should avoid that, getBBox is a bad name.

I would prefer to see one function that unifies computing all the different bounding boxes that are needed for SVG...
FloatRect RenderObject::computeSVGBoundingBox(SVGBoundingBoxType)

> Source/WebCore/rendering/RenderObject.h:369
> +    // SVG 2 introduced one function to compute 

to compute what? The name sounds confusing

> Source/WebCore/rendering/RenderObjectEnums.h:78
> +typedef unsigned SVGBoundingBoxFlags;

This is using a legacy way to define flags. enum class + OptionSet seems the more modern choice, no?

> Source/WebCore/rendering/svg/RenderSVGContainer.cpp:154
> +    SVGRenderSupport::computeContainerBoundingBoxes(*this, m_objectBoundingBox, m_objectBoundingBoxValid, m_strokeBoundingBox, m_strokeBoundingBoxDOM, m_repaintBoundingBox);

This function should not be extended with another argument, but instead a helper struct/class holding the various bounding boxes could be introduced.

> Source/WebCore/rendering/svg/RenderSVGShape.cpp:436
> +    // https://drafts.fxtf.org/css-masking-1/#compute-stroke-bounding-box

Aha, here is the spec link I asked for in the ChangeLog :-)

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:177
> +    const float sqrt2 = std::sqrt(2);

constexpr would make more sense, to avoid runtime computations (probably the compiler is clever enough in any way, but I did not verify this -- better use constexpr where it makes sense).

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:183
> +            inflateDelta *= inflateDelta * inflateDelta;

std::pow(inflateDelta, 2) is superior to x*x on some platforms (but only the variant that uses integer exponents, std::pow(x, 2.0) is slower).

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:188
> +FloatRect SVGRenderSupport::getBBox(const RenderElement& element, const SVGBoundingBoxFlags options)

I do not see the need for the const.
But I'd advice to avoid using "unsigned" as flag type, but use a type-safe way... OptionSet.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:196
> +        return box;

return {}, move FloatRect box below.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:199
> +        if (!is<RenderElement>(child))

Why branch here? You can ask for childrenOfType<RenderElement>.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:201
> +        box.unite(getBBox(downcast<RenderElement>(child), options));

What about empty child bounding boxes? Do we need to take special care here.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:205
> +    bool isNotAContainer = isShape || is<RenderSVGShape>(element) || is<RenderSVGImage>(element) || is<RenderSVGForeignObject>(element);

The second if condition is superfluous.
There are more cases that "are not a container" - the variable naming is confusing.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:213
> +        downcast<RenderSVGShape>(element).uniteRectWithMarkerRect(box);

This could be written in the same way as above, if we had proper accessors for the marker bounds.

> Source/WebCore/rendering/svg/SVGRenderSupport.cpp:360
> +    if (RenderSVGResourceClipper* clipper = resources->clipper())

s/RenderSVGResourceClipper/auto/ ?

> Source/WebCore/svg/SVGGraphicsElement.h:38
> +    struct BoundingBoxOptions {

This is the recommended way to handle "dict" options exposed to the bindings?