Fix contain-size-replaced-002.html.
Created attachment 449034 [details] Patch
Created attachment 449057 [details] Patch
Comment on attachment 449057 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449057&action=review Unofficial review :-) > Source/WebCore/rendering/svg/LegacyRenderSVGRoot.cpp:268 > + if (clipViewport && (contentWidth() == LayoutUnit() || contentHeight() == LayoutUnit())) While a decent compiler won't create LayoutUnit temporaries over and over, this is still hard to read. One could either make use of LayoutUnits operator bool() -- however since it's marked explicit, this is also awkward: if (clipViewport && (!bool(contentWidth()) || !bool(contentHeight()))) Therefore I'd avoid using contentWidth()/contentHeight() and instead query 'LayoutSize contentSize()', which encapsulates the logic you're looking for in the "isEmpty" function: if (clipViewport && contentSize().isEmpty())
Created attachment 449151 [details] Patch
Comment on attachment 449057 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449057&action=review >> Source/WebCore/rendering/svg/LegacyRenderSVGRoot.cpp:268 >> + if (clipViewport && (contentWidth() == LayoutUnit() || contentHeight() == LayoutUnit())) > > While a decent compiler won't create LayoutUnit temporaries over and over, this is still hard to read. > > One could either make use of LayoutUnits operator bool() -- however since it's marked explicit, this is also awkward: > if (clipViewport && (!bool(contentWidth()) || !bool(contentHeight()))) > > Therefore I'd avoid using contentWidth()/contentHeight() and instead query 'LayoutSize contentSize()', which encapsulates the logic you're looking for in the "isEmpty" function: > if (clipViewport && contentSize().isEmpty()) That is much better, thanks! I updated the patch.
Looks great, unofficial r=me.
Comment on attachment 449151 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449151&action=review r=me > Source/WebCore/rendering/svg/LegacyRenderSVGRoot.cpp:269 > + if (clipViewport && contentSize().isEmpty()) > + return; One question, should we move this up on the method, to avoid doing other checks that might not be needed?
Created attachment 449156 [details] Patch
Comment on attachment 449151 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449151&action=review >> Source/WebCore/rendering/svg/LegacyRenderSVGRoot.cpp:269 >> + return; > > One question, should we move this up on the method, to avoid doing other checks that might not be needed? Good comment, I think the position is okay, but I made a change regarding addRelevantRepaintedObject that I will run by Niko. Thanks for the review.
Comment on attachment 449156 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449156&action=review > Source/WebCore/rendering/svg/LegacyRenderSVGRoot.cpp:-259 > - page().addRelevantUnpaintedObject(this, visualOverflowRect()); You're no longer keeping track of relevant _unpainted_ objects? Apparently we lack test coverage for this, nothing turned red :-)
Comment on attachment 449156 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449156&action=review >> Source/WebCore/rendering/svg/LegacyRenderSVGRoot.cpp:-259 >> - page().addRelevantUnpaintedObject(this, visualOverflowRect()); > > You're no longer keeping track of relevant _unpainted_ objects? > Apparently we lack test coverage for this, nothing turned red :-) Yeah, misread the code!
(In reply to Rob Buis from comment #9) > Comment on attachment 449151 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=449151&action=review > > >> Source/WebCore/rendering/svg/LegacyRenderSVGRoot.cpp:269 > >> + return; > > > > One question, should we move this up on the method, to avoid doing other checks that might not be needed? > > Good comment, I think the position is okay, but I made a change regarding > addRelevantRepaintedObject that I will run by Niko. Thanks for the review. Hmm, it depends. Apparently addRelevantRepaintedObject/UnpaintedObject is not covered by tests that the bots run at least. Robs new condition (when viewport is clipped and either width or height are empty, bail out) is preceded by: if (paintInfo.phase == PaintPhase::Foreground) page().addRelevantRepaintedObject(this, visualOverflowRect()); In LBSE I can immediately answer what the visualOverflowRect() looks like for a SVG renderer in general. However for LegacyRenderSVGRoot and the whole legacy SVG engine we always used to have the problem that we are not exactly sure of the different coordinate systems: visualOverflowRect() is different to SVG objectBoundingBox for instance, defined related to the 'border box rect' in CSS terms. So what does that mean for SVG? Hard to answer for the legacy SVG engine, needs to be checked case-by-case. Therefore I'm not sure: if there is an empty contentSize, why bother claiming that the LegacyRenderSVGRoot is a 'relevant repainted object' -- it's not. The visualOverflowRect() should have either empty width or height, just like the contentSize, no? At least one should printf() the visualOverflowRect / contentSize for a few cases, to understand this better. Rego is probably right, that we should move the contentSize().isEmpty() check at least above the addRelevantRepaintedObject() section. Who knows more about addRelevantRepaintedObject()?
Created attachment 449168 [details] Patch
Created attachment 449185 [details] Patch
ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.
Created attachment 449253 [details] Patch
Committed r288057 (246077@main): <https://commits.webkit.org/246077@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 449253 [details].
<rdar://problem/87639203>