Add missing FractionalLayoutUnit += operator and move LineWidth to use all floats

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Levi Weintraub]

A number of bugs were discovered with sub-pixel layout when dealing with floats and particularly sub-pixel margins. Many of these issues came down to a missing FractionalLayoutUnit += operator which was incorrectly truncating sub-pixel margins when computing preferred widths and causing incorrect wrapping. To cover all cases, though, we also needed to move to using floats to represent the left and right bounds of lines. This is an improvement over our previous usage of ints in terms of precision, and should also avoid some unnecessary conversions between ints and floats.

Downstream Bugs this has been verified to fix:
http://code.google.com/p/chromium/issues/detail?id=130838
http://code.google.com/p/chromium/issues/detail?id=130742
http://code.google.com/p/chromium/issues/detail?id=130653
http://code.google.com/p/chromium/issues/detail?id=130760
http://code.google.com/p/chromium/issues/detail?id=129921
http://code.google.com/p/chromium/issues/detail?id=130378
http://code.google.com/p/chromium/issues/detail?id=130677

[Emil A Eklund]

Created attachment 145638 [details]
Patch

[Emil A Eklund]

Another downstream bug:
http://code.google.com/p/chromium/issues/detail?id=130659

[Ryosuke Niwa]

Comment on attachment 145638 [details]
Patch

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

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:110
> -    int m_left;
> -    int m_right;
> +    float m_left;
> +    float m_right;

Yikes. Nice to fix this.

[Emil A Eklund]

I plan on committing this (and the required rebaselines for mac and windows) later tonight when the tree is a bit more quiet.

[David Schlosnagle]

In addition to the patch changes in comment #3, there seems to be some inconsistency in FractionalLayoutUnit.h for the various operators and the permutations for FractionalLayoutUnit, int, float, and double arguments for left and right hand sides.

At initial glance, the following seem to be missing, although if they are not used, there may be no need for them:

inline bool operator<=(const FractionalLayoutUnit& a, double b)
{
    return a.toDouble() <= b;
}

inline bool operator<=(const double a, const FractionalLayoutUnit& b)
{
    return a <= b.toDouble();
}

inline bool operator>=(const FractionalLayoutUnit& a, float b)
{
    return a.toFloat() >= b;
}

inline bool operator>=(const double a, const FractionalLayoutUnit& b)
{
    return a >= b.toDouble();
}

inline bool operator<(const double a, const FractionalLayoutUnit& b)
{
    return a < b.toDouble();
}

inline bool operator!=(const FractionalLayoutUnit& a, double b)
{
    return a != FractionalLayoutUnit(b);
}

inline bool operator!=(const double a, const FractionalLayoutUnit& b)
{
    return FractionalLayoutUnit(a) != b;
}

inline bool operator!=(const float a, const FractionalLayoutUnit& b)
{
    return FractionalLayoutUnit(a) != b;
}

inline bool operator==(const FractionalLayoutUnit& a, double b)
{
    return a.toDouble() == b;
}

inline FractionalLayoutUnit& operator+=(FractionalLayoutUnit& a, double b)
{
    a = a + b;
    return a;
}

inline double& operator+=(double& a, const FractionalLayoutUnit& b)
{
    a = a + b;
    return a;
}

inline int& operator+=(int& a, const FractionalLayoutUnit& b)
{
    a = a + b;
    return a;
}

inline FractionalLayoutUnit& operator-=(FractionalLayoutUnit& a, float b)
{
    a = a - b;
    return a;
}

inline FractionalLayoutUnit& operator-=(FractionalLayoutUnit& a, double b)
{
    a = a - b;
    return a;
}

inline double& operator-=(double& a, const FractionalLayoutUnit& b)
{
    a = a - b;
    return a;
}

inline int& operator-=(int& a, const FractionalLayoutUnit& b)
{
    a = a - b;
    return a;
}

inline FractionalLayoutUnit& operator*=(FractionalLayoutUnit& a, const FractionalLayoutUnit& b)
{
    a = a * b;
    return a;
}

inline FractionalLayoutUnit& operator*=(FractionalLayoutUnit& a, double b)
{
    a = a * b;
    return a;
}

inline double& operator*=(double& a, const FractionalLayoutUnit& b)
{
    a = a * b;
    return a;
}

inline float& operator*=(float& a, const FractionalLayoutUnit& b)
{
    a = a * b;
    return a;
}

inline int& operator*=(int& a, const FractionalLayoutUnit& b)
{
    a = a * b;
    return a;
}

[David Schlosnagle]

The current version of FractionalLayoutUnit::epsilon does not seem correct either as it would always evaluate to 0.0f rather than the float equivalent of 0.0166666667, which seems like it could cause problems in FractionalLayoutRect::enclosingFractionalLayoutRect(const FloatRect& rect). I'd recommend the following:
    static float epsilon() { return 1.0f / kFixedPointDenominator; }

[Emil A Eklund]

Committed r119456: <http://trac.webkit.org/changeset/119456>

[Emil A Eklund]

(In reply to comment #5)
> In addition to the patch changes in comment #3, there seems to be some inconsistency in FractionalLayoutUnit.h for the various operators and the permutations for FractionalLayoutUnit, int, float, and double arguments for left and right hand sides.
> 
> At initial glance, the following seem to be missing, although if they are not used, there may be no need for them:

Up until now we've only added the operators that we've needed. This bug, however, shows that this might not have been the best of ideas though. As such we are seriously considering adding the missing operators.

The fact that epsilon is incorrect is a known problem and is on the list of things we need to fix. If you have any spare cycles and would like to help with any of this we'd _gladly_ welcome it! Either way, thank you for the comprehensive list of missing operators!

[David Schlosnagle]

> The fact that epsilon is incorrect is a known problem and is on the list of things we need to fix. If you have any spare cycles and would like to help with any of this we'd _gladly_ welcome it! Either way, thank you for the comprehensive list of missing operators!

Looks like Bug 86065 - "FractionalLayoutUnit minor math bugs" will address epsilon and some of the operator overloads. I may try to setup a WebKit build if I get a chance and post a more formal patch to that bug.

Thanks for fixing this bug, it was driving me crazy in recent Chrome builds.