Bug 235101 - [LBSE] Introduce SVGContainerLayout
Summary: [LBSE] Introduce SVGContainerLayout
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nikolas Zimmermann
URL:
Keywords: InRadar
Depends on: 235099 235100
Blocks: 90738
  Show dependency treegraph
 
Reported: 2022-01-12 02:26 PST by Nikolas Zimmermann
Modified: 2022-01-14 01:44 PST (History)
15 users (show)

See Also:


Attachments
Patch, v1 (25.74 KB, patch)
2022-01-12 05:14 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v2 (25.77 KB, patch)
2022-01-14 00:41 PST, Nikolas Zimmermann
rbuis: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikolas Zimmermann 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.
Comment 1 Nikolas Zimmermann 2022-01-12 05:14:13 PST
Created attachment 448935 [details]
Patch, v1
Comment 2 Rob Buis 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?
Comment 3 Nikolas Zimmermann 2022-01-14 00:41:29 PST
Created attachment 449144 [details]
Patch, v2
Comment 4 Nikolas Zimmermann 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.
Comment 5 Rob Buis 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!
Comment 6 Nikolas Zimmermann 2022-01-14 01:43:17 PST
Committed r288011 (246037@trunk): <https://commits.webkit.org/246037@trunk>
Comment 7 Radar WebKit Bug Importer 2022-01-14 01:44:18 PST
<rdar://problem/87592353>