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

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has granted 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 210123: Patch
https://bugs.webkit.org/attachment.cgi?id=210123&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=210123&action=review


r=me, but fix all the style queue errors. I also have a few suggestions below.

> Source/WebCore/rendering/RenderBox.cpp:1847
> +    // For fixed positioned elements inside out-of-flow named flows, we do
not want to
> +    // map their position further to regions based on their coordinates
inside the named flows.
> +    if (o->isOutOfFlowRenderFlowThread() &&
fixedPositionedWithNamedFlowContainingBlock())
> +	   o->mapLocalToContainer(toRenderLayerModelObject(o), transformState,
mode, wasFixed);
> +    else
> +	   o->mapLocalToContainer(repaintContainer, transformState, mode,
wasFixed);

I would invert this to give precedence to the non-flow-thread code path.

if (!o->isOutOfFlowRenderFlowThread() ||
!fixedPositionedWithNamedFlowContainingBlock())
   // Do the common thing. :)

> Source/WebCore/rendering/RenderBox.cpp:2975
> +	   if (containingBlock->isRenderNamedFlowThread() &&
(style()->position() == FixedPosition))

No need for parentheses around (style()->position() == FixedPosition())

> Source/WebCore/rendering/RenderBox.cpp:3033
> +	       if (containingBlock->isRenderNamedFlowThread() &&
(style()->position() == FixedPosition))

Ditto.

>> Source/WebCore/rendering/RenderLayer.cpp:4547
>> +	    RenderLayer* hitLayer =
fixedLayer->hitTestLayer(fixedLayer->renderer().flowThreadContainingBlock()->la
yer(), 0, request, tempResult,
>> +			hitTestRect, hitTestLocation, false, transformState,
zOffsetForDescendants);
> 
> When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]

Fix the funny indentation here.