[webkit-reviews] review granted: [Bug 69544] [CSS3 Regions] Compute starting and ending regions for boxes and clamp to them. : [Attachment 109985] Patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org has granted Dave Hyatt <hyatt at apple.com>'s request for review:
Bug 69544: [CSS3 Regions] Compute starting and ending regions for boxes and
clamp to them.
https://bugs.webkit.org/show_bug.cgi?id=69544

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

------- Additional Comments from mitz at webkit.org
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.