Bug 97533

Created attachment 165605 [details] Patch

Attachment 165605 [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 Source/WebCore/rendering/RenderView.cpp:149: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1 in 29 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 165605 [details] Patch 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.

Created attachment 166835 [details] Patch 2

(In reply to comment #3) > (From update of attachment 165605 [details]) > 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(). > You are right, resetRegionsOverrideLogicalContentHeight has void return type. > > 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. > Done. > > 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. > Added a FIXME in the code and call region->setNeedsLayout(true) for now. > > Source/WebCore/rendering/RenderFlowThread.cpp:825 > > + region->setNeedsLayout(true); > > Ditto. > Added a FIXME in the code and call region->setNeedsLayout(true) for now. > > 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). > Done. > > 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. > I have removed the call to setNeedsLayout(false). > > 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. Ok, now the 2 phases of layout are { RenderViewNormalLayout, ConstrainedFlowThreadsLayoutInAutoLogicalHeightRegions }. RenderViewNormalLayout is done regardless we have auto logical height regions, the second one is done only when we have auto logical height regions and we need a second pass.

Comment on attachment 166835 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=166835&action=review > LayoutTests/fast/regions/autoheight-horizontal-bt.html:26 > + aaaa aaaa aaaa Shouldn't there be 3 repetitions of 5 'a' to have 3 lines of 200px each? > LayoutTests/fast/regions/webkit-named-flow-first-empty-region-index.html:11 > - #region, #region2, region3 {width: 250px; height: 50px} > + #region, #region2, #region3 {width: 250px; height: 50px; } Was the test passing before? > Source/WebCore/rendering/FlowThreadController.cpp:183 > + RenderNamedFlowThread* flowRenderer = *iter; > + flowRenderer->resetRegionsOverrideLogicalContentHeight(); No need for the extra variable here: (*iter)->resetRegionsOverrideLogicalContentHeight() > Source/WebCore/rendering/FlowThreadController.cpp:191 > + RenderNamedFlowThread* flowRenderer = *iter; > + flowRenderer->markAutoLogicalHeightRegionsForLayout(); Ditto. > Source/WebCore/rendering/RenderFlowThread.cpp:637 > + bool useHorizontalWritingMode = isHorizontalWritingMode(); Unneeded change, also the variable name is worse than the API name. You *are* in an horizontal writing-mode, you don't *use* it. > Source/WebCore/rendering/RenderFlowThread.cpp:839 > + if (region->hasOverrideHeight() && view()->normalLayoutPhase()) { > + regionLogicalHeight = region->overrideLogicalContentHeight(); > + regionRect.setHeight(regionLogicalHeight); > + } Are you ignoring the overridden logical content height for the constrained phase as it is already included into the logicalHeight? Couldn't you ignore the overridden logical content height for the normal phase and properly update your flow-thread in the follow-up phase? > Source/WebCore/rendering/RenderFlowThread.cpp:864 > + bool useHorizontalWritingMode = isHorizontalWritingMode(); Same comment about the variable here. > Source/WebCore/rendering/RenderFlowThread.cpp:882 > + // Mark all the remaining auto logical height regions with 0 override computed height > + for (; regionIter != m_regionList.end(); ++regionIter) { > + RenderRegion* currRegion = *regionIter; > + if (!currRegion->isValid() || (currRegion == region)) > + continue; > + currRegion->setOverrideLogicalContentHeight(ZERO_LAYOUT_UNIT); > + } You may as well just clearOverrideSize() to avoid a hash lookup and extra-operations later (or better add a clearOverrideLogicalContentHeight()) > Source/WebCore/rendering/RenderRegion.cpp:593 > + if (view()->normalLayoutPhase()) > + return; > + > + if (hasAutoLogicalHeight() && hasOverrideHeight()) { Wouldn't the second check cover the first one as we only set the override height in computeOverflow, ie at the end of the normal layout phase. > Source/WebCore/rendering/RenderView.cpp:202 > + m_layoutPhase = RenderViewNormalLayout; The logic is really backwards here (not that it doesn't work, just that it opens a hole for potential badness). It would be better to always reset m_layoutPhase before calling the first layoutContent. > Source/WebCore/rendering/RenderView.h:191 > + enum RenderViewLayoutPhase { RenderViewNormalLayout, ConstrainedFlowThreadsLayoutInAutoLogicalHeightRegions }; Not super happy with the "normal" layout vs ("abnormal") constrained layout phase, I could live with it though as I couldn't find a better way to put it.

(In reply to comment #6) > (From update of attachment 166835 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=166835&action=review > > > LayoutTests/fast/regions/autoheight-horizontal-bt.html:26 > > + aaaa aaaa aaaa > > Shouldn't there be 3 repetitions of 5 'a' to have 3 lines of 200px each? > No, if i have a line with 5'a', that line would have 250px. I have changed the test to make it more clear. > > LayoutTests/fast/regions/webkit-named-flow-first-empty-region-index.html:11 > > - #region, #region2, region3 {width: 250px; height: 50px} > > + #region, #region2, #region3 {width: 250px; height: 50px; } > > Was the test passing before? > YES. When debugging the patch, i noticed that this test was having an auto-logical height region, which was obviously not the intent here, so i decided to change the test. > > Source/WebCore/rendering/FlowThreadController.cpp:183 > > + RenderNamedFlowThread* flowRenderer = *iter; > > + flowRenderer->resetRegionsOverrideLogicalContentHeight(); > > No need for the extra variable here: (*iter)->resetRegionsOverrideLogicalContentHeight() > OK. > > Source/WebCore/rendering/FlowThreadController.cpp:191 > > + RenderNamedFlowThread* flowRenderer = *iter; > > + flowRenderer->markAutoLogicalHeightRegionsForLayout(); > > Ditto. > OK. > > Source/WebCore/rendering/RenderFlowThread.cpp:637 > > + bool useHorizontalWritingMode = isHorizontalWritingMode(); > > Unneeded change, also the variable name is worse than the API name. You *are* in an horizontal writing-mode, you don't *use* it. > Agree. > > Source/WebCore/rendering/RenderFlowThread.cpp:839 > > + if (region->hasOverrideHeight() && view()->normalLayoutPhase()) { > > + regionLogicalHeight = region->overrideLogicalContentHeight(); > > + regionRect.setHeight(regionLogicalHeight); > > + } > > Are you ignoring the overridden logical content height for the constrained phase as it is already included into the logicalHeight? > Yes. > Couldn't you ignore the overridden logical content height for the normal phase and properly update your flow-thread in the follow-up phase? > Only for this patch, because i am not processing breaks. If it take a break into account and call addForcedBreak from RenderBlock::applyBeforeBreak/applyAfterBreak (not only from RenderFlowThread::computeOverflowStateForRegions), then i need to update the flow thread region rects so that the following auto logical height regions will have a chance to properly paginate flow thread content. > > Source/WebCore/rendering/RenderFlowThread.cpp:864 > > + bool useHorizontalWritingMode = isHorizontalWritingMode(); > > Same comment about the variable here. > > > Source/WebCore/rendering/RenderFlowThread.cpp:882 > > + // Mark all the remaining auto logical height regions with 0 override computed height > > + for (; regionIter != m_regionList.end(); ++regionIter) { > > + RenderRegion* currRegion = *regionIter; > > + if (!currRegion->isValid() || (currRegion == region)) > > + continue; > > + currRegion->setOverrideLogicalContentHeight(ZERO_LAYOUT_UNIT); > > + } > > You may as well just clearOverrideSize() to avoid a hash lookup and extra-operations later (or better add a clearOverrideLogicalContentHeight()) > I will add a new function, clearOverrideLogicalContentHeight for this. > > Source/WebCore/rendering/RenderRegion.cpp:593 > > + if (view()->normalLayoutPhase()) > > + return; > > + > > + if (hasAutoLogicalHeight() && hasOverrideHeight()) { > > Wouldn't the second check cover the first one as we only set the override height in computeOverflow, ie at the end of the normal layout phase. > > > Source/WebCore/rendering/RenderView.cpp:202 > > + m_layoutPhase = RenderViewNormalLayout; > > The logic is really backwards here (not that it doesn't work, just that it opens a hole for potential badness). It would be better to always reset m_layoutPhase before calling the first layoutContent. > OK > > Source/WebCore/rendering/RenderView.h:191 > > + enum RenderViewLayoutPhase { RenderViewNormalLayout, ConstrainedFlowThreadsLayoutInAutoLogicalHeightRegions }; > > Not super happy with the "normal" layout vs ("abnormal") constrained layout phase, I could live with it though as I couldn't find a better way to put it. I am not happy either with the name, i thought for a better name but without much luck. Patch coming.

Created attachment 167535 [details] Patch 3

Comment on attachment 167535 [details] Patch 3 View in context: https://bugs.webkit.org/attachment.cgi?id=167535&action=review > Source/WebCore/ChangeLog:47 > + (WebCore): Let's remove those useless lines from the ChangeLog. > Source/WebCore/rendering/RenderFlowThread.cpp:871 > + bool useHorizontalWritingMode = isHorizontalWritingMode(); If you need to cache the variable, use: bool isHorizontalWritingMode = this->isHorizontalWritingMode(); Alternatively shouldBeConsideredHorizontalWritingMode or equivalent would work. The word 'use' doesn't really fit in this context IMHO. > Source/WebCore/rendering/RenderFlowThread.cpp:885 > + for (; regionIter != m_regionList.end(); ++regionIter) { > + RenderRegion* currRegion = *regionIter; > + if (!currRegion->isValid() || (currRegion == region) || !currRegion->hasAutoLogicalHeight()) You could remove the need to check (currRegion == region) by tweaking the code a bit AFAICT. // Skip current region. ++regionIter; for (; regionItern != m_regionList.end(); ++regionIter) { > Source/WebCore/rendering/RenderRegion.cpp:238 > -void RenderRegion::repaintFlowThreadContentRectangle(const LayoutRect& repaintRect, bool immediate, const LayoutRect& flowThreadPortionRect, const LayoutRect& flowThreadPortionOverflowRect, const LayoutPoint& regionLocation) const > +void RenderRegion::repaintFlowThreadContentRectangle(const LayoutRect& repaintRect, bool immediate, const LayoutRect& flowThreadPortionRect, > + const LayoutRect& flowThreadPortionOverflowRect, const LayoutPoint& regionLocation) const Unneeded line break. > Source/WebCore/rendering/RenderRegion.cpp:616 > + if (view()->normalLayoutPhase()) > + return !hasOverrideHeight(); > + > + return false; Could be also written: return view()->normalLayoutPhase() && !hasOverrideHeight(); > Source/WebCore/rendering/RenderView.cpp:190 > + bool needsTwoPassLayoutForAutoLogicalHeightRegions = hasRenderNamedFlowThreads() > + && flowThreadController()->hasAutoLogicalHeightRegions(); Unneeded line breaking. > Source/WebCore/rendering/RenderView.cpp:206 > + layoutContent(); > +#ifndef NDEBUG > + checkLayoutState(state); > +#endif The checkLayoutState should be moved into layoutContent() as it will guarantee it's always called after you layout. > Source/WebCore/rendering/RenderView.cpp:-174 > - ASSERT(layoutDelta() == LayoutSize()); > - ASSERT(m_layoutStateDisableCount == 0); > - ASSERT(m_layoutState == &state); I really liked having these ASSERTs here as they ensured we always were in a good state before clearing m_layoutState. I would introduce a checkLayoutState here even if it is redundant with the other ones. > Source/WebCore/rendering/RenderView.h:257 > + void checkLayoutState(const LayoutState&); #ifndef NDEBUG?

Created attachment 168687 [details] Patch for landing

Comment on attachment 168687 [details] Patch for landing Clearing flags on attachment: 168687 Committed r131348: <http://trac.webkit.org/changeset/131348>

All reviewed patches have been landed. Closing bug.

Looks like it breaks in release: http://build.webkit.org/builders/Apple%20Lion%20Release%20%28Build%29/builds/5086/steps/compile-webkit/logs/stdio /Volumes/Data/slave/lion-release/build/Source/WebCore/rendering/RenderView.cpp:136:51: error: unused parameter 'state' [-Werror,-Wunused-parameter] void RenderView::layoutContent(const LayoutState& state)

(In reply to comment #13) > Looks like it breaks in release: > > http://build.webkit.org/builders/Apple%20Lion%20Release%20%28Build%29/builds/5086/steps/compile-webkit/logs/stdio > /Volumes/Data/slave/lion-release/build/Source/WebCore/rendering/RenderView.cpp:136:51: error: unused parameter 'state' [-Werror,-Wunused-parameter] > void RenderView::layoutContent(const LayoutState& state) Landed fix in r131363: <http://trac.webkit.org/changeset/131363>.