[webkit-reviews] review canceled: [Bug 97533] [CSSRegions]Add support for auto-height regions (without region-breaks) : [Attachment 165605] 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 97533: [CSSRegions]Add support for auto-height regions (without
region-breaks)
https://bugs.webkit.org/show_bug.cgi?id=97533

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

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


> Source/WebCore/rendering/FlowThreadController.cpp:179
> +bool FlowThreadController::resetRegionsOverrideLogicalContentHeight()

Not a huge fan of reset functions returning some bool parameter. I don't
understand why FlowThreadController::hasAutoLogicalHeightRegions() is not
enough as you check it in RenderView::layout().

> Source/WebCore/rendering/FlowThreadController.cpp:185
> +	   if (flowRenderer->resetRegionsOverrideLogicalContentHeight())
> +	       hadFlowsWithAutoLogicalHeightRegions = true;

Maybe better written:

hadFlowsWithAutoLogicalHeightRegions |=
flowRenderer->resetRegionsOverrideLogicalContentHeight();

(removes the need for the branch).

> Source/WebCore/rendering/RenderFlowThread.cpp:226
> +	   if (region->needsOverrideLogicalContentHeightComputation())
> +	       // If we have an auto logical height region for which we did not
compute a height yet,
> +	       // then we cannot compute and update the height of this flow.
> +	       return;

Please put curly braces around multi-line if () as per our style guide.

> Source/WebCore/rendering/RenderFlowThread.cpp:812
> +	   region->setNeedsLayout(true);

Please use the following instead:

region->setNeedsLayout(true, MarkOnlyThis)

Calling setNeedsLayout(true, MarkContainingBlockChain) during layout is a
programming mistake that we should ASSERT against (but that's another debate).
Here I think it is not too bad but I would rather avoid anti-patterns.

> Source/WebCore/rendering/RenderFlowThread.cpp:825
> +	   region->setNeedsLayout(true);

Ditto.

> Source/WebCore/rendering/RenderView.cpp:185
>      if (needsLayout()) {

I would really push this needsLayout() call above and make it an early return
(before we initialize the LayoutState but after the ASSERT).

> Source/WebCore/rendering/RenderView.cpp:199
> +	   m_layoutPhase = RenderViewNormalLayout;
> +	   setNeedsLayout(false);

This is confusing to reset the state to the normal layout for the second phase
and doesn't make much sense.

Reading the code, you would assume the setNeedsLayout(false) will trigger an
ASSERT in layoutContent but no due to marking below. AFAICT, you *know* you
should have at least one region to lay out.

> Source/WebCore/rendering/RenderView.h:191
> +    enum RenderViewLayoutPhase { RenderViewNormalLayout,
UnconstrainedFlowThreadsLayoutInAutoLogicalHeightRegions };

You are assuming that we always do a normal layout and may do a
pre-unconstrained flow thread layout. You could just do an unconstrained phase
first (regardless of whether there is any auto-height regions) and then if you
had auto-height region do a follow-up constrained phase. I would say this would
be more readable.