[Webkit-unassigned] [Bug 89019] Remove redundant code from RenderView and RenderBlock

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=89019


Abhishek Arya <inferno at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hyatt at apple.com,
                   |                            |inferno at chromium.org,
                   |                            |mitz at webkit.org




--- Comment #2 from Abhishek Arya <inferno at chromium.org>  2012-06-13 14:37:06 PST ---
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()

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.