[webkit-reviews] review denied: [Bug 121828] [CSS Regions] Activate all regions to have layers, as CSS Regions create a new stacking context : [Attachment 213002] patch. stacking contexts are not related to having backing.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Sep 30 09:53:58 PDT 2013


Alexandru Chiculita <achicu at adobe.com> has denied Mihai Maerean
<mmaerean at adobe.com>'s request for review:
Bug 121828: [CSS Regions] Activate all regions to have layers, as CSS Regions
create a new stacking context
https://bugs.webkit.org/show_bug.cgi?id=121828

Attachment 213002: patch. stacking contexts are not related to having backing.
https://bugs.webkit.org/attachment.cgi?id=213002&action=review

------- Additional Comments from Alexandru Chiculita <achicu at adobe.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=213002&action=review


> LayoutTests/ChangeLog:6
> +	   Reviewed by NOBODY (OOPS!).

Please explain why you had to change the tests below.

> LayoutTests/fast/repaint/region-painting-composited-element-expected.html:13
> +    position: relative; z-index: 1; /*create Stacking Context*/

nit: use two lines.

> Source/WebCore/ChangeLog:8
> +	   Test: fast/regions/stacking-context-paint-order.html

Please add more details on why the change is actually needed as we already
created layers in some cases before.

> Source/WebCore/rendering/RenderFlowThread.cpp:323
> +		   region->setRequiresLayerForCompositing();

It is not required to call setRequiresLayerForCompositing anymore as
requiresLayer is always going to be true.

> Source/WebCore/rendering/RenderRegion.cpp:722
> +void RenderRegion::setRequiresLayerForCompositing()

Please remove this function and all its friends:
RenderBox::updateLayerIfNeeded
RenderBox::updatePaintingContainerForFloatingObject
RenderBlock::updateFloatingObjectsPaintingContainer
RenderBlock::updateLocalFloatingObjectsForPaintingContainer
RenderBlock::updateAllDescendantsFloatingObjectsPaintingContainer

> Source/WebCore/rendering/RenderRegion.h:145
> +    virtual bool requiresLayer() const { return true; } // Because CSS
Regions create Stacking Contexts.

I would rewrite this to handle the isRenderRegionSet() instead of overwriting
it again below.


More information about the webkit-reviews mailing list