Bug 118526

Created attachment 206670 [details] WIP

Created attachment 207254 [details] Patch

Comment on attachment 207254 [details] Patch I need to rewrite the test descriptions. Will do before landing the patch if everything is OK.

Comment on attachment 207254 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207254&action=review > Source/WebCore/rendering/RenderRegion.cpp:137 > +// FIXME: This is wrong for horizontal overflow because the overflow rectangle doesn't take the region size into account. > +LayoutRect RenderRegion::layoutOverflowRectForFlowThreadPortion(const LayoutRect& flowThreadPortionRect, bool isFirstPortion, bool isLastPortion) const Is there no way to share more code with the other overflow function? There's a lot of similar logic here. > Source/WebCore/rendering/RenderRegion.cpp:375 > + if (style()->writingMode() != m_flowThread->style()->writingMode()) { > + if (m_flowThread->style()->writingMode() == RightToLeftWritingMode || style()->writingMode() == RightToLeftWritingMode) > + layoutRect.setX(width() - layoutRect.maxX()); > + else if (m_flowThread->style()->writingMode() == BottomToTopWritingMode || style()->writingMode() == BottomToTopWritingMode) > + layoutRect.setY(height() - layoutRect.maxY()); > + } This does not seem right to me.

Created attachment 207385 [details] Patch

Comment on attachment 207385 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207385&action=review > Source/WebCore/rendering/RenderRegion.cpp:116 > + // Only clip along the flow thread axis if we want the visual overflow. I'll correct the comment before committing. It was supposed to say we are not interested about the outline size when computing the layout overflow because the outline is just visual.

Comment on attachment 207385 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207385&action=review Looks good. Just fix the new argument to be an enum and it's good to go. > Source/WebCore/rendering/RenderRegion.h:148 > + LayoutRect overflowRectForFlowThreadPortion(const LayoutRect& flowThreadPortionRect, bool isFirstPortion, bool isLastPortion, bool isVisual) const; Use an enum for the new argument please. The general rule is to use enums if you're going to pass raw true/false at call sites so that the call sites aren't confusing.

Comment on attachment 207385 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=207385&action=review > Source/WebCore/rendering/RenderBlock.cpp:2770 > + // FIXME: Find a way to invalidate the knowToHaveNoOverflow flah on the InlineBoxes. flah -> flag > Source/WebCore/rendering/RenderFlowThread.cpp:62 > + , m_layoutPhase(0) 0 -> LayoutPhaseNormal > Source/WebCore/rendering/RenderFlowThread.h:162 > + enum LayoutPhase { It would be awesome if you can add a comment above clarifying what each phase is about. Something like the description for StyleDifference. > Source/WebCore/rendering/RenderRegion.cpp:333 > + if (!m_flowThread) Is this check needed? Should this be an ASSERT instead?

(In reply to comment #8) > (From update of attachment 207385 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=207385&action=review > > > Source/WebCore/rendering/RenderBlock.cpp:2770 > > + // FIXME: Find a way to invalidate the knowToHaveNoOverflow flah on the InlineBoxes. > > flah -> flag Will do. > > > Source/WebCore/rendering/RenderFlowThread.cpp:62 > > + , m_layoutPhase(0) > > 0 -> LayoutPhaseNormal Will do. > > > Source/WebCore/rendering/RenderFlowThread.h:162 > > + enum LayoutPhase { > > It would be awesome if you can add a comment above clarifying what each phase is about. Something like the description for StyleDifference. Will do. > > > Source/WebCore/rendering/RenderRegion.cpp:333 > > + if (!m_flowThread) > > Is this check needed? Should this be an ASSERT instead? This is needed in case the region is invalid and has no flow thread attached. I can replace it with !isValid().

Created attachment 208257 [details] Patch

Comment on attachment 208257 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208257&action=review > Source/WebCore/rendering/RenderFlowThread.h:164 > + LayoutPhaseNormal = 0, // The initial phase, used to measure content for the auto-height regions. Calling this LayoutPhaseMeasureContent or even LayoutPhaseMeasureContentForAutoHeightRegions makes the code readable: an assert like ASSERT( inNormalLayoutPhase() && hasAutoLogicalHeightRegions() ) in RenderFlowThread::updateRegionsFlowThreadPortionRect will sound like this: ASSERT( inMeasureContentForAutoHeightRegionsLayoutPhase() && hasAutoLogicalHeightRegions() ) It now makes sense why it's needed to test for hasAutoLogicalHeightRegions();

Comment on attachment 208257 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208257&action=review > Source/WebCore/ChangeLog:13 > + The overflow phase is set after the flow threads are laid out and it marks the regions they I think "and it marks the regions as needing to extract" would be more clear. I kept reading it as "(it marks the regions they need) to extract...".

Created attachment 208273 [details] Patch

Comment on attachment 208273 [details] Patch r=me

Comment on attachment 208273 [details] Patch Clearing flags on attachment: 208273 Committed r153814: <http://trac.webkit.org/changeset/153814>

All reviewed patches have been landed. Closing bug.