[webkit-reviews] review granted: [Bug 101862] [CSSRegions] Incorrect computed height for content with region-break-before : [Attachment 173514] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Mihnea Ovidenie
<mihnea at adobe.com>'s request for review:
Bug 101862: [CSSRegions] Incorrect computed height for content with
region-break-before
https://bugs.webkit.org/show_bug.cgi?id=101862

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

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=173514&action=review


r=me

> LayoutTests/ChangeLog:13
> +	   * fast/regions/autoheight-breakbefore-wrongheight-expected.html:
Added.
> +	   * fast/regions/autoheight-breakbefore-wrongheight.html: Added.

I would prefer this test to be a check-layout.js test as it would convey the
intent better, on top of being faster to run.

> Source/WebCore/rendering/RenderFlowThread.cpp:879
> +
> +	   // If the current offset if greater than the break offset, bail out
and skip the current region.
> +	   if (currentRegionOffsetInFlowThread >= offsetBreakInFlowThread) {
> +	       ++regionIter;
> +	       break;
> +	   }

Note that if you don't change the for loop's end condition, this could also be
pulled out of the loop and written:

if (regionIter != m_regionList.end())
   ++regionIter;

The upside of that being that it is guaranteed to run (even if you refactor the
inner loop), on top of being run only once instead of once per loop iteration.
The downside being that it's a bit less clear so I don't have a strong
preference for either form.