Float imprecision causes incorrect wrapping in LineLayout with subpixel layout

[Emil A Eklund]

Due to float imprecision an incorrect wrapping decision is made in certain cases where the values being compare are close but not exactly the same. This can happen as the size of blocks is represented in layout units while line layout uses floats.

Downstream chromium bug: http://code.google.com/p/chromium/issues/detail?id=139502

[Levi Weintraub]

So excited to get this one fixed!

[Emil A Eklund]

Created attachment 155604 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 155604 [details]
Patch

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

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:79
> +    bool fitsOnLine() const { return currentWidth() - m_availableWidth < LayoutUnit::epsilon(); }
> +    bool fitsOnLine(float extra) const { return currentWidth() + extra - m_availableWidth < LayoutUnit::epsilon(); }

Don't we have a helper to compare to layout units within epsilon?  It seems that might be more clear... I mean, not that this simple math isn't clear-ish too...

Also, this is looking specifically to see if currentWidth is no more than + epsilon greater than m_available width.  Why not just change the previous statements to be \

currentWidth() <= (m_availableWidth + LayoutUnit::epsilon())?

I guess your way doesn't overflow if available width is large?

[Emil A Eklund]

(In reply to comment #3)
> (From update of attachment 155604 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155604&action=review
> 
> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:79
> > +    bool fitsOnLine() const { return currentWidth() - m_availableWidth < LayoutUnit::epsilon(); }
> > +    bool fitsOnLine(float extra) const { return currentWidth() + extra - m_availableWidth < LayoutUnit::epsilon(); }
> 
> Don't we have a helper to compare to layout units within epsilon?  It seems that might be more clear... I mean, not that this simple math isn't clear-ish too...

Not here, we have one for comparing a LayoutUnit with a float but here we are comparing two floats. I could hardcode the tolerance to 0.01 which it what we use in css code but as LayoutUnit::epsilon is the max we support it seemed more correct.

> 
> Also, this is looking specifically to see if currentWidth is no more than + epsilon greater than m_available width.  Why not just change the previous statements to be \
> 
> currentWidth() <= (m_availableWidth + LayoutUnit::epsilon())?
> 
> I guess your way doesn't overflow if available width is large?

Old habit, doesn't matter here as it is all float. Changed it as suggested.

[Emil A Eklund]

Created attachment 155640 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 155640 [details]
Patch

LGTM.

[Emil A Eklund]

Saw some failures on mac that might I'm not entirely sure where not caused by this (as this effectively means that the value can be off by almost 1 on ports without subpixel support). Adding an ifdef just to make sure.

[Emil A Eklund]

Created attachment 155694 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 155694 [details]
Patch for landing

Clearing flags on attachment: 155694

Committed r124295: <http://trac.webkit.org/changeset/124295>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.