[webkit-reviews] review denied: [Bug 115456] [CSS Regions][CSS Exclusions] Shape-inside on regions should respect region borders and paddings : [Attachment 200950] proposed patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Zoltan Horvath <zoltan at webkit.org>'s
request for review:
Bug 115456: [CSS Regions][CSS Exclusions] Shape-inside on regions should
respect region borders and paddings
https://bugs.webkit.org/show_bug.cgi?id=115456

Attachment 200950: proposed patch
https://bugs.webkit.org/attachment.cgi?id=200950&action=review

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


Would be nice as this code grows to get it out of layoutRunsAndFloatsInRange
and into helpers.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1716
>  #if ENABLE(CSS_EXCLUSIONS)
> -	   // FIXME: Bug 95361: It is possible for a line to grow beyond
lineHeight, in which
> -	   // case these segments may be incorrect.
> -	   if (layoutState.flowThread())
> +	   RenderRegion* currentRegion = regionAtBlockOffset(logicalHeight());
> +	   if (currentRegion)
>	       exclusionShapeInsideInfo = layoutExclusionShapeInsideInfo();
>	   if (exclusionShapeInsideInfo) {
>	       LayoutUnit lineTop = logicalHeight() + absoluteLogicalTop;
> -	       exclusionShapeInsideInfo->computeSegmentsForLine(lineTop,
lineHeight(layoutState.lineInfo().isFirstLine(), isHorizontalWritingMode() ?
HorizontalLine : VerticalLine, PositionOfInteriorLineBoxes));
> +	       LayoutUnit lineHeight =
this->lineHeight(layoutState.lineInfo().isFirstLine(),
isHorizontalWritingMode() ? HorizontalLine : VerticalLine,
PositionOfInteriorLineBoxes);
> +
> +	       if (layoutState.flowThread()) {
> +		   RenderRegion* nextRegion =
regionAtBlockOffset(logicalHeight() + lineHeight - LayoutUnit(1));
> +		   lineTop +=
exclusionShapeInsideInfo->owner()->borderAndPaddingBefore();
> +		   // Content in a flow thread is relative to the beginning of
the thread, but the shape calculation should be relative to the current region.

> +		   if (currentRegion == nextRegion)
> +		       lineTop -=
currentRegion->logicalTopForFlowThreadContent();
> +	       }
> +
> +	       // FIXME: Bug 95361: It is possible for a line to grow beyond
lineHeight, in which case these segments may be incorrect.
> +	       exclusionShapeInsideInfo->computeSegmentsForLine(lineTop,
lineHeight);

Can we get this shifted into a helper function? It seems sizable enough that
getting it out of layoutRunsAndFloatsInRange and into a helper would be nice.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:1746
>  #if ENABLE(CSS_EXCLUSIONS)
>	   if (LayoutUnit adjustedLogicalLineTop =
adjustLogicalLineTop(exclusionShapeInsideInfo, resolver.position(), end,
wordMeasurements)) {
> -	       end = restartLayoutRunsAndFloatsInRange(logicalHeight(),
adjustedLogicalLineTop - absoluteLogicalTop, lastFloatFromPreviousLine,
resolver, oldEnd);
> +	       LayoutUnit newLogicalHeight = adjustedLogicalLineTop -
absoluteLogicalTop;
> +	       if (layoutState.flowThread())
> +		   newLogicalHeight -=
currentRegion->logicalTopForFlowThreadContent();
> +	       end = restartLayoutRunsAndFloatsInRange(logicalHeight(),
newLogicalHeight, lastFloatFromPreviousLine, resolver, oldEnd);
>	       continue;
>	   }
>  #endif

Seems like this could be shifted into a helper too... maybe combined with or
rolled into adjustLogicalLineTop.

if (helperFunction()) continue;