Bug 90655 – Refactor RenderSVGShape bounding box code

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Philip Rogers]

RenderSVGShape::StrokeBoundingBox works differently than RenderSVGShape::ObjectBoundingBox by returning a cached value (m_strokeAndMarkerBoundingBox) instead of computing the current value. For understandability this should probably be refactored so that they work similarly.

Furthermore, a goal of https://bugs.webkit.org/show_bug.cgi?id=88231 is to move the marker and zero-length subpath code that is specific to RenderSVGPath out of RenderSVGShape. This refactoring will aid in that goal by allowing a subclass (RenderSVGPath) to simply override StrokeBoundingBox() and include the marker/zero-length-subpath code there.

Patch forthcoming.

[Philip Rogers]

Created attachment 151017 [details]
Refactor RenderSVGShape::strokeBoundingBox

[Philip Rogers]

While this patch is only a little exciting on its own, check out what it enables: https://bugs.webkit.org/show_bug.cgi?id=90716 :)
Similarly, we will be able to extract the marker code out of RenderSVGShape and into RenderSVGPath.

[Nikolas Zimmermann]

Comment on attachment 151017 [details]
Refactor RenderSVGShape::strokeBoundingBox

In theory that makes things like filters slower that query the strokeBoundingBox(). Previously the cached values was returned, now you're recomputing every time.
Did you consider this?

[Philip Rogers]

Created attachment 151812 [details]
Update per reviewer comments

[Philip Rogers]

(In reply to comment #3)
> (From update of attachment 151017 [details])
> In theory that makes things like filters slower that query the strokeBoundingBox(). Previously the cached values was returned, now you're recomputing every time.
> Did you consider this?

I was completely missing the bigger picture here... thanks for flagging this!

Turns out the other instances of objectBoundingBox return cached values (e.g., RenderSVGRoot.h, RenderSVGImage.h). In this updated patch I've done the exact opposite from how I started... converting objectBoundingBox to return cached values.

I also took a hint from your large refactor and reused the bounding box variables from RenderSVGShape in RenderSVGRect and RenderSVGEllipse... saving those extra variables hanging off each instance.

[Nikolas Zimmermann]

Comment on attachment 151812 [details]
Update per reviewer comments

Great! I've chosen a similar approach in my svg2 branch, except that strokeBoundingBox() is fully gone for me. r=me!

[WebKit Review Bot]

Comment on attachment 151812 [details]
Update per reviewer comments

Clearing flags on attachment: 151812

Committed r122424: <http://trac.webkit.org/changeset/122424>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dirk Schulze]

(In reply to comment #6)
> (From update of attachment 151812 [details])
> Great! I've chosen a similar approach in my svg2 branch, except that strokeBoundingBox() is fully gone for me. r=me!

You don't need to remove it completely, since it will be part of SVG2 again. getStrokeBBox() will be a new method [1]. It just will get a new task.

[1] https://svgwg.org/svg2-draft/types.html#__svg__SVGLocatable__getStrokeBBox