Summary: | [LBSE] Introduce SVGBoundingBoxComputation | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nikolas Zimmermann <zimmermann> | ||||||||||
Component: | SVG | Assignee: | 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
Nikolas Zimmermann
2021-12-23 02:21:56 PST
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. |