Bug 71254 - Amend missing uses of LayoutUnit in RenderBlock
Summary: Amend missing uses of LayoutUnit in RenderBlock
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Levi Weintraub
URL:
Keywords:
Depends on:
Blocks: 63567
  Show dependency treegraph
 
Reported: 2011-10-31 17:00 PDT by Levi Weintraub
Modified: 2011-11-02 10:28 PDT (History)
4 users (show)

See Also:


Attachments
Patch (29.72 KB, patch)
2011-10-31 17:31 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (24.88 KB, patch)
2011-11-01 17:01 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch for landing (24.88 KB, patch)
2011-11-01 17:29 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Levi Weintraub 2011-10-31 17:00:26 PDT
There are still numerous places in RenderBlock we're using ints where we should be using LayoutUnits. Correcting.
Comment 1 Levi Weintraub 2011-10-31 17:31:13 PDT
Created attachment 113114 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-10-31 17:37:20 PDT
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. :(
Comment 3 Emil A Eklund 2011-11-01 15:39:40 PDT
(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?
Comment 4 Levi Weintraub 2011-11-01 15:52:55 PDT
(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
Comment 5 Eric Seidel (no email) 2011-11-01 16:01:43 PDT
LayoutUnit::zero would probably read better to my eyes, even though that's more characters.  :(
Comment 6 Levi Weintraub 2011-11-01 16:05:05 PDT
(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.
Comment 7 Eric Seidel (no email) 2011-11-01 16:10:20 PDT
I mean, it's trivial to convert from a typedef to a class if you feel that's holding you back. :)  But OK.
Comment 8 Levi Weintraub 2011-11-01 16:12:43 PDT
(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
Comment 9 Levi Weintraub 2011-11-01 17:01:19 PDT
Created attachment 113259 [details]
Patch
Comment 10 Darin Adler 2011-11-01 17:26:05 PDT
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 11 Levi Weintraub 2011-11-01 17:27:09 PDT
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!
Comment 12 Levi Weintraub 2011-11-01 17:29:29 PDT
Created attachment 113263 [details]
Patch for landing
Comment 13 WebKit Review Bot 2011-11-01 18:33:31 PDT
Comment on attachment 113263 [details]
Patch for landing

Clearing flags on attachment: 113263

Committed r99024: <http://trac.webkit.org/changeset/99024>
Comment 14 WebKit Review Bot 2011-11-01 18:33:36 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Levi Weintraub 2011-11-01 23:09:43 PDT
(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.
Comment 16 Darin Adler 2011-11-02 10:28:03 PDT
(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.