We currently overflow/wrap when computing the delta in RenderBlock::setLogicalTopForChild in cases where we have an element width a width or height exceeding maxLayoutUnit. When the delta is later added back in RenderBlock::layoutBlockChild the number wraps again getting us back to the correct value. With SATURATED_LAYOUT_ARITHMETIC enabled the values no longer wraps, which seems like the correct thing to do however this causes the compare to fail for obvious reasons. By accounting for this we can keep the asserts (which have proven very helpful) even when SATURATED_LAYOUT_ARITHMETIC is turned on.
Created attachment 167625 [details] Patch
Eric, would you mind reviewing this?
Mitz is the original author of LayoutDelta and LayoutState and is your best reviewer here.
Great, thanks!
Mitz, any chance you could take a look at this in the next couple of days? Would like to get your feedback on the approach.
Comment on attachment 167625 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167625&action=review > Source/WebCore/ChangeLog:10 > + element width a width or height exceeding maxLayoutUnit. When Typo: “width a width”
Thank you!
Created attachment 169935 [details] Patch for landing
Comment on attachment 169935 [details] Patch for landing Clearing flags on attachment: 169935 Committed r132105: <http://trac.webkit.org/changeset/132105>
All reviewed patches have been landed. Closing bug.
Comment on attachment 169935 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=169935&action=review > Source/WebCore/rendering/LayoutState.cpp:39 > + : m_clipped(false) Why make this change? m_clipped is set unconditionally below. Just for #if convenience? > Source/WebCore/rendering/LayoutState.cpp:43 > +#if !ASSERT_DISABLED && ENABLE(SATURATED_LAYOUT_ARITHMETIC) > + , m_layoutDeltaXSaturated(false) > + , m_layoutDeltaYSaturated(false) > +#endif Why initialize these here since they are unconditionally set in the function body below? This makes things more ugly for no real reason.
(In reply to comment #11) > (From update of attachment 169935 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=169935&action=review > > > Source/WebCore/rendering/LayoutState.cpp:39 > > + : m_clipped(false) > > Why make this change? m_clipped is set unconditionally below. Just for #if convenience? Got a compiler warning about out of order initialization without it. > > > Source/WebCore/rendering/LayoutState.cpp:43 > > +#if !ASSERT_DISABLED && ENABLE(SATURATED_LAYOUT_ARITHMETIC) > > + , m_layoutDeltaXSaturated(false) > > + , m_layoutDeltaYSaturated(false) > > +#endif > > Why initialize these here since they are unconditionally set in the function body below? This makes things more ugly for no real reason. You are right, these are not needed. I'll remove them. Thanks.
Comment on attachment 169935 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=169935&action=review >>> Source/WebCore/rendering/LayoutState.cpp:39 >>> + : m_clipped(false) >> >> Why make this change? m_clipped is set unconditionally below. Just for #if convenience? > > Got a compiler warning about out of order initialization without it. I don’t understand. Despite you reporting that, I am pretty sure you can remove this without creating a warning, particularly now that you have removed the initialization of the two new fields. This constructor should revert to exactly how it was before.
(In reply to comment #13) > I don’t understand. Despite you reporting that, I am pretty sure you can remove this without creating a warning, particularly now that you have removed the initialization of the two new fields. You are right, without the (unnecessary) initialization it is not needed. Reverted back to how it was before in r132233. Thanks.