Bug 234524

Created attachment 447652 [details] Patch, v1

Comment on attachment 447652 [details] Patch, v1 View in context: https://bugs.webkit.org/attachment.cgi?id=447652&action=review > Source/WebCore/ChangeLog:21 > + the reason for the unnecessary confusing SVG inheritance structure Furthermore we explicitly Forgot a period before "Furthermore". > Source/WebCore/Sources.txt:2483 > +rendering/svg/RenderSVGModelObject.cpp These additions break the sorted list. > Source/WebCore/rendering/RenderLayerModelObject.cpp:239 > + if (!is<RenderSVGModelObject>(*this)) Is the only time this can be hit when we deal with RenderLineBreak? If so then maybe it is nicer to add a computeVisibleRectInContainer to RenderLineBreak that skips straight to RenderObject:: computeVisibleRectInContainer and make this an ASSERT. > Source/WebCore/rendering/RenderLayerModelObject.cpp:281 > + if (!is<RenderSVGModelObject>(*this)) Ditto. > Source/WebCore/rendering/svg/RenderSVGModelObject.cpp:2 > + * Copyright (c) 2009, Google Inc. All rights reserved. Why Google and why 2009? Also if you added code (I assume you did) then maybe Igalia 2021 is in order. > Source/WebCore/rendering/svg/RenderSVGModelObject.h:2 > + * Copyright (c) 2009, Google Inc. All rights reserved. Ditto.

