Bug 73731 – RenderLayer::backgroundClipRect should not check parent()

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Julien Chaffraix]

All the callers are already calling parent() and it is a pretty hot function in some benchmark. Let's also clean up the function a bit while at it.

Patch forthcoming.

[Julien Chaffraix]

Created attachment 117734 [details]
Proposed change 1: remove parent() + some clean-up.

[Simon Fraser (smfr)]

Comment on attachment 117734 [details]
Proposed change 1: remove parent() + some clean-up.

View in context: https://bugs.webkit.org/attachment.cgi?id=117734&action=review

Did you do archaeology to see why the parent() check was there?

> Source/WebCore/rendering/RenderLayer.cpp:3617
> +    if (position == AbsolutePosition)
> +        return parentRects.posClipRect();

Does renderer-> isPositioned() return true for position:relative? If so, then you broke stuff.

[Julien Chaffraix]

Comment on attachment 117734 [details]
Proposed change 1: remove parent() + some clean-up.

View in context: https://bugs.webkit.org/attachment.cgi?id=117734&action=review

>> Source/WebCore/rendering/RenderLayer.cpp:3617
>> +        return parentRects.posClipRect();
> 
> Does renderer-> isPositioned() return true for position:relative? If so, then you broke stuff.

No, it does not. Here is the comment in RenderObject (still accurate per RenderBox::updateBoxModelInfoFromStyle):

bool isPositioned() const { return m_positioned; } // absolute or fixed positioning

isPositioned() also returns true for the RenderView but it will never be called here as it has no parent.

[Julien Chaffraix]

(In reply to comment #2)
> (From update of attachment 117734 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117734&action=review
> 
> Did you do archaeology to see why the parent() check was there?

I just did as I missed your question. The parent() check inside backgroundClipRect is a legacy of bug 29346 which split calculateRects in 2. It looks like it was missed during the split that all call sites were already checking parent() which made the check redundant.

[WebKit Review Bot]

Comment on attachment 117734 [details]
Proposed change 1: remove parent() + some clean-up.

Clearing flags on attachment: 117734

Committed r103955: <http://trac.webkit.org/changeset/103955>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.