Bug 85437 - Move RenderLayers z-index lists dirtying to post style change
Summary: Move RenderLayers z-index lists dirtying to post style change
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Julien Chaffraix
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-05-02 17:17 PDT by Julien Chaffraix
Modified: 2012-05-08 19:18 PDT (History)
3 users (show)

See Also:


Attachments
Proposed refactoring 1: Move code and added handling for the stacking context transition. (7.59 KB, patch)
2012-05-07 17:01 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff
Patch for landing (7.70 KB, patch)
2012-05-08 17:32 PDT, Julien Chaffraix
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Julien Chaffraix 2012-05-02 17:17:01 PDT
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.
Comment 1 Julien Chaffraix 2012-05-07 17:01:23 PDT
Created attachment 140623 [details]
Proposed refactoring 1: Move code and added handling for the stacking context transition.
Comment 2 Darin Adler 2012-05-08 13:08:21 PDT
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.
Comment 3 Julien Chaffraix 2012-05-08 17:32:37 PDT
Created attachment 140827 [details]
Patch for landing
Comment 4 WebKit Review Bot 2012-05-08 19:17:56 PDT
Comment on attachment 140827 [details]
Patch for landing

Clearing flags on attachment: 140827

Committed r116480: <http://trac.webkit.org/changeset/116480>
Comment 5 WebKit Review Bot 2012-05-08 19:18:00 PDT
All reviewed patches have been landed.  Closing bug.