Bug 73731 - RenderLayer::backgroundClipRect should not check parent()
Summary: RenderLayer::backgroundClipRect should not check parent()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Julien Chaffraix
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-02 19:15 PST by Julien Chaffraix
Modified: 2012-01-03 12:09 PST (History)
2 users (show)

See Also:


Attachments
Proposed change 1: remove parent() + some clean-up. (3.63 KB, patch)
2011-12-02 19:29 PST, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2011-12-02 19:15:55 PST
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.
Comment 1 Julien Chaffraix 2011-12-02 19:29:24 PST
Created attachment 117734 [details]
Proposed change 1: remove parent() + some clean-up.
Comment 2 Simon Fraser (smfr) 2011-12-05 08:54:14 PST
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.
Comment 3 Julien Chaffraix 2011-12-05 12:41:45 PST
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.
Comment 4 Julien Chaffraix 2011-12-28 07:21:33 PST
(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.
Comment 5 WebKit Review Bot 2012-01-03 12:09:02 PST
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>
Comment 6 WebKit Review Bot 2012-01-03 12:09:06 PST
All reviewed patches have been landed.  Closing bug.