[webkit-reviews] review granted: [Bug 85437] Move RenderLayers z-index lists dirtying to post style change : [Attachment 140623] Proposed refactoring 1: Move code and added handling for the stacking context transition.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Julien Chaffraix
<jchaffraix at webkit.org>'s request for review:
Bug 85437: Move RenderLayers z-index lists dirtying to post style change
https://bugs.webkit.org/show_bug.cgi?id=85437

Attachment 140623: Proposed refactoring 1: Move code and added handling for the
stacking context transition.
https://bugs.webkit.org/attachment.cgi?id=140623&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=140623&action=review


> Source/WebCore/rendering/RenderLayer.cpp:195
> +    m_zOrderListsDirty = isStackingContext();

This needs a “why” comment.

> Source/WebCore/rendering/RenderLayer.cpp:4702
> +    // FIXME: RenderLayer already handles visibility changes through our
visiblity dirty bits. This logic could
> +    // likely be folded along with the rest.
> +    if (oldStyle->zIndex() != renderer()->style()->zIndex() ||
oldStyle->visibility() != renderer()->style()->visibility()) {
> +	   dirtyStackingContextZOrderLists();
> +	   if (isStackingContext)
> +	       dirtyZOrderLists();
> +    }

This work is redundant if isStackingContext != wasStackingContext since
dirtyStackingContextZOrderLists and dirtyZOrderLists were already called as
necessary. I think it would be better to structure the code so we don’t do it
twice. Could be as simple as putting a return inside the earlier code.

> Source/WebCore/rendering/RenderLayer.h:620
> +    bool layerWithStyleIsStackingContext(const RenderStyle* style) const {
return !style->hasAutoZIndex() || renderer()->isRenderView(); }

The naming here is strange. I think that it could be named isStackingContext
just like the public function and just be an overload that takes an explicit
style argument.