RESOLVED FIXED 235101
[LBSE] Introduce SVGContainerLayout
https://bugs.webkit.org/show_bug.cgi?id=235101
Summary [LBSE] Introduce SVGContainerLayout
Nikolas Zimmermann
Reported 2022-01-12 02:26:16 PST
SVGContainerLayout contains the implementation for the two-pass layout procedure, as described in the technical design document. The container-children-layout procedure was spread over SVGRenderSupport before and is now in a central place for LBSE.
Attachments
Patch, v1 (25.74 KB, patch)
2022-01-12 05:14 PST, Nikolas Zimmermann
no flags
Patch, v2 (25.77 KB, patch)
2022-01-14 00:41 PST, Nikolas Zimmermann
rbuis: review+
Nikolas Zimmermann
Comment 1 2022-01-12 05:14:13 PST
Created attachment 448935 [details] Patch, v1
Rob Buis
Comment 2 2022-01-13 01:46:11 PST
Comment on attachment 448935 [details] Patch, v1 View in context: https://bugs.webkit.org/attachment.cgi?id=448935&action=review > Source/WebCore/ChangeLog:17 > + Currently the functionality is not observable yet, as we don't create Probably remove the first 'yet'. > Source/WebCore/rendering/svg/SVGContainerLayout.cpp:99 > + bool childNeededLayout = child.needsLayout(); Is it possible that childNeededLayout != needsLayout here? > Source/WebCore/rendering/svg/SVGContainerLayout.h:44 > + static bool transformToRootChanged(const RenderObject* ancestor); Do these 2 need to be public? > Source/WebCore/rendering/svg/SVGContainerLayout.h:47 > + void layoutDifferentRootIfNeeded(const RenderElement&); Different from what?
Nikolas Zimmermann
Comment 3 2022-01-14 00:41:29 PST
Created attachment 449144 [details] Patch, v2
Nikolas Zimmermann
Comment 4 2022-01-14 00:41:34 PST
(In reply to Rob Buis from comment #2) > > Source/WebCore/ChangeLog:17 > > + Currently the functionality is not observable yet, as we don't create > > Probably remove the first 'yet'. Fixed. > > Source/WebCore/rendering/svg/SVGContainerLayout.cpp:99 > > + bool childNeededLayout = child.needsLayout(); > > Is it possible that childNeededLayout != needsLayout here? Yes, there are cases where e.g. a text renderer did not originally need layout, but we force a relayout, e.g. if transform to root changes (on-screen scaling factor for text rendering -- which is an ugly topic that we should gt rid of in the future). > > > Source/WebCore/rendering/svg/SVGContainerLayout.h:44 > > + static bool transformToRootChanged(const RenderObject* ancestor); > > Do these 2 need to be public? Yes, they are used in other places in LBSE branch. > > > Source/WebCore/rendering/svg/SVGContainerLayout.h:47 > > + void layoutDifferentRootIfNeeded(const RenderElement&); > > Different from what? Different from the layout root belonging to the passed RenderElement... Used for cases like referencing a SVG filter from a different document. I agree the naming is not wise, though it's using existing terminology from SVGResources, which we do want to re-visit in future. SVGResources are mostly obsolete in LBSE, after all clipper/masker/filter... applyResource are all no-ops in LBSE as opposed to the legacy engine, since clipping/masking/filtering is handled via RenderLayer in LBSE. Anyhow, I'd like to keep the existing terminology for now, in order not to open another construction site.
Rob Buis
Comment 5 2022-01-14 01:07:06 PST
(In reply to Nikolas Zimmermann from comment #4) > > Different from what? > Different from the layout root belonging to the passed RenderElement... > Used for cases like referencing a SVG filter from a different document. > > I agree the naming is not wise, though it's using existing terminology from > SVGResources, which we do want to re-visit in future. SVGResources are > mostly obsolete in LBSE, after all clipper/masker/filter... applyResource > are all no-ops in LBSE as opposed to the legacy engine, since > clipping/masking/filtering is handled via RenderLayer in LBSE. That is great to hear!
Nikolas Zimmermann
Comment 6 2022-01-14 01:43:17 PST
Radar WebKit Bug Importer
Comment 7 2022-01-14 01:44:18 PST
Note You need to log in before you can comment on or make changes to this bug.