Bug 85437 – Move RenderLayers z-index lists dirtying to post style change

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Julien Chaffraix]

This is a follow-up of bug 84920, currently RenderLayer z-index list dirtying is done in RenderBoxModelObject::styleWillChange.

This has one downside: to tighten our code, we would rather be able to determine when we are switch from being a stacking context to a non-stacking context. We could add a pre-style change hook in RenderLayer but we already have a post-style change hook in RenderLayer::styleChanged. To avoid duplication, we should try to move the code there.

As a side note, this would also make the layer handle its low-level operation instead of delegating that to the associated RenderBoxModelObject.

[Julien Chaffraix]

Created attachment 140623 [details]
Proposed refactoring 1: Move code and added handling for the stacking context transition.

[Darin Adler]

Comment on attachment 140623 [details]
Proposed refactoring 1: Move code and added handling for the stacking context transition.

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.

[Julien Chaffraix]

Created attachment 140827 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 140827 [details]
Patch for landing

Clearing flags on attachment: 140827

Committed r116480: <http://trac.webkit.org/changeset/116480>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.