Bug 235101 – [LBSE] Introduce SVGContainerLayout

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Nikolas Zimmermann]

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.

[Nikolas Zimmermann]

Created attachment 448935 [details]
Patch, v1

[Rob Buis]

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]

Created attachment 449144 [details]
Patch, v2

[Nikolas Zimmermann]

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

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

Committed r288011 (246037@trunk): <https://commits.webkit.org/246037@trunk>

[Radar WebKit Bug Importer]

<rdar://problem/87592353>