89019 2012-06-13 10:36:04 -0700 Remove redundant code from RenderView and RenderBlock 2012-06-13 19:04:19 -0700 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 yael webkit-unassigned eric hyatt inferno mitz webkit.review.bot oldest_to_newest 648232 0 yael 2012-06-13 10:36:04 -0700 As pointed out in https://bugs.webkit.org/show_bug.cgi?id=83981#c7 , RenderView::insertFixedPositionedObject() is redundant. A separate solution is needed for fixed elements that are inside a ForeignObject. 648247 1 147362 yael 2012-06-13 10:53:43 -0700 Created attachment 147362 Patch. 648475 2 inferno 2012-06-13 14:37:06 -0700 Putting history here if someone wants to check Comment #7 From Abhishek Arya 2012-06-11 00:03:45 PST (-) [reply] (From update of attachment 137332 [details]) View in context: https://bugs.webkit.org/attachment.cgi?id=137332&action=review > Source/WebCore/rendering/RenderBlock.cpp:3422 > + if (o->style()->position() == FixedPosition && view()) I don't think this is right and you should definitely ask Dave Hyatt for a review of this patch. Two reasons:: 1) 99% of the time, fixed positioned objects are always added to their containing view. if (child->isPositioned()) { child->containingBlock()->insertPositionedObject(child); and if you see containingBlock() if (!isText() && m_style->position() == FixedPosition) { while (o && !o->isRenderView() && !(o->hasTransform() && o->isRenderBlock())) o = o->parent(); we would only return our containing view. 2) There are some exceptions for cases like <foreignObject>. http://trac.webkit.org/changeset/119914 This call is just redundant and forces things to be always added to renderview which is incorrect for cases like <foreignObject>, etc. Comment #8 From Yael 2012-06-11 05:45:08 PST (-) [reply] (In reply to comment #7) > (From update of attachment 137332 [details] [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=137332&action=review > > > Source/WebCore/rendering/RenderBlock.cpp:3422 > > + if (o->style()->position() == FixedPosition && view()) > > I don't think this is right and you should definitely ask Dave Hyatt for a review of this patch. Two reasons:: > 1) 99% of the time, fixed positioned objects are always added to their containing view. > if (child->isPositioned()) { > child->containingBlock()->insertPositionedObject(child); > and if you see containingBlock() > if (!isText() && m_style->position() == FixedPosition) { > while (o && !o->isRenderView() && !(o->hasTransform() && o->isRenderBlock())) > o = o->parent(); > we would only return our containing view. > 2) There are some exceptions for cases like <foreignObject>. http://trac.webkit.org/changeset/119914 > > This call is just redundant and forces things to be always added to renderview which is incorrect for cases like <foreignObject>, etc. thanks for your comment, I'll take a look :) Comment #9 From Yael 2012-06-12 18:15:45 PST (-) [reply] (In reply to comment #7) > (From update of attachment 137332 [details] [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=137332&action=review > > > Source/WebCore/rendering/RenderBlock.cpp:3422 > > + if (o->style()->position() == FixedPosition && view()) > > I don't think this is right and you should definitely ask Dave Hyatt for a review of this patch. Two reasons:: > 1) 99% of the time, fixed positioned objects are always added to their containing view. > if (child->isPositioned()) { > child->containingBlock()->insertPositionedObject(child); > and if you see containingBlock() > if (!isText() && m_style->position() == FixedPosition) { > while (o && !o->isRenderView() && !(o->hasTransform() && o->isRenderBlock())) > o = o->parent(); > we would only return our containing view. > 2) There are some exceptions for cases like <foreignObject>. http://trac.webkit.org/changeset/119914 > > This call is just redundant and forces things to be always added to renderview which is incorrect for cases like <foreignObject>, etc. This list is used for quickly identifying all the fixed position elements, so that we can mark them for layout, is that an incorrect way for doing that? BTW, The same idea is used in http://opensource.apple.com/source/WebCore/WebCore-1298.39/rendering/RenderView.cpp (search for RenderView::setCustomFixedPositionedObjectsNeedLayout). Comment #10 From Abhishek Arya 2012-06-12 19:19:51 PST (-) [reply] (In reply to comment #9) > (In reply to comment #7) > > (From update of attachment 137332 [details] [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=137332&action=review > > > > > Source/WebCore/rendering/RenderBlock.cpp:3422 > > > + if (o->style()->position() == FixedPosition && view()) > > > > I don't think this is right and you should definitely ask Dave Hyatt for a review of this patch. Two reasons:: > > 1) 99% of the time, fixed positioned objects are always added to their containing view. > > if (child->isPositioned()) { > > child->containingBlock()->insertPositionedObject(child); > > and if you see containingBlock() > > if (!isText() && m_style->position() == FixedPosition) { > > while (o && !o->isRenderView() && !(o->hasTransform() && o->isRenderBlock())) > > o = o->parent(); > > we would only return our containing view. > > 2) There are some exceptions for cases like <foreignObject>. http://trac.webkit.org/changeset/119914 > > > > This call is just redundant and forces things to be always added to renderview which is incorrect for cases like <foreignObject>, etc. > > This list is used for quickly identifying all the fixed position elements, so that we can mark them for layout, is that an incorrect way for doing that? > BTW, The same idea is used in http://opensource.apple.com/source/WebCore/WebCore-1298.39/rendering/RenderView.cpp (search for RenderView::setCustomFixedPositionedObjectsNeedLayout). Fixed position objects are already added to their RenderView in most cases. Why did you need to define insertFixedPositionedObject, removeFixedPositionedObject and call them in insertPositionedObject and removePositionedObject ? That part is wrong. you should see that all callers to insertPositionedObject are like child->containingBlock()->insertPositionedObject and read the containingBlock code if (!isText() && m_style->position() == FixedPosition) { while (o && !o->isRenderView() && !(o->hasTransform() && o->isRenderBlock())) { #if ENABLE(SVG) // foreignObject is the containing block for its contents. if (o->isSVGForeignObject()) break; #endif o = o->parent(); } } What you are doing here is causing redundant calls which will slow down insertPositionedObject and will cause it to be added in RenderView where it was not intended. e.g. o->hasTransform() && o->isRenderBlock() AND o->isSVGForeignObject() 648681 3 147362 webkit.review.bot 2012-06-13 19:04:14 -0700 Comment on attachment 147362 Patch. Clearing flags on attachment: 147362 Committed r120265: <http://trac.webkit.org/changeset/120265> 648682 4 webkit.review.bot 2012-06-13 19:04:19 -0700 All reviewed patches have been landed. Closing bug. 147362 2012-06-13 10:53:43 -0700 2012-06-13 19:04:14 -0700 Patch. 89019.diff text/plain 3125 yael SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEyMDIyNCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIyIEBACisyMDEyLTA2LTEzICBZYWVsIEFo YXJvbiAgPHlhZWwuYWhhcm9uQG5va2lhLmNvbT4KKworICAgICAgICBSZW1vdmUgcmVkdW5kYW50 IGNvZGUgZnJvbSBSZW5kZXJWaWV3IGFuZCBSZW5kZXJCbG9jaworICAgICAgICBodHRwczovL2J1 Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9ODkwMTkKKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBBcyBwb2ludGVkIG91dCBpbiBodHRwczovL2J1 Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9ODM5ODEjYzcgLCBSZW5kZXJWaWV3OjppbnNl cnRGaXhlZFBvc2l0aW9uZWRPYmplY3QoKQorICAgICAgICBpcyByZWR1bmRhbnQgYW5kIHNob3Vs ZCBiZSByZW1vdmVkLgorICAgICAgICBObyBuZXcgdGVzdHMuCisKKyAgICAgICAgKiByZW5kZXJp bmcvUmVuZGVyQmxvY2suY3BwOgorICAgICAgICAoV2ViQ29yZTo6UmVuZGVyQmxvY2s6Omluc2Vy dFBvc2l0aW9uZWRPYmplY3QpOgorICAgICAgICAoV2ViQ29yZTo6UmVuZGVyQmxvY2s6OnJlbW92 ZVBvc2l0aW9uZWRPYmplY3QpOgorICAgICAgICAqIHJlbmRlcmluZy9SZW5kZXJWaWV3LmNwcDoK KyAgICAgICAgKFdlYkNvcmUpOgorICAgICAgICAqIHJlbmRlcmluZy9SZW5kZXJWaWV3Lmg6Cisg ICAgICAgIChSZW5kZXJWaWV3KToKKwogMjAxMi0wNi0xMyAgQW15IE91c3RlcmhvdXQgIDxhb3Vz dGVyaEBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgUmVuYW1lIGN1cnJlbnREZXZpY2VNb3Rpb24g dG8gbGFzdE1vdGlvbiBpbiBEZXZpY2VNb3Rpb25DbGllbnQKSW5kZXg6IFNvdXJjZS9XZWJDb3Jl L3JlbmRlcmluZy9SZW5kZXJWaWV3LmgKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUvcmVu ZGVyaW5nL1JlbmRlclZpZXcuaAkocmV2aXNpb24gMTIwMTkzKQorKysgU291cmNlL1dlYkNvcmUv cmVuZGVyaW5nL1JlbmRlclZpZXcuaAkod29ya2luZyBjb3B5KQpAQCAtMTgzLDggKzE4Myw2IEBA CiAgICAgSW50U2l6ZSB2aWV3cG9ydFNpemUoKSBjb25zdCB7IHJldHVybiBkb2N1bWVudCgpLT52 aWV3cG9ydFNpemUoKTsgfQogCiAgICAgdm9pZCBzZXRGaXhlZFBvc2l0aW9uZWRPYmplY3RzTmVl ZExheW91dCgpOwotICAgIHZvaWQgaW5zZXJ0Rml4ZWRQb3NpdGlvbmVkT2JqZWN0KFJlbmRlckJv eCopOwotICAgIHZvaWQgcmVtb3ZlRml4ZWRQb3NpdGlvbmVkT2JqZWN0KFJlbmRlckJveCopOwog CiBwcm90ZWN0ZWQ6CiAgICAgdmlydHVhbCB2b2lkIG1hcExvY2FsVG9Db250YWluZXIoUmVuZGVy Qm94TW9kZWxPYmplY3QqIHJlcGFpbnRDb250YWluZXIsIGJvb2wgdXNlVHJhbnNmb3JtcywgYm9v bCBmaXhlZCwgVHJhbnNmb3JtU3RhdGUmLCBBcHBseUNvbnRhaW5lckZsaXBPck5vdCA9IEFwcGx5 Q29udGFpbmVyRmxpcCwgYm9vbCogd2FzRml4ZWQgPSAwKSBjb25zdDsKSW5kZXg6IFNvdXJjZS9X ZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJWaWV3LmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9yZW5kZXJpbmcvUmVuZGVyVmlldy5jcHAJKHJldmlzaW9uIDEyMDE5MykKKysrIFNvdXJj ZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJWaWV3LmNwcAkod29ya2luZyBjb3B5KQpAQCAtOTQ1 LDIwICs5NDUsNCBAQAogICAgIH0KIH0KIAotdm9pZCBSZW5kZXJWaWV3OjppbnNlcnRGaXhlZFBv c2l0aW9uZWRPYmplY3QoUmVuZGVyQm94KiBvYmplY3QpCi17Ci0gICAgaWYgKCFtX3Bvc2l0aW9u ZWRPYmplY3RzKQotICAgICAgICBtX3Bvc2l0aW9uZWRPYmplY3RzID0gYWRvcHRQdHIobmV3IFBv c2l0aW9uZWRPYmplY3RzTGlzdEhhc2hTZXQpOwotCi0gICAgbV9wb3NpdGlvbmVkT2JqZWN0cy0+ YWRkKG9iamVjdCk7Ci19Ci0KLXZvaWQgUmVuZGVyVmlldzo6cmVtb3ZlRml4ZWRQb3NpdGlvbmVk T2JqZWN0KFJlbmRlckJveCogb2JqZWN0KQotewotICAgIGlmICghbV9wb3NpdGlvbmVkT2JqZWN0 cykKLSAgICAgICAgcmV0dXJuOwotCi0gICAgbV9wb3NpdGlvbmVkT2JqZWN0cy0+cmVtb3ZlKG9i amVjdCk7Ci19Ci0KIH0gLy8gbmFtZXNwYWNlIFdlYkNvcmUKSW5kZXg6IFNvdXJjZS9XZWJDb3Jl L3JlbmRlcmluZy9SZW5kZXJCbG9jay5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNvcmUv cmVuZGVyaW5nL1JlbmRlckJsb2NrLmNwcAkocmV2aXNpb24gMTIwMTkzKQorKysgU291cmNlL1dl YkNvcmUvcmVuZGVyaW5nL1JlbmRlckJsb2NrLmNwcAkod29ya2luZyBjb3B5KQpAQCAtMzUwOSwx OCArMzUwOSwxMiBAQAogICAgICAgICBtX3Bvc2l0aW9uZWRPYmplY3RzID0gYWRvcHRQdHIobmV3 IFBvc2l0aW9uZWRPYmplY3RzTGlzdEhhc2hTZXQpOwogCiAgICAgbV9wb3NpdGlvbmVkT2JqZWN0 cy0+YWRkKG8pOwotCi0gICAgaWYgKG8tPnN0eWxlKCktPnBvc2l0aW9uKCkgPT0gRml4ZWRQb3Np dGlvbiAmJiB2aWV3KCkpCi0gICAgICAgIHZpZXcoKS0+aW5zZXJ0Rml4ZWRQb3NpdGlvbmVkT2Jq ZWN0KG8pOwogfQogCiB2b2lkIFJlbmRlckJsb2NrOjpyZW1vdmVQb3NpdGlvbmVkT2JqZWN0KFJl bmRlckJveCogbykKIHsKICAgICBpZiAobV9wb3NpdGlvbmVkT2JqZWN0cykKICAgICAgICAgbV9w b3NpdGlvbmVkT2JqZWN0cy0+cmVtb3ZlKG8pOwotCi0gICAgaWYgKHZpZXcoKSkKLSAgICAgICAg dmlldygpLT5yZW1vdmVGaXhlZFBvc2l0aW9uZWRPYmplY3Qobyk7CiB9CiAKIHZvaWQgUmVuZGVy QmxvY2s6OnJlbW92ZVBvc2l0aW9uZWRPYmplY3RzKFJlbmRlckJsb2NrKiBvKQo=