Bug 103738

Created attachment 179607 [details] Patch

I will follow up with another patch for other properties that are in the same situation, for instance the width of the auto-height regions.

Comment on attachment 179607 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179607&action=review > Source/WebCore/rendering/RenderRegion.cpp:214 > + if (m_hasAutoLogicalHeight) { I don't like that we need to check for these specific properties here. There might be other properties that have the same issue and I would like to see a more generic fix.

Created attachment 180129 [details] Patch 2

Comment on attachment 180129 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=180129&action=review Looks a lot better, but I think you just detected a special case. > Source/WebCore/rendering/RenderFlowThread.cpp:146 > + if (view()->flowThreadController()->hasAutoLogicalHeightRegions() && view()->normalLayoutPhase()) Looks like "view()->flowThreadController()" is used many times in the RenderFlowThread, I think we could promote it to an inline function like RenderFlowThread::controller(). > Source/WebCore/rendering/RenderRegion.cpp:220 > + if (m_hasAutoLogicalHeight && view()->normalLayoutPhase()) Let me just go through the current state: 1. We didn't know that the RenderFlowThread needed a layout, so we didn't reset the RenderRegion's auto-heights and went directly into the "second-pass". 2. needsTwoPassLayoutForAutoLogicalHeightRegions flag in RenderView::layout set to false, so there's no second pass. 3. So we do "normal-layout", but finally notice that a RenderRegion actually needs layout. 4. It means, we would like to fallback to the "first-pass" of the layout. 5. But, it might be too late. That's because other auto-height RenderRegions already used their computed height in this "first-pass" and those heights might influence the size of other RenderRegions that used percentages for their height. That would make the layout unstable/unpredictable. Also, reseting the height here has no meaning if needsTwoPassLayoutForAutoLogicalHeightRegions was false, because we will not force a second layout. I think you could add an assert at the end of RenderView::layout to catch cases when RenderRegions with auto-height still have their heights "cleared" after the final pass. In some cases there is no way to detect whether a RenderRegion actually needs layout. That's because a parent might force the layout, so we need to wait until RenderRegion::layout is called in order to actually detect such a case. That means we have two options: 1. Always reset the auto-height render regions. This will force us to make a full-two-passes-layout every time, even though there might be no need to do it. 2. Introduce a middle pass. #2 above would be like this: 1. Try to detect changes of the RenderFlowThread that might need to reset the heights: - ie. any RenderRegion or any RenderFlowThread needed layout. 2. If we couldn't "early" detect anything wrong with the RenderFlowThread, just do a "simple" layout, reusing the computed heights from the previous layout. In most cases this will finish without touching RenderRegions::layout. 3. If we detect a change of the flow, just reset all the computed RenderRegion heights and do the two-passes as usual. 4. If we went with the simple layout, but it still entered RenderRegions::layout and changed it's box we would need to fallback and go through the full-two-passes-layout. That could be done by repeating the layout after we reset all the regions heights. We need to analyse from a performance point of view whether early catching the "needsLayout" on the RenderRegions is actually useful or not in practice. We could either (1) make FlowThreadController::hasRenderNamedFlowThreadsNeedingLayout look for RenderRegions that need layout and avoid going into a layout that we know for sure is not going to end up well or (2) we could always wait until RenderRegion::layout to actually detect if the width/height changed for some reason and force a middle layout. Note that both (1) is not eliminating the need for (2) as there might be other reasons a RenderRegion hits it's layout functions. It might be that a parent of the RenderRegion resized and needs to relayout it and we could not catch that ahead of time. Only doing (2) from above has the advantage that simple changes in layout like "border size" would simply do a quick one-pass layout instead.

Created attachment 182267 [details] Additional test Test showing that patch2 does not catch all situations

Created attachment 182815 [details] Patch 3

Comment on attachment 182815 [details] Patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=182815&action=review Looks good! > LayoutTests/fast/regions/autoheight-region-remove-minheight-expected.txt:3 > +On success you should not see FAIL below. I think you could remove this line. It made me think the test actually failed. > Source/WebCore/rendering/RenderView.cpp:194 > m_layoutPhase = RenderViewNormalLayout; What about extracting the method and adding more comments in here. Here's a quick example. if (hasRenderNamedFlowThreads() && m_flowThreadController->hasAutoLogicalHeightRegions()) layoutContentWithAutoHeightRegions(state); else layoutContent(state); RenderView::layoutContentWithAutoHeightRegions() { { if (!flowThreadController()->hasRenderNamedFlowThreadsNeedingLayout()) { // If we couldn't detect any flow or regions changes, try to just do a quick "second phase" layout. m_layoutPhase = ConstrainedFlowThreadsLayoutInAutoLogicalHeightRegions; layoutContent(state); // There might be cases when the initial assumption was wrong and some regions actually got a new layout. At this point // we have to reset the auto-height regions and start the two phases layout. if (!flowThreadController()->needsTwoPassLayoutForAutoHeightRegions()) return; } flowThreadController()->setNeedsTwoPassLayoutForAutoHeightRegions(false); // Compute the sizes of the regions considering auto-height regions to have 0-height by default. m_layoutPhase = RenderViewNormalLayout; flowThreadController()->resetRegionsOverrideLogicalContentHeight(); layoutContent(state); // At this point the flow thread calculated the best break points for the auto-height regions. We will // do another layout with all the auto-height regions heights fixed to the previous computed sizes. m_layoutPhase = ConstrainedFlowThreadsLayoutInAutoLogicalHeightRegions; flowThreadController()->markAutoLogicalHeightRegionsForLayout(); layoutContent(state); ASSERT(!flowThreadController()->needsTwoPassLayoutForAutoHeightRegions()); }

Created attachment 183466 [details] Patch 4

(In reply to comment #8) > (From update of attachment 182815 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=182815&action=review > > Looks good! > > > LayoutTests/fast/regions/autoheight-region-remove-minheight-expected.txt:3 > > +On success you should not see FAIL below. > > I think you could remove this line. It made me think the test actually failed. Changed all the test to "should see PASS" instead of "should not see FAIL". > > > Source/WebCore/rendering/RenderView.cpp:194 > > m_layoutPhase = RenderViewNormalLayout; > > What about extracting the method and adding more comments in here. Here's a quick example. > > if (hasRenderNamedFlowThreads() && m_flowThreadController->hasAutoLogicalHeightRegions()) > layoutContentWithAutoHeightRegions(state); > else > layoutContent(state); > > > RenderView::layoutContentWithAutoHeightRegions() { > { > if (!flowThreadController()->hasRenderNamedFlowThreadsNeedingLayout()) { > // If we couldn't detect any flow or regions changes, try to just do a quick "second phase" layout. > m_layoutPhase = ConstrainedFlowThreadsLayoutInAutoLogicalHeightRegions; > layoutContent(state); > // There might be cases when the initial assumption was wrong and some regions actually got a new layout. At this point > // we have to reset the auto-height regions and start the two phases layout. > if (!flowThreadController()->needsTwoPassLayoutForAutoHeightRegions()) > return; > } > > flowThreadController()->setNeedsTwoPassLayoutForAutoHeightRegions(false); > > // Compute the sizes of the regions considering auto-height regions to have 0-height by default. > m_layoutPhase = RenderViewNormalLayout; > flowThreadController()->resetRegionsOverrideLogicalContentHeight(); > layoutContent(state); > > // At this point the flow thread calculated the best break points for the auto-height regions. We will > // do another layout with all the auto-height regions heights fixed to the previous computed sizes. > m_layoutPhase = ConstrainedFlowThreadsLayoutInAutoLogicalHeightRegions; > flowThreadController()->markAutoLogicalHeightRegionsForLayout(); > layoutContent(state); > > ASSERT(!flowThreadController()->needsTwoPassLayoutForAutoHeightRegions()); > } Done, i created a new method RenderView::layoutContentInAutoLogicalHeightRegions() to do that.

Comment on attachment 183466 [details] Patch 4 r=me

Created attachment 183922 [details] Patch for landing

Comment on attachment 183922 [details] Patch for landing Clearing flags on attachment: 183922 Committed r140400: <http://trac.webkit.org/changeset/140400>

All reviewed patches have been landed. Closing bug.