Bug 88171

Created attachment 148009 [details] Patch

Comment on attachment 148009 [details] Patch Attachment 148009 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12975223 New failing tests: css1/formatting_model/floating_elements.html

Created attachment 148017 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 148019 [details] Patch

Comment on attachment 148019 [details] Patch Attachment 148019 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12978108 New failing tests: css1/formatting_model/floating_elements.html

Created attachment 148023 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 149588 [details] New result in floating_elements

Created attachment 149589 [details] Current rendering of change to floating_elements

Created attachment 149591 [details] Patch

Comment on attachment 149591 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149591&action=review > LayoutTests/ChangeLog:64 > + * platform/chromium-win/css1/formatting_model/floating_elements-expected.txt: > + This is a progression due to lineboxes now avoiding floats they overlap. The change in rendering that causes the change in results is viewable here: https://bugs.webkit.org/attachment.cgi?id=149588 You can compare that with the current rendering: https://bugs.webkit.org/attachment.cgi?id=149589

Comment on attachment 149591 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=149591&action=review I think the change is OK. Need answers to a couple questions first: > Source/WebCore/rendering/RenderBlock.cpp:3958 > + if ((m_lowValue >= interval.low() && m_lowValue < interval.high()) > + || (m_lowValue < interval.low() && m_highValue > interval.high()) > + || (m_highValue > m_lowValue && m_highValue > interval.low() && m_highValue <= interval.high())) { This is just a non-intersecting range check, right? Don't we have a cleaner way to write this? Maybe with a helper? I guess we don't really have a "range" object. > Source/WebCore/rendering/RenderBlock.cpp:3963 > + ASSERT(r->isPlaced() && ((m_renderer->pixelSnappedLogicalTopForFloat(r) <= m_lowValue && m_renderer->pixelSnappedLogicalBottomForFloat(r) > m_lowValue) > + || (m_lowValue < m_renderer->pixelSnappedLogicalTopForFloat(r) && m_highValue > m_renderer->pixelSnappedLogicalBottomForFloat(r)) > + || (m_highValue > m_lowValue && m_renderer->pixelSnappedLogicalTopForFloat(r) < m_highValue && m_highValue <= m_renderer->pixelSnappedLogicalBottomForFloat(r)))); I'm not sure I understand this, but it looks similar to above, and likely could use the same helper function? > Source/WebCore/rendering/RenderBlock.cpp:4018 > + FloatIntervalSearchAdapter<FloatingObject::FloatLeft> adapter(this, roundToInt(logicalTop), roundToInt(logicalTop + logicalHeight), left, heightRemaining); Should these be rounded? Do we need to snap these? Have you asked Emil/Levi or consulted http://trac.webkit.org/wiki/LayoutUnit ? > Source/WebCore/rendering/RenderBlock.h:132 > + LayoutUnit availableLogicalWidthForLine(LayoutUnit position, bool firstLine, RenderRegion* region, LayoutUnit offsetFromLogicalTopOfFirstPage, LayoutUnit logicalHeight = 0) const A height of 0 seems like an odd default. Why does this need a default? > Source/WebCore/rendering/RenderBlockLineLayout.cpp:134 > + m_left = m_block->logicalLeftOffsetForLine(height, m_isFirstLine, logicalBottom); > + m_right = m_block->logicalRightOffsetForLine(height, m_isFirstLine, logicalBottom); Now your passing bottom where other places you pass top, height. Is this intentional?

Created attachment 150336 [details] Patch

(In reply to comment #11) > (From update of attachment 149591 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=149591&action=review > > I think the change is OK. Need answers to a couple questions first: > > > Source/WebCore/rendering/RenderBlock.cpp:3958 > > + if ((m_lowValue >= interval.low() && m_lowValue < interval.high()) > > + || (m_lowValue < interval.low() && m_highValue > interval.high()) > > + || (m_highValue > m_lowValue && m_highValue > interval.low() && m_highValue <= interval.high())) { > > This is just a non-intersecting range check, right? Don't we have a cleaner way to write this? Maybe with a helper? I guess we don't really have a "range" object. I've improved the code but not the quality of the check. I'm sure there's a way to do it more concisely but I can't see it. > > > Source/WebCore/rendering/RenderBlock.cpp:4018 > > + FloatIntervalSearchAdapter<FloatingObject::FloatLeft> adapter(this, roundToInt(logicalTop), roundToInt(logicalTop + logicalHeight), left, heightRemaining); > > Should these be rounded? Do we need to snap these? Have you asked Emil/Levi or consulted http://trac.webkit.org/wiki/LayoutUnit ? roundToInt(logicalTop) is current code - so I followed it here. From my reading of the wiki page roundToInt() seems right here. > > > Source/WebCore/rendering/RenderBlock.h:132 > > + LayoutUnit availableLogicalWidthForLine(LayoutUnit position, bool firstLine, RenderRegion* region, LayoutUnit offsetFromLogicalTopOfFirstPage, LayoutUnit logicalHeight = 0) const > > A height of 0 seems like an odd default. Why does this need a default? All of the prototypes here have a bunch of callers that don't (at the moment care) about the logical height of the child - so the default gets set to zero and this gets added to the logical top of the child when checking its top and 'bottom' against floats. > > > Source/WebCore/rendering/RenderBlockLineLayout.cpp:134 > > + m_left = m_block->logicalLeftOffsetForLine(height, m_isFirstLine, logicalBottom); > > + m_right = m_block->logicalRightOffsetForLine(height, m_isFirstLine, logicalBottom); > > Now your passing bottom where other places you pass top, height. Is this intentional? Yeah - logicalHeight is a slightly misleading name in a lot of RenderBlock stuff - here it means the logicalTop of the current child or line, which is why I went for logicalBottom to express the height/bottom of the line. But I think you're right that that just confuses things a bit more. I've changed it back so it's the same color of confusing everywhere.

Comment on attachment 150336 [details] Patch LGTM. I think we should consider adding a LineRange class in a follow-up patch. I suspect with a little object orientation we could make this read a lot nicer. :)

Comment on attachment 150336 [details] Patch Clearing flags on attachment: 150336 Committed r121789: <http://trac.webkit.org/changeset/121789>

All reviewed patches have been landed. Closing bug.