Bug 69544

Summary: [CSS3 Regions] Compute starting and ending regions for boxes and clamp to them.
Product: WebKit Reporter: Dave Hyatt <hyatt>
Component: Layout and RenderingAssignee: Dave Hyatt <hyatt>
Status: RESOLVED FIXED    
Severity: Normal CC: mihnea, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 57312    
Attachments:
Description Flags
Patch
hyatt: review-
Patch mitz: review+

Description Dave Hyatt 2011-10-06 11:24:03 PDT
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.
Comment 1 Dave Hyatt 2011-10-06 11:27:43 PDT
Created attachment 109983 [details]
Patch
Comment 2 Dave Hyatt 2011-10-06 11:29:15 PDT
Comment on attachment 109983 [details]
Patch

Minusing. I left some code commented out.
Comment 3 Dave Hyatt 2011-10-06 11:32:07 PDT
Created attachment 109985 [details]
Patch
Comment 4 WebKit Review Bot 2011-10-06 11:34:36 PDT
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.
Comment 5 WebKit Review Bot 2011-10-06 11:37:29 PDT
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 6 mitz 2011-10-06 11:48:26 PDT
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.
Comment 7 Dave Hyatt 2011-10-06 12:03:01 PDT
Fixed in r96842.