[Webkit-unassigned] [Bug 109966] Early out of RenderLayer::updateCanBeStackingContainer more often

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Mar 4 11:58:09 PST 2013


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


Glenn Hartmann <hartmanng at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #191282|                            |review?
               Flag|                            |




--- Comment #4 from Glenn Hartmann <hartmanng at chromium.org>  2013-03-04 12:00:32 PST ---
Created an attachment (id=191282)
 --> (https://bugs.webkit.org/attachment.cgi?id=191282&action=review)
Patch

(In reply to comment #3)
> (From update of attachment 191080 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191080&action=review
> 
> > Source/WebCore/rendering/RenderBox.cpp:360
> > +            if (layer())
> > +                layer()->dirtyAncestorStackingContextDescendantsAreContiguousInStackingOrder();
> 
> I think this should call a function on RenderLayer with a simpler name that describes when it's called, but I can't tell from this call site what actually matters.

done.


> > Source/WebCore/rendering/RenderLayer.cpp:710
> > +    // Overflow are a box concept.
> 
> is a?

yup, fixed.


> > Source/WebCore/rendering/RenderLayer.cpp:721
> > +    bool overflowXHasChanged = overflowX == OSCROLL && (!oldStyle || oldStyle->overflowX() != OSCROLL);
> > +    bool overflowYHasChanged = overflowY == OSCROLL && (!oldStyle || oldStyle->overflowY() != OSCROLL);
> > +    if (overflowXHasChanged || overflowYHasChanged)
> > +        dirtyAncestorStackingContextDescendantsAreContiguousInStackingOrder();
> 
> It's odd that you're dirtying a flag related to stacking order when overflow changes. Sure, it's related to composited overflow, but you're making the assumption that only composited overflow cares about the flag.

I've abstracted this out into a new overflowDidChange() function, it should be more generalizable.


> > Source/WebCore/rendering/RenderLayer.cpp:1140
> > +            ScopedNeedsCompositedScrollingUpdater(this);
> 
> This seems like an odd place for a Scope thing. Won't it be destroyed two lines below?

Yes, that's intentional. This works together with the other scoped objects further up in the stack to add itself to its stacking context's m_nextLayerToUpdateNeedsCompositedScrolling list.


> > Source/WebCore/rendering/RenderLayer.h:1319
> > +    RenderLayer* m_nextLayerToUpdateNeedsCompositedScrolling;
> > +    RenderLayer* m_lastLayerToUpdateNeedsCompositedScrolling;
> 
> It seems wrong to take up space on RenderLayer with these.

I've modified this to be a LIFO list instead of FIFO, which allows us to use only one pointer instead of two. Alternatively, instead of storing a list of layers that need updating, we could store a dirty bit on each layer, and then walk the subtree from the stacking context and update any dirty layers when ScopedNeedsCompositedScrollingUpdater goes out of scope.

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


More information about the webkit-unassigned mailing list