Bug 89019 – Remove redundant code from RenderView and RenderBlock

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Yael]

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.

[Yael]

Created attachment 147362 [details]
Patch.

[Abhishek Arya]

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

[WebKit Review Bot]

Comment on attachment 147362 [details]
Patch.

Clearing flags on attachment: 147362

Committed r120265: <http://trac.webkit.org/changeset/120265>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.