Bug 234632 – [LBSE] Introduce SVGBoundingBoxComputation

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

[Nikolas Zimmermann]

Introduce 'SVGBoundingBoxComputation' a standalone class that implements the SVG2 object bounding box computation algorithm.
This is currently not Web-exposed, but very easy now to expose the algorithm to the Web (e.g. in getBbox() - which takes the new data structure as argument) -- this would improve SVG2 conformance.

[Radar WebKit Bug Importer]

<rdar://problem/87001334>

[Nikolas Zimmermann]

Created attachment 448643 [details]
Patch, v1

[Nikolas Zimmermann]

Created attachment 448644 [details]
Patch, v1

[Nikolas Zimmermann]

Comment on attachment 448644 [details]
Patch, v1

This patch depends on 234954, postponing r? / EWS until it's resolved.

[Rob Buis]

Comment on attachment 448644 [details]
Patch, v1

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

> Source/WebCore/rendering/svg/SVGBoundingBoxComputation.cpp:45
> +SVGBoundingBoxComputation::~SVGBoundingBoxComputation()

consider = default.

> Source/WebCore/rendering/svg/SVGBoundingBoxComputation.cpp:237
> +    //    defined by the âxâ, âyâ, âwidthâ and âheightâ geometric properties of the element.

Unicode problem?

> Source/WebCore/rendering/svg/SVGBoundingBoxComputation.h:24
> +#include "RenderLayerModelObject.h"

Is forward reference enough here?

> Source/WebCore/rendering/svg/SVGBoundingBoxComputation.h:46
> +        OverrideBoxWithFilterBoxForChildren = 1 << 7  /* WebKit extension - interal     */

Some internal typos here.

[Nikolas Zimmermann]

(In reply to Rob Buis from comment #5)
> Comment on attachment 448644 [details]
> Patch, v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448644&action=review
> 
> > Source/WebCore/rendering/svg/SVGBoundingBoxComputation.cpp:45
> > +SVGBoundingBoxComputation::~SVGBoundingBoxComputation()
> 
> consider = default.
Done in the header.

> > Source/WebCore/rendering/svg/SVGBoundingBoxComputation.cpp:237
> > +    //    defined by the âxâ, âyâ, âwidthâ and âheightâ geometric properties of the element.
> 
> Unicode problem?
Looks fine with a UTF-8 capable viewer :-) Indeed copy and paste from SVG spec -- I'll get rid of the non-ASCII quotes to avoid that.

> 
> > Source/WebCore/rendering/svg/SVGBoundingBoxComputation.h:24
> > +#include "RenderLayerModelObject.h"
> 
> Is forward reference enough here?
We store the RenderLayerModelObject as const reference member -- so we need to know the class size at compile time here -- therefore an include is necessary.

> 
> > Source/WebCore/rendering/svg/SVGBoundingBoxComputation.h:46
> > +        OverrideBoxWithFilterBoxForChildren = 1 << 7  /* WebKit extension - interal     */
> 
> Some internal typos here.
Fixed, apparently a common typo in WebKit:
WebKitVanilla % git grep interal | wc -l
      15

Will upload a new version, so we can hear EWS opinion about the patch.

[Nikolas Zimmermann]

Created attachment 448727 [details]
Patch, v2

[Nikolas Zimmermann]

Oops, this cannot compile yet: it depends on 234992.

[Nikolas Zimmermann]

Created attachment 448736 [details]
Patch, v3

[Rob Buis]

Comment on attachment 448736 [details]
Patch, v3

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

> Source/WebCore/rendering/svg/SVGBoundingBoxComputation.cpp:80
> +#endif

Likely not needed?

> Source/WebCore/rendering/svg/SVGBoundingBoxComputation.cpp:132
> +    auto transformationMatrixFromChild = [](const RenderLayerModelObject& child) -> std::optional<TransformationMatrix> {

Much more common to write lambda's like "[] (...".

> Source/WebCore/rendering/svg/SVGBoundingBoxComputation.cpp:153
> +            return;

Cool logic ;)

> Source/WebCore/rendering/svg/SVGBoundingBoxComputation.cpp:221
> +        box.intersect(FloatRect(overflowClipRect));

The explicit conversion is likely not needed.

[Nikolas Zimmermann]

(In reply to Rob Buis from comment #10)
> Comment on attachment 448736 [details]
> Patch, v3
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448736&action=review
> 
> > Source/WebCore/rendering/svg/SVGBoundingBoxComputation.cpp:80
> > +#endif
> 
> Likely not needed?
True, debugging leftover.

> 
> > Source/WebCore/rendering/svg/SVGBoundingBoxComputation.cpp:132
> > +    auto transformationMatrixFromChild = [](const RenderLayerModelObject& child) -> std::optional<TransformationMatrix> {
> 
> Much more common to write lambda's like "[] (...".
Ok.

> 
> > Source/WebCore/rendering/svg/SVGBoundingBoxComputation.cpp:153
> > +            return;
> 
> Cool logic ;)
? :-)

> 
> > Source/WebCore/rendering/svg/SVGBoundingBoxComputation.cpp:221
> > +        box.intersect(FloatRect(overflowClipRect));
> 
> The explicit conversion is likely not needed.
You're right, I've adapted the code.

Thanks for the reviews Rob, it's highly appreciated.

[Nikolas Zimmermann]

Committed r287873 (245917@trunk): <https://commits.webkit.org/245917@trunk>

[Darin Adler]

Comment on attachment 448736 [details]
Patch, v3

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

>>> Source/WebCore/rendering/svg/SVGBoundingBoxComputation.cpp:132
>>> +    auto transformationMatrixFromChild = [](const RenderLayerModelObject& child) -> std::optional<TransformationMatrix> {
>> 
>> Much more common to write lambda's like "[] (...".
> 
> Ok.

I thought same as you, Rob, but Yusuke told me I was wrong about this, and we had agreed on "[](..."; I keep forgetting to do that. I am annoyed we have both styles!

[Rob Buis]

Comment on attachment 448736 [details]
Patch, v3

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

>>>> Source/WebCore/rendering/svg/SVGBoundingBoxComputation.cpp:132
>>>> +    auto transformationMatrixFromChild = [](const RenderLayerModelObject& child) -> std::optional<TransformationMatrix> {
>>> 
>>> Much more common to write lambda's like "[] (...".
>> 
>> Ok.
> 
> I thought same as you, Rob, but Yusuke told me I was wrong about this, and we had agreed on "[](..."; I keep forgetting to do that. I am annoyed we have both styles!

Right, I did a quick grep, and there seemed to be more of the '[] (foo" kind, so that is what I suggested. Maybe we can have a (style) rule for this.

[Nikolas Zimmermann]

(In reply to Darin Adler from comment #13)
> >> 
> >> Much more common to write lambda's like "[] (...".
> > 
> > Ok.
> 
> I thought same as you, Rob, but Yusuke told me I was wrong about this, and
> we had agreed on "[](..."; I keep forgetting to do that. I am annoyed we
> have both styles!

Thanks for the comment - I'll move back to "[] (...)", if I touch the file next time. I agree with Rob, that a style rule would be helpful to ensure future consistency.