Bug 185806

Created attachment 382013 [details] Patch

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.

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?