Bug 71254 – Amend missing uses of LayoutUnit in RenderBlock

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Levi Weintraub]

There are still numerous places in RenderBlock we're using ints where we should be using LayoutUnits. Correcting.

[Levi Weintraub]

Created attachment 113114 [details]
Patch

[Eric Seidel (no email)]

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. :(

[Emil A Eklund]

(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?

[Levi Weintraub]

(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

[Eric Seidel (no email)]

LayoutUnit::zero would probably read better to my eyes, even though that's more characters.  :(

[Levi Weintraub]

(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.

[Eric Seidel (no email)]

I mean, it's trivial to convert from a typedef to a class if you feel that's holding you back. :)  But OK.

[Levi Weintraub]

(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

[Levi Weintraub]

Created attachment 113259 [details]
Patch

[Darin Adler]

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"?

[Levi Weintraub]

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!

[Levi Weintraub]

Created attachment 113263 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 113263 [details]
Patch for landing

Clearing flags on attachment: 113263

Committed r99024: <http://trac.webkit.org/changeset/99024>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Levi Weintraub]

(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.

[Darin Adler]

(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.