Bug 88918 - Cast paddings to int in RenderTableCell
Summary: Cast paddings to int in RenderTableCell
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: Emil A Eklund
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-12 14:23 PDT by Emil A Eklund
Modified: 2012-06-14 23:08 PDT (History)
5 users (show)

See Also:


Attachments
Patch (498.95 KB, patch)
2012-06-12 14:39 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Emil A Eklund 2012-06-12 14:23:45 PDT
Table layout uses integers throughout (as documented on https://trac.webkit.org/wiki/LayoutUnit) yet the TableCell paddingLeft/Right/Top/Bottom methods returns LayoutUnits. This causes inconsistent rounding as some call sites cast the numbers to ints before doing computation and others do computation before casting. By changing the methods to always cast the padding values to int we ensure consistent padding calculations. Ideally we'd change the type of the return value for the methods but as they are overriden that would likely cause more confusion.
Comment 1 Emil A Eklund 2012-06-12 14:39:15 PDT
Created attachment 147165 [details]
Patch
Comment 2 Emil A Eklund 2012-06-12 17:06:46 PDT
FYI: The chromium test expectation changes here (apart from the new test) reflects us going back to rendering that more closely matches the other platforms (and in most cases matches how they looked in chromium before we turned on subpixel layout).
Comment 3 Levi Weintraub 2012-06-13 10:01:18 PDT
Comment on attachment 147165 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147165&action=review

You need to also update the test expectations files to prevent the bots from going red on non-linux platforms. With that r=me.

> Source/WebCore/rendering/RenderTableCell.cpp:228
> -    return computedCSSPaddingBefore() + intrinsicPaddingBefore();
> +    return static_cast<int>(computedCSSPaddingBefore() + intrinsicPaddingBefore());

Only casting the computedCSSPaddingBefore would save unnecessarily promoting intrinsicPaddingBefore to a LayoutUnit.
Comment 4 Emil A Eklund 2012-06-13 13:34:03 PDT
(In reply to comment #3)
> (From update of attachment 147165 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147165&action=review
> 
> You need to also update the test expectations files to prevent the bots from going red on non-linux platforms. With that r=me.

Will do, I really wish there was a better way to do this.

> 
> > Source/WebCore/rendering/RenderTableCell.cpp:228
> > -    return computedCSSPaddingBefore() + intrinsicPaddingBefore();
> > +    return static_cast<int>(computedCSSPaddingBefore() + intrinsicPaddingBefore());
> 
> Only casting the computedCSSPaddingBefore would save unnecessarily promoting intrinsicPaddingBefore to a LayoutUnit.

Ah, good idea.
Comment 5 Emil A Eklund 2012-06-14 23:07:41 PDT
Committed r120403: <http://trac.webkit.org/changeset/120403>