[Webkit-unassigned] [Bug 66143] [CSSRegions] RenderFlowThread layout should use the attached region sizes

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=66143


Dave Hyatt <hyatt at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #104660|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #3 from Dave Hyatt <hyatt at apple.com>  2011-08-22 12:08:39 PST ---
(From update of attachment 104660)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.