[webkit-reviews] review requested: [Bug 109966] Early out of RenderLayer::updateCanBeStackingContainer more often : [Attachment 191282] Patch

[bugzilla-daemon at webkit.org]

Glenn Hartmann <hartmanng at chromium.org> has asked  for review:
Bug 109966: Early out of RenderLayer::updateCanBeStackingContainer more often
https://bugs.webkit.org/show_bug.cgi?id=109966

Attachment 191282: Patch
https://bugs.webkit.org/attachment.cgi?id=191282&action=review

------- Additional Comments from Glenn Hartmann <hartmanng at chromium.org>
(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.