Bug 234632

Summary: [LBSE] Introduce SVGBoundingBoxComputation
Product: WebKit Reporter: Nikolas Zimmermann <zimmermann>
Component: SVGAssignee: Nikolas Zimmermann <zimmermann>
Status: RESOLVED FIXED    
Severity: Normal CC: changseok, darin, dino, esprehn+autocc, ews-watchlist, fmalita, glenn, gyuyoung.kim, kondapallykalyan, pdr, rbuis, sabouhallawa, schenney, sergio, webkit-bug-importer, zimmermann
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 234524, 234954, 234992    
Bug Blocks: 90738    
Attachments:
Description Flags
Patch, v1
none
Patch, v1
none
Patch, v2
none
Patch, v3 rbuis: review+

Description Nikolas Zimmermann 2021-12-23 02:21:56 PST
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.
Comment 1 Radar WebKit Bug Importer 2021-12-30 02:22:15 PST
<rdar://problem/87001334>
Comment 2 Nikolas Zimmermann 2022-01-07 16:09:12 PST
Created attachment 448643 [details]
Patch, v1
Comment 3 Nikolas Zimmermann 2022-01-07 16:12:55 PST
Created attachment 448644 [details]
Patch, v1
Comment 4 Nikolas Zimmermann 2022-01-07 16:17:19 PST
Comment on attachment 448644 [details]
Patch, v1

This patch depends on 234954, postponing r? / EWS until it's resolved.
Comment 5 Rob Buis 2022-01-10 01:44:42 PST
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.
Comment 6 Nikolas Zimmermann 2022-01-10 01:54:30 PST
(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.
Comment 7 Nikolas Zimmermann 2022-01-10 01:58:46 PST
Created attachment 448727 [details]
Patch, v2
Comment 8 Nikolas Zimmermann 2022-01-10 02:33:25 PST
Oops, this cannot compile yet: it depends on 234992.
Comment 9 Nikolas Zimmermann 2022-01-10 05:16:02 PST
Created attachment 448736 [details]
Patch, v3
Comment 10 Rob Buis 2022-01-10 06:38:33 PST
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.
Comment 11 Nikolas Zimmermann 2022-01-11 04:15:39 PST
(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.
Comment 12 Nikolas Zimmermann 2022-01-11 04:57:59 PST
Committed r287873 (245917@trunk): <https://commits.webkit.org/245917@trunk>
Comment 13 Darin Adler 2022-01-11 13:03:28 PST
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!
Comment 14 Rob Buis 2022-01-11 13:27:09 PST
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.
Comment 15 Nikolas Zimmermann 2022-01-12 01:12:16 PST
(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.