Bug 93475 – Floored and truncated rounding confused

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

[Allan Sandfeld Jensen]

The two rounding forms floor and truncate are mixed up in several places in the code, mostly in the subpixel layout code, but also in flooredIntPoint(FloatPoint). For most cases this is not a problem since truncate and floor are identical for positive numbers, they only differ for negative numbers, which are not as nearly as common in the WebCore layout code.

This bug currently only leads to minor pixel differences with subpixel layout enabled, but the fix for bug #23170 depends on it both with and without subpixel layout enabled.

[Allan Sandfeld Jensen]

Created attachment 157211 [details]
Patch

[Levi Weintraub]

Comment on attachment 157211 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        This patch changes several instances of floored rounding to do actual floored
> +        rounding, and a fixes and cleans up several places using floored rounding.

What are you saying this changes? I'm not sure what "actual" means here.

> Source/WebCore/platform/graphics/FractionalLayoutRect.cpp:134
> -    float x = floorf(rect.x().toFloat());
> -    float y = floorf(rect.y().toFloat());
> -    float width = ceilf(rect.maxX().toFloat()) - x;
> -    float height = ceilf(rect.maxY().toFloat()) - y;
> +    IntPoint location = flooredIntPoint(rect.minXMinYCorner());
> +    IntPoint maxPoint = ceiledIntPoint(rect.maxXMaxYCorner());
>  
> -    return IntRect(clampToInteger(x), clampToInteger(y), 
> -                   clampToInteger(width), clampToInteger(height));
> +    return IntRect(location, maxPoint - location);

So much cleaner, well done!

> Source/WebCore/rendering/RenderReplaced.cpp:291
> -            m_intrinsicSize = flooredIntSize(intrinsicSize); // FIXME: This introduces precision errors. We should convert m_intrinsicSize to be a float.
> +            m_intrinsicSize = IntSize(intrinsicSize.width(), intrinsicSize.height()); // FIXME: This introduces precision errors. We should convert m_intrinsicSize to be a float.

Why this change is correct isn't obvious to me. Since intrinsic size is never negative this doesn't seem to change any behavior. How about an explanation in the changelog?

[Allan Sandfeld Jensen]

(In reply to comment #2)
> (From update of attachment 157211 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157211&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        This patch changes several instances of floored rounding to do actual floored
> > +        rounding, and a fixes and cleans up several places using floored rounding.
> 
> What are you saying this changes? I'm not sure what "actual" means here.
> 
That is not clear I agree. I will remove 'actual' and instead explain that some floor operations were previously doing rounding towards zero, when they should be rounding down (away from zero for negative numbers).

> > Source/WebCore/rendering/RenderReplaced.cpp:291
> > -            m_intrinsicSize = flooredIntSize(intrinsicSize); // FIXME: This introduces precision errors. We should convert m_intrinsicSize to be a float.
> > +            m_intrinsicSize = IntSize(intrinsicSize.width(), intrinsicSize.height()); // FIXME: This introduces precision errors. We should convert m_intrinsicSize to be a float.
> 
> Why this change is correct isn't obvious to me. Since intrinsic size is never negative this doesn't seem to change any behavior. How about an explanation in the changelog?

You are right, I should have left that unchanged. I was at one point considering removing all uses of flooredIntSize (around 3), but decided to not to include that in this patch. I guess I forgot to revert this change.

[Allan Sandfeld Jensen]

Committed r125167: <http://trac.webkit.org/changeset/125167>

[Levi Weintraub]

Comment on attachment 157211 [details]
Patch

Clearing flags.