Bug 233863

Created attachment 446024 [details] Patch, v1

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?

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

Created attachment 446035 [details] Patch, v2

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.

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

Created attachment 446152 [details] Patch, v3

Created attachment 446162 [details] Patch, v4

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.

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

Created attachment 446165 [details] Patch, v5

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.

(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 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 :)

Thanks Rob, great comment ;-)

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

<rdar://problem/86315362>