73731 2011-12-02 19:15:55 -0800 RenderLayer::backgroundClipRect should not check parent() 2012-01-03 12:09:06 -0800 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 1 jchaffraix jchaffraix simon.fraser webkit.review.bot oldest_to_newest 514375 0 jchaffraix 2011-12-02 19:15:55 -0800 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. 514377 1 117734 jchaffraix 2011-12-02 19:29:24 -0800 Created attachment 117734 Proposed change 1: remove parent() + some clean-up. 515108 2 117734 simon.fraser 2011-12-05 08:54:14 -0800 Comment on attachment 117734 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. 515310 3 117734 jchaffraix 2011-12-05 12:41:45 -0800 Comment on attachment 117734 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. 527666 4 jchaffraix 2011-12-28 07:21:33 -0800 (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. 528943 5 117734 webkit.review.bot 2012-01-03 12:09:02 -0800 Comment on attachment 117734 Proposed change 1: remove parent() + some clean-up. Clearing flags on attachment: 117734 Committed r103955: <http://trac.webkit.org/changeset/103955> 528944 6 webkit.review.bot 2012-01-03 12:09:06 -0800 All reviewed patches have been landed. Closing bug. 117734 2011-12-02 19:29:24 -0800 2012-01-03 12:09:02 -0800 Proposed change 1: remove parent() + some clean-up. bug-73731-20111202192923.patch text/plain 3720 jchaffraix U3VidmVyc2lvbiBSZXZpc2lvbjogMTAxMjc4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMDJiNDQ3MmEwNjdmMmFh NWE3NzM2MWFiZjgyOGQ2OGNkYzM1MjFiNS4uOTlmNTcwMGRmYzQwNTRiMDk1MjY5NDI0ZGVhYTU1 ZTZjMzg5ZDlkMyAxMDA3NTUKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIyIEBACisyMDExLTEyLTAyICBKdWxp ZW4gQ2hhZmZyYWl4ICA8amNoYWZmcmFpeEB3ZWJraXQub3JnPgorCisgICAgICAgIFJlbmRlckxh eWVyOjpiYWNrZ3JvdW5kQ2xpcFJlY3Qgc2hvdWxkIG5vdCBjaGVjayBwYXJlbnQoKQorICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NzM3MzEKKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBDbGVhbi11cCBvbmx5LCBu byBleHBlY3RlZCBjaGFuZ2UgaW4gYmVoYXZpb3IuCisKKyAgICAgICAgKiByZW5kZXJpbmcvUmVu ZGVyTGF5ZXIuY3BwOgorICAgICAgICAoV2ViQ29yZTo6YmFja2dyb3VuZENsaXBSZWN0Rm9yUG9z aXRpb24pOiBDaGFuZ2VkIFJlbmRlck9iamVjdDo6aXNQb3NpdGlvbmVkKCkgdG8KKyAgICAgICAg YSBjaGVjayBmb3IgQWJzb2x1dGVQb3NpdGlvbiBmb3IgY29uc2lzdGVuY3kgYnV0IGFsc28gYXMg dGhpcyBpcyBlcXVpdmFsZW50IGR1ZSB0bzoKKyAgICAgICAgLSB0aGUgcHJldmlvdXMgY2hlY2sg Zm9yIEZpeGVkUG9zaXRpb24uCisgICAgICAgIC0gUmVuZGVyVmlldywgd2hpY2ggaXMgcG9zaXRp b25lZCwgd2lsbCBuZXZlciBnb2VzIHRvIHRoaXMgY29kZSBhcyBpdCBoYXMgbm8gcGFyZW50KCku CisKKyAgICAgICAgKFdlYkNvcmU6OlJlbmRlckxheWVyOjpiYWNrZ3JvdW5kQ2xpcFJlY3QpOiBS ZW1vdmVkIHRoZSBwYXJlbnQoKSBjaGVjay4gV2hpbGUgYXQKKyAgICAgICAgaXQsIGFsc28gbW92 ZWQgdGhlIGlubGluZSBpbml0aWFsaXphdGlvbiBvZiB8YmFja2dyb3VuZENsaXBSZWN0fCB0byBp dHMgb3duIGZ1bmN0aW9uCisgICAgICAgIGFuZCByZW1vdmVkIGEgfHZpZXd8IGNoZWNrIGFzIHRo ZSBhc3NvY2lhdGVkIEFTU0VSVCBzZWVtcyB0byBuZXZlciBoYXZlIGJlZW4gcmVhY2hlZC4KKwog MjAxMS0xMS0yOCAgRmFkeSBTYW11ZWwgIDxmc2FtdWVsQGNocm9taXVtLm9yZz4KIAogICAgICAg ICBGaXggQXNwZWN0IFJhdGlvIFByb3BlcnR5IEluaGVyaXRhbmNlIEFuZCBNYWtlIHRoZSBDb21w dXRlZCBWYWx1ZSBFcXVhbCB0aGUgU3BlY2lmaWVkIFZhbHVlCmRpZmYgLS1naXQgYS9Tb3VyY2Uv V2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyTGF5ZXIuY3BwIGIvU291cmNlL1dlYkNvcmUvcmVuZGVy aW5nL1JlbmRlckxheWVyLmNwcAppbmRleCAwZjgwNmJiOGMxYzIwMmQ1ZTU3OWNjM2VlOTE3Yzg1 NmFhMzA4MmNlLi5lZjdkZmUxYjhkZGQzOWFiOGVhNzJkYTY4MmU1ZmYyY2I3M2VkNWQ4IDEwMDY0 NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyTGF5ZXIuY3BwCisrKyBiL1Nv dXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJMYXllci5jcHAKQEAgLTM2MDgsMjEgKzM2MDgs MjggQEAgdm9pZCBSZW5kZXJMYXllcjo6cGFyZW50Q2xpcFJlY3RzKGNvbnN0IFJlbmRlckxheWVy KiByb290TGF5ZXIsIFJlbmRlclJlZ2lvbiogcmUKICAgICBjbGlwUmVjdHMgPSAqcGFyZW50KCkt PmNsaXBSZWN0cygpOwogfQogCitzdGF0aWMgaW5saW5lIENsaXBSZWN0IGJhY2tncm91bmRDbGlw UmVjdEZvclBvc2l0aW9uKGNvbnN0IENsaXBSZWN0cyYgcGFyZW50UmVjdHMsIEVQb3NpdGlvbiBw b3NpdGlvbikKK3sKKyAgICBpZiAocG9zaXRpb24gPT0gRml4ZWRQb3NpdGlvbikKKyAgICAgICAg cmV0dXJuIHBhcmVudFJlY3RzLmZpeGVkQ2xpcFJlY3QoKTsKKworICAgIGlmIChwb3NpdGlvbiA9 PSBBYnNvbHV0ZVBvc2l0aW9uKQorICAgICAgICByZXR1cm4gcGFyZW50UmVjdHMucG9zQ2xpcFJl Y3QoKTsKKworICAgIHJldHVybiBwYXJlbnRSZWN0cy5vdmVyZmxvd0NsaXBSZWN0KCk7Cit9CisK IENsaXBSZWN0IFJlbmRlckxheWVyOjpiYWNrZ3JvdW5kQ2xpcFJlY3QoY29uc3QgUmVuZGVyTGF5 ZXIqIHJvb3RMYXllciwgUmVuZGVyUmVnaW9uKiByZWdpb24sIGJvb2wgdGVtcG9yYXJ5Q2xpcFJl Y3RzLCBPdmVybGF5U2Nyb2xsYmFyU2l6ZVJlbGV2YW5jeSByZWxldmFuY3kpIGNvbnN0CiB7Ci0g ICAgQ2xpcFJlY3QgYmFja2dyb3VuZFJlY3Q7Ci0gICAgaWYgKHBhcmVudCgpKSB7Ci0gICAgICAg IENsaXBSZWN0cyBwYXJlbnRSZWN0czsKLSAgICAgICAgcGFyZW50Q2xpcFJlY3RzKHJvb3RMYXll ciwgcmVnaW9uLCBwYXJlbnRSZWN0cywgdGVtcG9yYXJ5Q2xpcFJlY3RzLCByZWxldmFuY3kpOwot ICAgICAgICBiYWNrZ3JvdW5kUmVjdCA9IHJlbmRlcmVyKCktPnN0eWxlKCktPnBvc2l0aW9uKCkg PT0gRml4ZWRQb3NpdGlvbiA/IHBhcmVudFJlY3RzLmZpeGVkQ2xpcFJlY3QoKSA6Ci0gICAgICAg ICAgICAgICAgICAgICAgICAgKHJlbmRlcmVyKCktPmlzUG9zaXRpb25lZCgpID8gcGFyZW50UmVj dHMucG9zQ2xpcFJlY3QoKSA6IAotICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgIHBhcmVudFJlY3RzLm92ZXJmbG93Q2xpcFJlY3QoKSk7Ci0gICAg ICAgIFJlbmRlclZpZXcqIHZpZXcgPSByZW5kZXJlcigpLT52aWV3KCk7Ci0gICAgICAgIEFTU0VS VCh2aWV3KTsKLSAgICAgICAgaWYgKHZpZXcgJiYgcGFyZW50UmVjdHMuZml4ZWQoKSAmJiByb290 TGF5ZXItPnJlbmRlcmVyKCkgPT0gdmlldykKLSAgICAgICAgICAgIGJhY2tncm91bmRSZWN0Lm1v dmUodmlldy0+ZnJhbWVWaWV3KCktPnNjcm9sbFhGb3JGaXhlZFBvc2l0aW9uKCksIHZpZXctPmZy YW1lVmlldygpLT5zY3JvbGxZRm9yRml4ZWRQb3NpdGlvbigpKTsKLSAgICB9Ci0gICAgcmV0dXJu IGJhY2tncm91bmRSZWN0OworICAgIEFTU0VSVChwYXJlbnQoKSk7CisgICAgQ2xpcFJlY3RzIHBh cmVudFJlY3RzOworICAgIHBhcmVudENsaXBSZWN0cyhyb290TGF5ZXIsIHJlZ2lvbiwgcGFyZW50 UmVjdHMsIHRlbXBvcmFyeUNsaXBSZWN0cywgcmVsZXZhbmN5KTsKKyAgICBDbGlwUmVjdCBiYWNr Z3JvdW5kQ2xpcFJlY3QgPSBiYWNrZ3JvdW5kQ2xpcFJlY3RGb3JQb3NpdGlvbihwYXJlbnRSZWN0 cywgcmVuZGVyZXIoKS0+c3R5bGUoKS0+cG9zaXRpb24oKSk7CisgICAgUmVuZGVyVmlldyogdmll dyA9IHJlbmRlcmVyKCktPnZpZXcoKTsKKyAgICBBU1NFUlQodmlldyk7CisgICAgaWYgKHBhcmVu dFJlY3RzLmZpeGVkKCkgJiYgcm9vdExheWVyLT5yZW5kZXJlcigpID09IHZpZXcpCisgICAgICAg IGJhY2tncm91bmRDbGlwUmVjdC5tb3ZlKHZpZXctPmZyYW1lVmlldygpLT5zY3JvbGxYRm9yRml4 ZWRQb3NpdGlvbigpLCB2aWV3LT5mcmFtZVZpZXcoKS0+c2Nyb2xsWUZvckZpeGVkUG9zaXRpb24o KSk7CisgICAgcmV0dXJuIGJhY2tncm91bmRDbGlwUmVjdDsKIH0KIAogdm9pZCBSZW5kZXJMYXll cjo6Y2FsY3VsYXRlUmVjdHMoY29uc3QgUmVuZGVyTGF5ZXIqIHJvb3RMYXllciwgUmVuZGVyUmVn aW9uKiByZWdpb24sIGNvbnN0IExheW91dFJlY3QmIHBhaW50RGlydHlSZWN0LCBMYXlvdXRSZWN0 JiBsYXllckJvdW5kcywK