Bug 91097

Created attachment 156074 [details] Patch

Attachment 156074 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 LayoutTests/ChangeLog:10: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5] LayoutTests/ChangeLog:11: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5] LayoutTests/ChangeLog:12: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5] LayoutTests/ChangeLog:13: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5] Total errors found: 4 in 49 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 156074 [details] Patch 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?

Comment on attachment 156074 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156074&action=review >> 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? Also a comment about repainting, this will likely cause spurious repaintings due to the LayoutRepainter logic assuming a single phase layout (see for example bug 92800). This is currently an open problem in WebKit but you should be aware of it.

Comment on attachment 156074 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156074&action=review > Source/WebCore/rendering/RenderRegion.h:102 > + bool hasAnchoredTopAndBottom = (isOutOfFlowPositioned() || isRelPositioned()) You should remove isRelPositioned from this. You only should be checking isOutOfFlowPositioned, and as Julien says, in vertical writing mode left/right could be the ones giving you height.

Comment on attachment 156074 [details] Patch I disagree with Julien that this needs to be outside the RenderObjects. This two-pass "auto height regions including breaks" algorithm does need to originate at renderers. Don't think of it as some global multi-pass layout algorithm. Note that pagination already has numerous multi-pass layout systems right in the renderers, so I don't think pulling this outside of the RenderObject tree is what we want to do, especially given it has to work originating at multi-column blocks for the new multi-column layout. I think it's pretty much fine as is, although I do agree that the two phases should be more descriptive and elaborate on what they are actually doing.

Support for auto-height regions has landed in trunk, i have added the relevant bugs as dependencies.