[webkit-reviews] review denied: [Bug 66198] [CSSRegions] Regions should not slice line box rendering : [Attachment 105971] Patch V4

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Chiculita Alexandru
<achicu at adobe.com>'s request for review:
Bug 66198: [CSSRegions] Regions should not slice line box rendering
https://bugs.webkit.org/show_bug.cgi?id=66198

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

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


> Source/WebCore/rendering/RenderBlock.cpp:2085
> +	       if (!child->selfNeedsLayout() && view()->hasRenderFlowThread())
> +		   child->markForFlowThreadRelayoutIfNeeded();

Why selfNeedsLayout and not needsLayout?

> Source/WebCore/rendering/RenderBlock.cpp:2258
> +	   if (!r->selfNeedsLayout() && view()->hasRenderFlowThread())
> +	       r->markForFlowThreadRelayoutIfNeeded();

Why selfNeedsLayout and not needsLayout?

> Source/WebCore/rendering/RenderBlock.cpp:2301
> +void RenderBlock::markForFlowThreadRelayoutIfNeeded()
> +{
> +    ASSERT(!selfNeedsLayout() && view()->hasRenderFlowThread());
> +    // FIXME: We can do better here, we can detect the cases when the change
of the logical top
> +    // is not actually doing any harm on the lines. Until that time, we will
force a full relayout here.
> +    // Flow threads are different from the columns pagination because
currently there's no way to detect a line that
> +    // moves from one region to the other in
RenderBlock::determineStartPosition. The width might change in that cases
> +    // and we wouldn't catch it. For columns, the only reason the width may
change are new floats, but floats are cached
> +    // on RootInlineBox which make the structure a litle bigger.
> +    if (view()->layoutState()->pageLogicalOffset(logicalTop()) !=
pageLogicalOffset())
> +	   setNeedsLayout(true, false);
> +}

Don't get why this is needed at all. Why isn't
markForPaginationRelayoutIfNeeded sufficient?

> Source/WebCore/rendering/RenderBlock.cpp:3409
> +	       if (!childBox->selfNeedsLayout() &&
view()->hasRenderFlowThread())
> +		   childBox->markForFlowThreadRelayoutIfNeeded();

Same question here.

> Source/WebCore/rendering/RenderBlock.cpp:6115
> +    LayoutUnit pageLogicalHeight = pageLogicalHeightForLine(logicalOffset);

"line" isn't the right term to use here. Again it's just a position or offset.
pageLogicalHeightForOffset would be better.

> Source/WebCore/rendering/RenderBlock.cpp:6120
> +    LayoutUnit remainingLogicalHeight =
pageRemainingLogicalHeightForLine(logicalOffset);

Same quibble here. Just using an offset. No lines even have to be involved.

> Source/WebCore/rendering/RenderBlock.cpp:6187
> +bool RenderBlock::isPaginated() const
> +{
> +    const LayoutState* layoutState = view()->layoutState();
> +    return view()->hasRenderFlowThread() || (layoutState &&
layoutState->isPaginated());
> +}

Why not just set isPaginated to true in the layout state when laying out the
children of the RenderFlowThread? That will be faster than doing two checks
every time.

> Source/WebCore/rendering/RenderBlock.cpp:6193
> +bool RenderBlock::hasFixedPageHeight() const
> +{
> +    const LayoutState* layoutState = view()->layoutState();
> +    return view()->hasRenderFlowThread() || (layoutState &&
layoutState->pageLogicalHeight());
> +}

I kind of see why you can't reuse pageLogicalHeight() since it's variable, so I
guess this is ok. I still dislike having two checks every time.

> Source/WebCore/rendering/RenderBlock.cpp:6289
> +	   // FIXME: For regions we cannot use pagination strut, because the
region width might change and the line
> +	   // would need to be recomputed anyway. If the delta is 0 the line
would not be recomputed.
> +	   if (lineBox == firstRootBox() && totalLogicalHeight <
pageLogicalHeight && !isPositioned() && !isTableCell() &&
!renderView->hasRenderFlowThread())

This is a problem with all line pagination and not just with regions. I'd
prefer it if you take the hasRenderFlowThread() check out here and we can file
a bug on improving line layout when you paginate to a position with a different
available line width.

You can reproduce the same problem with floats.

> Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:375
> +	       if (!child->selfNeedsLayout() && view()->hasRenderFlowThread())
> +		   child->markForFlowThreadRelayoutIfNeeded();

Again, confused over all these additional checks. I would think
markForPaginationRelayoutIfNeeded would be enough.

> Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:439
> +	       if (!child->selfNeedsLayout() && view()->hasRenderFlowThread())
> +		   child->markForFlowThreadRelayoutIfNeeded();

Ditto.

> Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:671
> +	       if (!child->selfNeedsLayout() && view()->hasRenderFlowThread())
> +		   child->markForFlowThreadRelayoutIfNeeded();

Ditto.