Bug 69544 – [CSS3 Regions] Compute starting and ending regions for boxes and clamp to them.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dave Hyatt]

Compute starting and ending regions for boxes and clamp to them. This is because any descendants should be considered overflow out of the block, and so ultimately will render in the region range that the containing block begins/ends in.

[Dave Hyatt]

Created attachment 109983 [details]
Patch

[Dave Hyatt]

Comment on attachment 109983 [details]
Patch

Minusing. I left some code commented out.

[Dave Hyatt]

Created attachment 109985 [details]
Patch

[WebKit Review Bot]

Attachment 109983 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/rendering/RenderRegion.h:70:  The parameter name "box" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/rendering/RenderFlowThread.cpp:652:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/rendering/RenderFlowThread.cpp:686:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 109985 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/rendering/RenderRegion.h:70:  The parameter name "box" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 109985 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109985&action=review

> Source/WebCore/rendering/RenderBlock.cpp:1231
> +        setLogicalHeight(INT_MAX / 2); // FIXME: With the eventual refactoring of logical height computation to be region-aware, we can avoid this hack.

This is better written using numeric_limits<LayoutUnit>::max().

> Source/WebCore/rendering/RenderFlowThread.cpp:574
> +        if (region == endRegion)
> +            break;

Perhaps this should be part of the loop condition?

> Source/WebCore/rendering/RenderFlowThread.cpp:653
> +        if (region == endRegion)
> +            break;

Ditto

> Source/WebCore/rendering/RenderFlowThread.cpp:687
> +        if (region == endRegion)
> +            break;

Ditto

> Source/WebCore/rendering/RenderFlowThread.cpp:765
> +    RenderRegion* endRegion = box->contentLogicalHeight() < 0 ? lastRegion() : renderRegionForLine(offsetFromLogicalTopOfFirstPage + box->logicalHeight(), box, true);

As discussed on IRC, please remove the impossible < 0 case.

> Source/WebCore/rendering/RenderFlowThread.cpp:783
> +            if (region == range->endRegion())
> +                break;

Perhaps this should be part of the loop condition?

> Source/WebCore/rendering/RenderFlowThread.cpp:790
> +    m_regionRangeMap.set(box, range);

This isn’t super-hot code, so it might not make any practical difference, but the more efficient idiom for this is to do an add() at the beginning instead of a get(). Then you just set the new region into the iterator from add(), saving a hash lookup.

> Source/WebCore/rendering/RenderFlowThread.h:122
> +    void regionRangeForBox(const RenderBox*, RenderRegion*& startRegion, RenderRegion*& endRegion) const;

Perhaps this should be called getRegionRangeForBox, since it returns the result in out parameters.

> Source/WebCore/rendering/RenderFlowThread.h:171
> +    // A maps from RenderBox

A maps?

> Source/WebCore/rendering/RenderRegion.h:70
> +    void removeRenderBoxRegionInfo(const RenderBox* box);

You should drop the parameter name here.

[Dave Hyatt]

Fixed in r96842.