Bug 41497 – GraphicsLayer: z-index style change causes a repaint of a layer

• Component: WebCore Misc.
• Status: NEW

[Sam Magnuson]

A z-index change on a layer backed node shouldn't really need redisplay - the backend can determine the drawing order and if cached just use the old contents.

[Sam Magnuson]

Created attachment 60326 [details]
Patch

[WebKit Review Bot]

Attachment 60326 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/rendering/style/RenderStyle.cpp:490:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Magnuson]

Created attachment 60342 [details]
Patch

[chris fleizach]

Comment on attachment 60342 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=60342&action=review

> WebCore/ChangeLog:10
> +

title of the bug should go first, then the bug URL. then after a new line, the comments about the bug.

> WebCore/rendering/RenderObject.cpp:1499
> +    // If zindex changed, and we are not composited, need to repaint

please make this comment into a full sentence, with period

[Sam Magnuson]

The comment that is there is (more or less) a duplicate of the comment in the conditions around it - are you asking me to fix those as well? Is the patch otherwise acceptable?

[Sam Magnuson]

Created attachment 70169 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 70169 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=70169&action=review

> WebCore/rendering/style/RenderStyle.cpp:510
> +        } else if (m_box->zIndex() != other->m_box->zIndex() || m_box->hasAutoZIndex() != other->m_box->hasAutoZIndex()
> +                 || visual->clip != other->visual->clip || visual->hasClip != other->visual->hasClip)
> +#if USE(ACCELERATED_COMPOSITING)
> +            changedContextSensitiveProperties |= ContextSensitivePropertyZIndex;

You're setting ContextSensitivePropertyZIndex when the clip changes, which is wrong.

[Sam Magnuson]

Created attachment 71503 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 71503 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71503&action=review

> WebCore/rendering/style/RenderStyle.cpp:515
> +        } else if (visual->clip != other->visual->clip || visual->hasClip != other->visual->hasClip) {
> +            return StyleDifferenceRepaintLayer;
> +        }

Style.

[Sam Magnuson]

(In reply to comment #9)
> (From update of attachment 71503 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71503&action=review
> 
> > WebCore/rendering/style/RenderStyle.cpp:515
> > +        } else if (visual->clip != other->visual->clip || visual->hasClip != other->visual->hasClip) {
> > +            return StyleDifferenceRepaintLayer;
> > +        }
> 
> Style.

I'm not clear what is wrong here - what is it that is undesirable?

[Simon Fraser (smfr)]

Single line clauses don't need braces.

[Sam Magnuson]

Created attachment 75702 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 75702 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75702&action=review

cq- for the bad ChagneLgo entry.

> WebCore/ChangeLog:27382
> [phone]  Sam Magnuson  <[e-mail]>

Your ChangeLog entry shoudl be at the top of the file.

[Sam Magnuson]

Created attachment 78617 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 78617 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78617&action=review

> Source/WebCore/ChangeLog:5
> +        GraphicsLayer: z-index causes a repaint of a layer

Should be "z-index change causes..."

> Source/WebCore/ChangeLog:8


This needs to be re-worded; it's not very clear.

> Source/WebCore/ChangeLog:9
> +        https://bugs.webkit.org/show_bug.cgi?id=41497

Bug URL should come after the bug title. webkit-patch will format this for you.

> Source/WebCore/ChangeLog:11
> +        No new tests; this is an optimization.

But I don't think that are there tests that exercise this code path, so new tests should be added (in LayoutTests/compositing).

> Source/WebCore/rendering/style/RenderStyle.cpp:530
> -        } else if (m_box->zIndex() != other->m_box->zIndex() || m_box->hasAutoZIndex() != other->m_box->hasAutoZIndex() ||
> -                 visual->clip != other->visual->clip || visual->hasClip != other->visual->hasClip)
> +        } else if (m_box->zIndex() != other->m_box->zIndex() || m_box->hasAutoZIndex() != other->m_box->hasAutoZIndex()
> +                   || visual->clip != other->visual->clip || visual->hasClip != other->visual->hasClip)
> +#if USE(ACCELERATED_COMPOSITING)

This is wrong. The previous version of the patch was more correctly.

I'm a bit worried about going from auto z-index to explicit z-index, because this also affects whether the element creates a stacking context. I think you should only do this optimization if old and new style both have explicit z-index.