[webkit-reviews] review denied: [Bug 91097] [CSSRegions]Auto height is not working for regions : [Attachment 156074] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Mihnea Ovidenie
<mihnea at adobe.com>'s request for review:
Bug 91097: [CSSRegions]Auto height is not working for regions
https://bugs.webkit.org/show_bug.cgi?id=91097

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

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


You really have to split this patch. 76k is not something anyone can review
properly.

> LayoutTests/fast/regions/region-auto-height-01.html:7
> +	       #red { width: 50px; height: 50px; background-color: red; }

maybe factoring the width / height into a div selector would make it more
obvious what you are expecting.

> Source/WebCore/ChangeLog:35
> +		  fast/regions/region-auto-height-18.html

I hate such naming for tests. Unless you have written the tests, the difference
between 01 and 18 is unknown. When you have 2 - 3 tests, it's possible to look
at them and find the difference but when you have 18, not so much.

> Source/WebCore/rendering/RenderRegion.h:104
> +	       && style()->logicalTop().isSpecified()
> +	       && style()->logicalBottom().isSpecified();

Shouldn't you also check that the logical left or right are specified too?

> Source/WebCore/rendering/RenderRegion.h:117
> +    LayoutUnit computedAutoHeight() const { return m_computedAutoHeight; }
> +    void setComputedAutoHeight(LayoutUnit computedAutoHeight)
> +    {
> +	   m_computedAutoHeight = computedAutoHeight;
> +	   m_hasComputedAutoHeight = true;
> +    }

Couldn't this reuse the overrideLogicalHeight logic instead of re-implementing
it?

> Source/WebCore/rendering/RenderView.cpp:170
> +    if (inFirstLayoutPhaseAutoHeightRegions()) {

This is really a poor's man implementation of multi-phase layout. On top of
duplicating the code, it removes a lot of the safety that you usually get from
doing a full layout (see the ASSERT below for example). I am not supportive of
such a hack.

If you want to do multiple-phase layout then you should patch FrameView::layout
to actually call RenderView::layout several time as needed for regions. You can
also implement a proper state machine in RenderView in this case.

Btw, how does this code deal with subtree layout?

> Source/WebCore/rendering/RenderView.cpp:175
> +	   if (hasRenderNamedFlowThreads() && hasAutoHeightRegions())
> +	      
flowThreadController()->markAutoHeightRegionsForSecondLayoutPhase();

Globally calling setNeedsLayout during layout is a bad idea. Here you can
probably get away with it but I would rather that this get moved outside
layout.

> Source/WebCore/rendering/RenderView.h:191
> +    enum AutoHeightRegionsLayoutPhase { AutoHeightRegionsFirstLayoutPhase,
AutoHeightRegionsSecondLayoutPhase };
> +    bool inFirstLayoutPhaseAutoHeightRegions() const { return
m_autoHeightRegionsLayoutPhase == AutoHeightRegionsFirstLayoutPhase; }
> +    bool needsSecondPassLayoutForRegionsAutoHeight() const;

This is poorly named. first vs second layout don't give much information about
*what* you are doing. The first layout is unconstrained vs the second one is
properly breaking.

> Source/WebCore/rendering/RenderView.h:311
> +    AutoHeightRegionsLayoutPhase m_autoHeightRegionsLayoutPhase;
> +    unsigned m_autoHeightRegionsCount;

Shouldn't those be on the FlowThreadController and not on the RenderView?