[webkit-reviews] review canceled: [Bug 96267] [CSSRegions]Flag auto-height regions : [Attachment 163592] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled 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 163592: Patch
https://bugs.webkit.org/attachment.cgi?id=163592&action=review

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


The patch doesn't apply on ToT. I would like to see a revised version but the
approach looks OK.

>> LayoutTests/platform/chromium/TestExpectations:2503
>> +BUGWK96267 SKIP : fast/regions/autoheight-regions-mark.html = FAIL
> 
> Unsupported expectation: FAIL  [test/expectations] [5]

I think you just put PASS instead of FAIL as you are SKIP'ping them.

> Source/WebCore/ChangeLog:11
> +	   - has height: auto and is part of the normal flow
> +	   - has height: auto, is not part of normal flow and does not have
top/bottom specified

Let's use precise language here. You are doing the right thing in the code by
looking at the *logical* height and *logical* top/bottom but this ChangeLog
could lead people to think otherwise.

> Source/WebCore/rendering/RenderRegion.cpp:304
> +    if (isValid()) {

Nit: Early return are preferred.

> Source/WebCore/rendering/RenderRegion.h:118
> +    bool usesAutoHeight() const { return m_usesAutoHeight; }

Not a huge fan of the naming, first it should be usesAutoLogicalHeight. But the
whole hasAutoHeightStyle vs usesAutoHeight is not super clear to me.

In RenderLayer, we use a pattern of should* / is* (shouldBeNormalFlowOnly /
isNormalFlowOnly, shouldBeSelfPaintingLayer / isSelfPaintingLayer). It would
fit nicely here.

> Source/WebCore/rendering/RenderRegion.h:195
> +    bool m_usesAutoHeight;

May be better to move it with the other booleans above.

> Source/WebCore/rendering/RenderView.cpp:160
> +#ifndef NDEBUG
> +	   if (m_flowThreadController)
> +	       m_flowThreadController->checkAutoHeightRegions();
> +#endif

Pushing that into layoutRenderNamedFlowThreads() would have made more sense but
that means losing some coverage as the check wouldn't be run if you have no
named flow thread. This could easily be fixed by moving the count check for
named flow thread into layoutRenderNamedFlowThreads.