Bug 233863 – [LBSE] Begin layer-aware RenderSVGRoot implementation

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

[Nikolas Zimmermann]

Following the move RenderSVGRoot -> LegacyRenderSVGRoot (bug #233666), a new RenderSVGRoot implementation, tied to ENABLE(LAYER_BASED_SVG_ENGINE) is needed. Instead of starting totally from scratch, I've decided to import the RenderSVGRoot implementation from the LBSE downstream repository, and stub out parts that can and should be upstreamed / reviewed independently.

Many parts of the "new" RenderSVGRoot are still 1:1 copies of the existing code in LegacyRenderSVGRoot.

[Nikolas Zimmermann]

Created attachment 446024 [details]
Patch, v1

[Rob Buis]

Comment on attachment 446024 [details]
Patch, v1

View in context: https://bugs.webkit.org/attachment.cgi?id=446024&action=review

I think the patch is too big to review. I think you can split it more:
- introduce isSVGRootOrLegacySVGRoot and ajust call sites
- add LAYER_BASED_SVG_ENGINE/runtime stuff
- Introduce RenderSVGRoot
or something like that.

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:955
> +        if (auto svgRoot = ancestorsOfType<RenderSVGRoot>(*m_renderer).first()) {

It now seems you are checking the same condition twice?

[Nikolas Zimmermann]

(In reply to Rob Buis from comment #2)
> Comment on attachment 446024 [details]
> Patch, v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=446024&action=review
> 
> I think the patch is too big to review. I think you can split it more:
Absolutely good idea, will do.

> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:955
> > +        if (auto svgRoot = ancestorsOfType<RenderSVGRoot>(*m_renderer).first()) {
> 
> It now seems you are checking the same condition twice?

Indeed, a typo.

[Nikolas Zimmermann]

Created attachment 446035 [details]
Patch, v2

[Rob Buis]

Comment on attachment 446035 [details]
Patch, v2

View in context: https://bugs.webkit.org/attachment.cgi?id=446035&action=review

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:185
> +#endif

I wonder if this can be introduced when/just after introducing SVGLogger.

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:294
> +#endif

Ditto.

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:393
> +    newPhase = (newPhase == PaintPhase::ChildBlockBackgrounds) ? PaintPhase::ChildBlockBackground : newPhase;

Probably better as if statement. Why is this phase changing needed? May need a comment?

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:469
> +    if (zoom != 1)

I guess a float notation may be nicer here.

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:543
> +        if (zoom != 1)

Ditto.

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:650
> +        if (zoom != 1) {

Ditto.

[Nikolas Zimmermann]

(In reply to Rob Buis from comment #5)
> Comment on attachment 446035 [details]
> Patch, v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=446035&action=review
> 
> > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:185
> > +#endif
> 
> I wonder if this can be introduced when/just after introducing SVGLogger.
Agreed, much better to actually also show the output in the ChangeLog, makes it easier to judge about the logger code.

I used this extensively during development / verification of the two-pass layout approach - it's a SVG-focused view of the render tree, showing SVG specific information (objectBoundingBox, ... etc), CSS view of the SVG renderer (frameRectEquivalent/visualOverflowRectEquivalent/...), and dumps the whole tree in ASCII art with "boxes" enclosing renderers.

> 
> > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:294
> > +#endif
> 
> Ditto.
Ack.

> 
> > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:393
> > +    newPhase = (newPhase == PaintPhase::ChildBlockBackgrounds) ? PaintPhase::ChildBlockBackground : newPhase;
> 
> Probably better as if statement. Why is this phase changing needed? May need
> a comment?
That's a 1:1 copy from RenderBlock::paintContents(), for the non-inline children case. In principle one could refactor both RenderBlock::paintContents() and RenderSVGRoot::paintContents() into e.g. a protected RenderBox::paintBlockContents() helper. However it doesn't really belong into RenderBox, just happens to be the common ancestor for RenderSVGRoot - thus RenderReplaced - and RenderBlock.

Regarding the question, why is this phase changing needed?

May 6, 2003, Dave Hyatt (2da7013f33ed6):
+    // We don't paint our own background, but we do let the kids paint their backgrounds.
+    if (paintAction == PaintActionChildBackgrounds)
+        paintAction = PaintActionChildBackground;

His comment got list in time.

> > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:469
> > +    if (zoom != 1)
>
> I guess a float notation may be nicer here.
I thought we avoid that in the CSS places that deal with this, I'll double check.

> 
> > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:543
> > +        if (zoom != 1)
> 
> Ditto.
> 
> > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:650
> > +        if (zoom != 1) {
> 
> Ditto.

[Nikolas Zimmermann]

Created attachment 446152 [details]
Patch, v3

[Nikolas Zimmermann]

Created attachment 446162 [details]
Patch, v4

[Rob Buis]

Comment on attachment 446162 [details]
Patch, v4

View in context: https://bugs.webkit.org/attachment.cgi?id=446162&action=review

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:557
> +        // There can't be a transform between repaintContainer and o, because transforms create containers, so it should be safe

o is not very clear :)

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:582
> +            if (transformAboveSVGFragment) {

Can be combined to one if statement.

[Nikolas Zimmermann]

(In reply to Rob Buis from comment #9)
> Comment on attachment 446162 [details]
> Patch, v4
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=446162&action=review
> 
> > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:557
> > +        // There can't be a transform between repaintContainer and o, because transforms create containers, so it should be safe
> 
> o is not very clear :)
> 
> > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:582
> > +            if (transformAboveSVGFragment) {
> 
> Can be combined to one if statement.

Fixed, thanks for the review!

[Nikolas Zimmermann]

Created attachment 446165 [details]
Patch, v5

[Rob Buis]

Comment on attachment 446165 [details]
Patch, v5

View in context: https://bugs.webkit.org/attachment.cgi?id=446165&action=review

> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:246
> +    return effectiveOverflowX() == Overflow::Hidden

The effectiveOverflowX makes only sense when compared with visible IIRC. I guess the question is what to do here for overflow:clip or it being enabled due to contain: paint.

[Nikolas Zimmermann]

(In reply to Rob Buis from comment #12)
> Comment on attachment 446165 [details]
> Patch, v5
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=446165&action=review
> 
> > Source/WebCore/rendering/svg/RenderSVGRoot.cpp:246
> > +    return effectiveOverflowX() == Overflow::Hidden
> 
> The effectiveOverflowX makes only sense when compared with visible IIRC. I
> guess the question is what to do here for overflow:clip or it being enabled
> due to contain: paint.

I followed your change in LegacyRenderSVGRoot from Nov 16.

5ed3c610d08e7 Source/WebCore/rendering/svg/RenderSVGRoot.cpp       (Rob Buis              2021-11-16 22:32:39 +0000 218)     return effectiveOverflowX() == Overflow::Hidden

commit 5ed3c610d08e786fec6b85e6137676ce89002aed
Author: Rob Buis <[e-mail]>
Date:   Tue Nov 16 22:32:39 2021 +0000

    [css-contain] Support contain:paint
    https://bugs.webkit.org/show_bug.cgi?id=224742

There you specifically moved to check effectiveOverflowX() == Overflow::Hidden for (Legacy)RenderSVGRoot. You have to clarify if that's correct wrt to contain: paint ;-)

[Rob Buis]

Comment on attachment 446165 [details]
Patch, v5

View in context: https://bugs.webkit.org/attachment.cgi?id=446165&action=review

>>> Source/WebCore/rendering/svg/RenderSVGRoot.cpp:246
>>> +    return effectiveOverflowX() == Overflow::Hidden
>> 
>> The effectiveOverflowX makes only sense when compared with visible IIRC. I guess the question is what to do here for overflow:clip or it being enabled due to contain: paint.
> 
> I followed your change in LegacyRenderSVGRoot from Nov 16.
> 
> 5ed3c610d08e7 Source/WebCore/rendering/svg/RenderSVGRoot.cpp       (Rob Buis              2021-11-16 22:32:39 +0000 218)     return effectiveOverflowX() == Overflow::Hidden
> 
> commit 5ed3c610d08e786fec6b85e6137676ce89002aed
> Author: Rob Buis <[e-mail]>
> Date:   Tue Nov 16 22:32:39 2021 +0000
> 
>     [css-contain] Support contain:paint
>     https://bugs.webkit.org/show_bug.cgi?id=224742
> 
> There you specifically moved to check effectiveOverflowX() == Overflow::Hidden for (Legacy)RenderSVGRoot. You have to clarify if that's correct wrt to contain: paint ;-)

Ok, then it must be right :)

[Nikolas Zimmermann]

Thanks Rob, great comment ;-)

[Nikolas Zimmermann]

Committed r286842 (245075@trunk): <https://commits.webkit.org/245075@trunk>

[Radar WebKit Bug Importer]

<rdar://problem/86315362>