[webkit-reviews] review granted: [Bug 96267] [CSSRegions]Flag auto-height regions : [Attachment 164389] Patch 2

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Mihnea Ovidenie
<mihnea at adobe.com>'s request for review:
Bug 96267: [CSSRegions]Flag auto-height regions
https://bugs.webkit.org/show_bug.cgi?id=96267

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

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


> LayoutTests/ChangeLog:14
> +	   * platform/mac/fast/regions/autoheight-regions-mark-expected.png:
Added.
> +	   * platform/mac/fast/regions/autoheight-regions-mark-expected.txt:
Added.

Unless I am mistaking, those baselines are cross-platform and could be moved
next to the test case.

> Source/WebCore/rendering/FlowThreadController.cpp:104
> +#ifndef NDEBUG
> +    checkAutoLogicalHeightRegions();
> +#endif

An alternative to that would be to change checkAutoLogicalHeightRegions to
isAutoLogicalHeightRegionsFlagConsistent and make it return a boolean so that
you can just do:

ASSERT(isAutoLogicalHeightRegionsFlagConsistent());

That would be my preferred way.

> Source/WebCore/rendering/RenderRegion.cpp:206
> +    checkRegionHavingAutoLogicalHeight();

Checking is more about a 'check' (for example your check in RenderFlowThread),
not really an 'update'. I would name it updateRegionHasAutoLogicalHeightFlag,
recomputeRegionAutoLogicalHeightFlag or similar.

> Source/WebCore/rendering/RenderTreeAsText.cpp:698
> +		       if (renderRegion->hasAutoLogicalHeight())
> +			   ts << " hasAutoLogicalHeight";

This really looks like it should be in RenderRegion::renderName(). As this is
the existing pattern, I am not asking to change it now but you should consider
that.