[webkit-reviews] review denied: [Bug 41497] GraphicsLayer: z-index style change causes a repaint of a layer : [Attachment 78617] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Sam Magnuson
<smagnuso at gmail.com>'s request for review:
Bug 41497: GraphicsLayer: z-index style change causes a repaint of a layer
https://bugs.webkit.org/show_bug.cgi?id=41497

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

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
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.