[webkit-reviews] review denied: [Bug 111176] [CSS Regions] position: fixed is computed relative to the first region, not the viewport : [Attachment 206094] Patch

[bugzilla-daemon at webkit.org]

Alexandru Chiculita <achicu at adobe.com> has denied Mihnea Ovidenie
<mihnea at adobe.com>'s request for review:
Bug 111176: [CSS Regions] position: fixed is computed relative to the first
region, not the viewport
https://bugs.webkit.org/show_bug.cgi?id=111176

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

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


Looks good, I have a couple of questions below.

Is there any other case that we need to patch in order to avoid the CSS Region
rules for the content of the fixed elements. For example flowThreadState() ==
RenderObject::InsideFlowThread will be true even if the element is inside the
fixed elements that are now collected by the RenderView. In that case you don't
have any flow thread layout state on the stack and it might have undefined
behavior.

> Source/WebCore/rendering/RenderBlock.cpp:7758
> +    LayoutState* layoutState = view()->layoutState();

nit: this variable seems to be unused.

> Source/WebCore/rendering/RenderBox.cpp:4602
>      if ((layoutState && !layoutState->isPaginated()) || (!layoutState &&
!flowThreadContainingBlock()))
> +	   return false;
> +    return true;

not: there's no need for the if statement. write just one return statement.

> Source/WebCore/rendering/RenderLayer.cpp:1359
> +	   layer = layer->parent();

This is based on the fact that the RenderView is the parent of the
RenderFlowThread. I think we need some asserts to catch these subtle
assumptions. We may need to change that in the future and asserts will help
finding such assumptions.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1787
> +	   && (layer->renderer()->flowThreadState() ==
RenderObject::NotInsideFlowThread ||
layer->renderer()->fixedPositionInNamedFlowWithViewContainingBlock());

What happens for the 3D elements nested in the "positioned: fixed" elements. I
think that those layers should have compositing enabled too.

> Source/WebCore/rendering/RenderLayerModelObject.cpp:136
> +			   layer()->dirtyStackingContainerZOrderLists();

Should we also dirty the receiving stacking container? Where is that done? Can
you add a comment to clarify it?

> Source/WebCore/rendering/RenderView.cpp:145
>	   flowThreadController()->layoutRenderNamedFlowThreads();
> +	   layoutPositionedObjects(false, true);

This doesn't work with nested flows. For example a fixed region inside a flow
will stay without a layout.