Bug 235175 – [css-contain] Fix contain-size-replaced-002.html

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Rob Buis]

Fix contain-size-replaced-002.html.

[Rob Buis]

Created attachment 449034 [details]
Patch

[Rob Buis]

Created attachment 449057 [details]
Patch

[Nikolas Zimmermann]

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

[Rob Buis]

Created attachment 449151 [details]
Patch

[Rob Buis]

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.

[Nikolas Zimmermann]

Looks great, unofficial r=me.

[Manuel Rego Casasnovas]

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?

[Rob Buis]

Created attachment 449156 [details]
Patch

[Rob Buis]

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.

[Nikolas Zimmermann]

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

[Rob Buis]

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!

[Nikolas Zimmermann]

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

[Rob Buis]

Created attachment 449168 [details]
Patch

[Rob Buis]

Created attachment 449185 [details]
Patch

[EWS]

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

[Rob Buis]

Created attachment 449253 [details]
Patch

[EWS]

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

[Radar WebKit Bug Importer]

<rdar://problem/87639203>