Bug 234632

<rdar://problem/87001334>

Created attachment 448643 [details] Patch, v1

Created attachment 448644 [details] Patch, v1

Comment on attachment 448644 [details] Patch, v1 This patch depends on 234954, postponing r? / EWS until it's resolved.

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.

(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.

Created attachment 448727 [details] Patch, v2

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

Created attachment 448736 [details] Patch, v3

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.

(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.

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

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 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.

(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.