Bug 235101

Summary: [LBSE] Introduce SVGContainerLayout
Product: WebKit Reporter: Nikolas Zimmermann <zimmermann>
Component: SVGAssignee: Nikolas Zimmermann <zimmermann>
Status: RESOLVED FIXED    
Severity: Normal CC: changseok, 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: 235099, 235100    
Bug Blocks: 90738    
Attachments:
Description Flags
Patch, v1
none
Patch, v2 rbuis: review+

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>