(In reply to Rob Buis from comment #2) Thanks for the review! > > Source/WebCore/ChangeLog:21 > > + the reason for the unnecessary confusing SVG inheritance structure Furthermore we explicitly > > Forgot a period before "Furthermore". Fixed. > > Source/WebCore/Sources.txt:2483 > > +rendering/svg/RenderSVGModelObject.cpp > > These additions break the sorted list. Oh, not checked by EWS apparently. Fixed. > > Source/WebCore/rendering/RenderLayerModelObject.cpp:239 > > + if (!is<RenderSVGModelObject>(*this)) > > Is the only time this can be hit when we deal with RenderLineBreak? If so > then maybe it is nicer to add a computeVisibleRectInContainer to > RenderLineBreak that skips straight to RenderObject:: > computeVisibleRectInContainer and make this an ASSERT. Hmm, what's the relation to RenderLineBreak? Cannot follow you here. In the full LBSE, only RenderSVGModelObject / RenderSVGBlock need a computeVisibleRectInContainer() implementation, and RenderLayerModelObject is the common base class for them. > > Source/WebCore/rendering/RenderLayerModelObject.cpp:281 > > + if (!is<RenderSVGModelObject>(*this)) > > Ditto. Same as above, not sure what you mean. > > > Source/WebCore/rendering/svg/RenderSVGModelObject.cpp:2 > > + * Copyright (c) 2009, Google Inc. All rights reserved. > > Why Google and why 2009? Also if you added code (I assume you did) then > maybe Igalia 2021 is in order. Well that's a straight copy from LegacyRenderSVGModelObject, and I have to preserve copyright. But you're right: Igalia is missing. > > > Source/WebCore/rendering/svg/RenderSVGModelObject.h:2 > > + * Copyright (c) 2009, Google Inc. All rights reserved. > > Ditto.

Created attachment 447745 [details] Patch, v2

(In reply to Rob Buis from comment #2) > Comment on attachment 447652 [details] > > Is the only time this can be hit when we deal with RenderLineBreak? If so > > then maybe it is nicer to add a computeVisibleRectInContainer to > > RenderLineBreak that skips straight to RenderObject:: > > computeVisibleRectInContainer and make this an ASSERT. > Hmm, what's the relation to RenderLineBreak? Cannot follow you here. > In the full LBSE, only RenderSVGModelObject / RenderSVGBlock need a > computeVisibleRectInContainer() implementation, and RenderLayerModelObject > is the common base class for them. computeVisibleRectInContainer is overridden in all subclasses except in RenderLineBreak. So by adding computeVisibleRectInContainer to RenderLineBreak, RenderLayerModelObject::computeVisibleRectInContainer should only by called by SVG subclasses, so the if should be able to be converted into an ASSERT.

(In reply to Rob Buis from comment #5) > (In reply to Rob Buis from comment #2) > > Comment on attachment 447652 [details] > > > Is the only time this can be hit when we deal with RenderLineBreak? If so > > > then maybe it is nicer to add a computeVisibleRectInContainer to > > > RenderLineBreak that skips straight to RenderObject:: > > > computeVisibleRectInContainer and make this an ASSERT. > > Hmm, what's the relation to RenderLineBreak? Cannot follow you here. > > In the full LBSE, only RenderSVGModelObject / RenderSVGBlock need a > > computeVisibleRectInContainer() implementation, and RenderLayerModelObject > > is the common base class for them. > > computeVisibleRectInContainer is overridden in all subclasses except in > RenderLineBreak. So by adding computeVisibleRectInContainer to > RenderLineBreak, > RenderLayerModelObject::computeVisibleRectInContainer should only by called > by SVG subclasses, so the if should be able to be converted into an ASSERT. That sounds fishy to me: a) I would not expect a function in RenderLayerModelObject to assert that it‘s called on a RenderSVGModelObject/RenderSVGBlock only. b) It hurting extensibility: a new HTML/CSS renderer class inheriting from RenderLayerModelObject has to implement mapLocalToContainer() and call the super-super class implementation (e.g. RenderLineBreak needs to skip RenderLayerModelObject::mapLocalToContainer()). The „short cut“ to implement the SVG-style mapLocalToContainer() implementation would affect all non-SVG callsites for no reason. But still you have a valid point. I propose to add only a protected mapLocalToContainerForSVG() method and not implement RenderLayerModelObject::mapLocalToContainer(). Each SVG renderers that needs mapLocalToContainer() can implement it and just call mapLocalToContainerSVG()… What do you think?

(In reply to Nikolas Zimmermann from comment #6) > That sounds fishy to me: > > a) I would not expect a function in RenderLayerModelObject to assert that > it‘s called on a RenderSVGModelObject/RenderSVGBlock only. > > b) It hurting extensibility: a new HTML/CSS renderer class inheriting from > RenderLayerModelObject has to implement mapLocalToContainer() and call the > super-super class implementation (e.g. RenderLineBreak needs to skip > RenderLayerModelObject::mapLocalToContainer()). The „short cut“ to implement > the SVG-style mapLocalToContainer() implementation would affect all non-SVG > callsites for no reason. > > But still you have a valid point. I propose to add only a protected > mapLocalToContainerForSVG() method and not implement > RenderLayerModelObject::mapLocalToContainer(). Each SVG renderers that needs > mapLocalToContainer() can implement it and just call > mapLocalToContainerSVG()… > > What do you think? I was more looking at computeVisibleRectInContainer. But your suggestion sounds good to me.

Created attachment 447817 [details] Patch, v3

Created attachment 447819 [details] Patch, v4

Comment on attachment 447819 [details] Patch, v4 View in context: https://bugs.webkit.org/attachment.cgi?id=447819&action=review > Source/WebCore/rendering/svg/RenderSVGModelObject.h:95 > + virtual Path computeClipPath(AffineTransform&) const; I think it is better to introduce these (RenderLayer specific) APIs when we start calling them. > Source/WebCore/rendering/svg/RenderSVGModelObject.h:116 > + void paintSVGOutline(PaintInfo&, const LayoutPoint& adjustedPaintOffset); Ditto.

Created attachment 447866 [details] Patch, v5

(In reply to Rob Buis from comment #10) > Comment on attachment 447819 [details] > Patch, v4 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=447819&action=review > > > Source/WebCore/rendering/svg/RenderSVGModelObject.h:95 > > + virtual Path computeClipPath(AffineTransform&) const; > > I think it is better to introduce these (RenderLayer specific) APIs when we > start calling them. > > > Source/WebCore/rendering/svg/RenderSVGModelObject.h:116 > > + void paintSVGOutline(PaintInfo&, const LayoutPoint& adjustedPaintOffset); > > Ditto. I agree with both.

Comment on attachment 447866 [details] Patch, v5 View in context: https://bugs.webkit.org/attachment.cgi?id=447866&action=review Getting closer :) > Source/WebCore/rendering/svg/RenderSVGModelObject.cpp:179 > + auto* container = this->parent(); Probably this can be removed. > Source/WebCore/rendering/svg/RenderSVGModelObject.cpp:212 > + auto repaintBoundingBox = enclosingLayoutRect(this->repaintRectInLocalCoordinates()); Ditto? > Source/WebCore/rendering/svg/RenderSVGModelObject.cpp:228 > + if (style().visibility() == Visibility::Hidden || style().display() == DisplayType::None) Can DisplayType::None really happen? What about display: contents? > Source/WebCore/rendering/svg/RenderSVGModelObject.cpp:278 > +Path RenderSVGModelObject::computeClipPath(AffineTransform& transform) const Needs to be removed.

Created attachment 447887 [details] Patch, v6

<rdar://problem/86944226>

(In reply to Rob Buis from comment #13) > Getting closer :) Step by step :-) > > > Source/WebCore/rendering/svg/RenderSVGModelObject.cpp:179 > > + auto* container = this->parent(); > > Probably this can be removed. Done. > > Source/WebCore/rendering/svg/RenderSVGModelObject.cpp:212 > > + auto repaintBoundingBox = enclosingLayoutRect(this->repaintRectInLocalCoordinates()); > > Ditto? Indeed, the function call used to be named 'repaintBoundingBox' in the downstream branch, that's a left-over from renaming back to legacy nomenclature: 'repaintRectInLocalCoordinates' (to minimize patch size). > > Source/WebCore/rendering/svg/RenderSVGModelObject.cpp:228 > > + if (style().visibility() == Visibility::Hidden || style().display() == DisplayType::None) > > Can DisplayType::None really happen? What about display: contents? DisplayType::None can happen -- we create renderers for e.g. <g display="none">. No idea about "display: contents" - never used it nor thought about its usage within SVG context. That's a general issue I think, out of scope for this patch -- shouldPaintSVGRenderer() behaves the same in the legacy engine. > Source/WebCore/rendering/svg/RenderSVGModelObject.cpp:278 > > +Path RenderSVGModelObject::computeClipPath(AffineTransform& transform) const > > Needs to be removed. Already done in patch v6, which was posted before I saw these comments. New patch coming soon.

Created attachment 448214 [details] Patch, v7

(In reply to Nikolas Zimmermann from comment #16) > > Can DisplayType::None really happen? What about display: contents? > DisplayType::None can happen -- we create renderers for e.g. <g > display="none">. No idea about "display: contents" - never used it nor > thought about its usage within SVG context. That's a general issue I think, > out of scope for this patch -- shouldPaintSVGRenderer() behaves the same in > the legacy engine. Agreed that display: contents is out of scope for this patch, we should remember to look into it at some point though.

Comment on attachment 448214 [details] Patch, v7 View in context: https://bugs.webkit.org/attachment.cgi?id=448214&action=review Probably by now copyright info has to be updated to include 2022 if changes are made in the file(s). > Source/WebCore/rendering/RenderLayerModelObject.cpp:252 > + ASSERT_UNUSED(containerIsSkipped, !containerIsSkipped); What is this for? > Source/WebCore/rendering/svg/RenderSVGModelObject.cpp:64 > + if (RenderObject::node() && is<SVGGraphicsElement>(nodeForNonAnonymous())) Can we have anonymous nodes in SVG? > Source/WebCore/rendering/svg/RenderSVGModelObject.cpp:107 > + ASSERT_UNUSED(ancestorSkipped, !ancestorSkipped); Ditto. > Source/WebCore/rendering/svg/RenderSVGModelObject.cpp:187 > + bool preserve3D = mode & UseTransforms && (container->style().preserves3D() || style().preserves3D()); The preserve3D checks below could be stored in a temp variable at this line, making things a bit more readable hopefully. > Source/WebCore/rendering/svg/RenderSVGModelObject.cpp:242 > + if (other.isEmpty() && !r.isEmpty()) Looks like r.isEmpty() is already proven to be false!

(In reply to Rob Buis from comment #19) > Comment on attachment 448214 [details] > Patch, v7 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=448214&action=review > > Probably by now copyright info has to be updated to include 2022 if changes > are made in the file(s). Correct, done. > > > Source/WebCore/rendering/RenderLayerModelObject.cpp:252 > > + ASSERT_UNUSED(containerIsSkipped, !containerIsSkipped); > > What is this for? This is a copy of the generic computeVisibleRectInContainer() method, adapted for SVG. Since containerIsSkipped is always false in SVG content, this is assert'ed here. > > > Source/WebCore/rendering/svg/RenderSVGModelObject.cpp:64 > > + if (RenderObject::node() && is<SVGGraphicsElement>(nodeForNonAnonymous())) > > Can we have anonymous nodes in SVG? We indeed had at some point in the LBSE prototype, when I modelled SVG view box, by creating an anonymous RenderSVGViewportContainer as direct child of the RenderSVGRoot, encapsulating all SVG content. While this has some benefits, the complexity increase is not worth it, so I've abandoned this. I'll adapt the code to remove the node() check (it would then assert in nodeForNonAnymous() when called for anonymous nodes). > > Source/WebCore/rendering/svg/RenderSVGModelObject.cpp:107 > > + ASSERT_UNUSED(ancestorSkipped, !ancestorSkipped); > > Ditto. I'd like to keep this... > > > Source/WebCore/rendering/svg/RenderSVGModelObject.cpp:187 > > + bool preserve3D = mode & UseTransforms && (container->style().preserves3D() || style().preserves3D()); > ... and also that, since it's a 1:1 copy of the generic methods, adapted for SVG. > The preserve3D checks below could be stored in a temp variable at this line, > making things a bit more readable hopefully. I agree, but maybe we'll save this for refactoring all of the preserve3D checks in the render tree? > > > Source/WebCore/rendering/svg/RenderSVGModelObject.cpp:242 > > + if (other.isEmpty() && !r.isEmpty()) > > Looks like r.isEmpty() is already proven to be false! Hehe, yes, fixed.

Created attachment 448222 [details] Patch, v8

Committed r287538 (245673@trunk): <https://commits.webkit.org/245673@trunk>

Thanks Rob!