There are still numerous places in RenderBlock we're using ints where we should be using LayoutUnits. Correcting.
Created attachment 113114 [details] Patch
Comment on attachment 113114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113114&action=review > Source/WebCore/rendering/RenderBlock.cpp:115 > + m_positiveMargin = m_canCollapseMarginBeforeWithChildren ? block->maxPositiveMarginBefore() : LayoutUnit(0); Explicit wrapping of zero makes me sad-faced. :(
(In reply to comment #2) > (From update of attachment 113114 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=113114&action=review > > > Source/WebCore/rendering/RenderBlock.cpp:115 > > + m_positiveMargin = m_canCollapseMarginBeforeWithChildren ? block->maxPositiveMarginBefore() : LayoutUnit(0); > > Explicit wrapping of zero makes me sad-faced. :( Sadly it's needed for the ternary operator. Would a constant (something like LAYOUT_ZERO) be less ugly?
(In reply to comment #2) > (From update of attachment 113114 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=113114&action=review > > > Source/WebCore/rendering/RenderBlock.cpp:115 > > + m_positiveMargin = m_canCollapseMarginBeforeWithChildren ? block->maxPositiveMarginBefore() : LayoutUnit(0); > > Explicit wrapping of zero makes me sad-faced. :( We can also save these for our final patch, where we can just use LayoutUnit() if you prefer
LayoutUnit::zero would probably read better to my eyes, even though that's more characters. :(
(In reply to comment #5) > LayoutUnit::zero would probably read better to my eyes, even though that's more characters. :( We can't do that with our typedef, unfortunately, but we can once we've switched from ints. In the meantime, I think I'll just remove the wrapping and leave it for our branch integration.
I mean, it's trivial to convert from a typedef to a class if you feel that's holding you back. :) But OK.
(In reply to comment #7) > I mean, it's trivial to convert from a typedef to a class if you feel that's holding you back. :) But OK. I'd much rather postpone this than switch to a class to represent the int base type :p
Created attachment 113259 [details] Patch
Comment on attachment 113259 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113259&action=review Is numeric_limits<LayoutUnit>::max() ever actually the right/helpful value for LayoutUnit other than int? > Source/WebCore/rendering/RenderBlock.cpp:2503 > + // We don't want to had off painting in the line box tree with the accumulated error of the render tree, as this will cause "had off"?
Comment on attachment 113259 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113259&action=review >> Source/WebCore/rendering/RenderBlock.cpp:2503 >> + // We don't want to had off painting in the line box tree with the accumulated error of the render tree, as this will cause > > "had off"? "hand off", thanks for the catch and the review!
Created attachment 113263 [details] Patch for landing
Comment on attachment 113263 [details] Patch for landing Clearing flags on attachment: 113263 Committed r99024: <http://trac.webkit.org/changeset/99024>
All reviewed patches have been landed. Closing bug.
(In reply to comment #10) > (From update of attachment 113259 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=113259&action=review > > Is numeric_limits<LayoutUnit>::max() ever actually the right/helpful value for LayoutUnit other than int? I missed your first comment about numeric_limits<LayoutUnit>. If you look at the subpixellayout branch or the patch on bug 71143, you'll see I actually implemented the numeric_limits struct for AppUnit, which is what we're aiming to replace ints with.
(In reply to comment #15) > (In reply to comment #10) > > Is numeric_limits<LayoutUnit>::max() ever actually the right/helpful value for LayoutUnit other than int? > > I missed your first comment about numeric_limits<LayoutUnit>. If you look at the subpixellayout branch or the patch on bug 71143, you'll see I actually implemented the numeric_limits struct for AppUnit, which is what we're aiming to replace ints with. I don’t think the standard allows specializing numeric_limits for non-built-in types, but it might work with most compilers we deal with now. There are other solutions that don’t require non-standard code.