[webkit-reviews] review denied: [Bug 66143] [CSSRegions] RenderFlowThread layout should use the attached region sizes : [Attachment 104660] Patch V2

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Chiculita Alexandru
<achicu at adobe.com>'s request for review:
Bug 66143: [CSSRegions] RenderFlowThread layout should use the attached region
sizes
https://bugs.webkit.org/show_bug.cgi?id=66143

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

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


Just need some minor tweaks.

> Source/WebCore/rendering/RenderBlock.cpp:1229
>      LayoutStateMaintainer statePusher(view(), this, locationOffset(),
hasColumns() || hasTransform() || hasReflection() ||
style()->isFlippedBlocksWritingMode(), pageLogicalHeight,
pageLogicalHeightChanged, colInfo);
>  
> +    bool disableRegionFitting = view()->hasRenderFlowThread() &&
(hasColumns() || (isPositioned() && !isRenderFlowThread()) || isFloating());
> +    RegionFittingDisabler
regionFittingDisabler(view()->currentRenderFlowThread(), disableRegionFitting);


Pull view() into a local since it's being accessed three times here in quick
succession.

> Source/WebCore/rendering/RenderBlock.cpp:3576
> +    if (view()->hasRenderFlowThread()) {
> +	   RenderFlowThread* flowThread = view()->currentRenderFlowThread();
> +	   if (flowThread->isRegionFittingEnabled()) {
> +	       LayoutState* layoutState = view()->layoutState();
> +	       IntSize delta = layoutState->m_layoutOffset -
layoutState->m_pageOffset;
> +	       int offset = isHorizontalWritingMode() ? delta.height() :
delta.width();
> +	       LayoutUnit regionWidth =
flowThread->regionLogicalWidthForLine(offset + logicalTop);
> +	       right -= flowThread->logicalWidth() - regionWidth;
> +	   }
> +    }

Pull this into a helper function. Even a little static function would be nice.
I'm not sure if there's any compelling logic behind picking the right side to
shrink or if you just made an arbitrary call. It's cool that this gets lines
working, but we know long-term that this solution isn't going to work for block
splitting. I guess it's better than nothing though.

> Source/WebCore/rendering/RenderFlowThread.cpp:466
> +    // FIXME: The regions are always in order, optimize this search.

Interval tree might work here right?

> Source/WebCore/rendering/RenderFlowThread.h:108
> +    RenderRegion* renderRegionForLine(LayoutUnit logicalTop) const;

This isn't a "logicalTop" really. It's just a position that is examined. I'd
rename "logicalTop" to "position" since that matches the other "ForLine"
methods in RenderBlock.

> Source/WebCore/rendering/RenderRegion.cpp:89
> +    if (m_flowThread && isValid())
> +	   m_flowThread->invalidateRegions();

Can you go from valid to invalid? If so, then you would need an invalidate for
that also. I'm thinking you probably can't, but I figured I'd double-check. I'm
assuming going from valid to invalid can only happen across a reconstruction of
the RenderObject.