Bug 233863 - [LBSE] Begin layer-aware RenderSVGRoot implementation
Summary: [LBSE] Begin layer-aware RenderSVGRoot implementation
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: 233666 233870 233871
Blocks: 90738 233872 233873
  Show dependency treegraph
 
Reported: 2021-12-05 16:08 PST by Nikolas Zimmermann
Modified: 2022-01-05 04:41 PST (History)
32 users (show)

See Also:


Attachments
Patch, v1 (93.46 KB, patch)
2021-12-06 04:13 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v2 (46.29 KB, patch)
2021-12-06 06:22 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v3 (45.07 KB, patch)
2021-12-07 04:12 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v4 (46.46 KB, patch)
2021-12-07 04:47 PST, Nikolas Zimmermann
no flags Details | Formatted Diff | Diff
Patch, v5 (46.44 KB, patch)
2021-12-07 06:06 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 2021-12-05 16:08:23 PST
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.
Comment 1 Nikolas Zimmermann 2021-12-06 04:13:03 PST
Created attachment 446024 [details]
Patch, v1
Comment 2 Rob Buis 2021-12-06 04:31:01 PST
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?
Comment 3 Nikolas Zimmermann 2021-12-06 05:59:41 PST
(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.
Comment 4 Nikolas Zimmermann 2021-12-06 06:22:44 PST
Created attachment 446035 [details]
Patch, v2
Comment 5 Rob Buis 2021-12-06 12:23:00 PST
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.
Comment 6 Nikolas Zimmermann 2021-12-07 03:50:44 PST
(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.
Comment 7 Nikolas Zimmermann 2021-12-07 04:12:33 PST
Created attachment 446152 [details]
Patch, v3
Comment 8 Nikolas Zimmermann 2021-12-07 04:47:22 PST
Created attachment 446162 [details]
Patch, v4
Comment 9 Rob Buis 2021-12-07 06:00:31 PST
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.
Comment 10 Nikolas Zimmermann 2021-12-07 06:05:38 PST
(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!
Comment 11 Nikolas Zimmermann 2021-12-07 06:06:02 PST
Created attachment 446165 [details]
Patch, v5
Comment 12 Rob Buis 2021-12-10 02:57:40 PST
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.
Comment 13 Nikolas Zimmermann 2021-12-10 03:16:08 PST
(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 <rbuis@igalia.com>
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 ;-)
Comment 14 Rob Buis 2021-12-10 03:21:37 PST
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 <rbuis@igalia.com>
> 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 :)
Comment 15 Nikolas Zimmermann 2021-12-10 03:25:10 PST
Thanks Rob, great comment ;-)
Comment 16 Nikolas Zimmermann 2021-12-10 03:25:14 PST
Committed r286842 (245075@trunk): <https://commits.webkit.org/245075@trunk>
Comment 17 Radar WebKit Bug Importer 2021-12-10 03:26:19 PST
<rdar://problem/86315362>