Bug 89019 - Remove redundant code from RenderView and RenderBlock
Summary: Remove redundant code from RenderView and RenderBlock
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-13 10:36 PDT by Yael
Modified: 2012-06-13 19:04 PDT (History)
5 users (show)

See Also:


Attachments
Patch. (3.05 KB, patch)
2012-06-13 10:53 PDT, Yael
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yael 2012-06-13 10:36:04 PDT
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.
Comment 1 Yael 2012-06-13 10:53:43 PDT
Created attachment 147362 [details]
Patch.
Comment 2 Abhishek Arya 2012-06-13 14:37:06 PDT
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()
Comment 3 WebKit Review Bot 2012-06-13 19:04:14 PDT
Comment on attachment 147362 [details]
Patch.

Clearing flags on attachment: 147362

Committed r120265: <http://trac.webkit.org/changeset/120265>
Comment 4 WebKit Review Bot 2012-06-13 19:04:19 PDT
All reviewed patches have been landed.  Closing bug.