Bug 54244 – Convert the line box tree to floating point and eliminate font rounding hacks

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

[Dave Hyatt]

Convert the line box tree to floating point and eliminate font rounding hacks.  This fixes numerous issues in environments that don't return integral values for widths of glyphs.

[Dave Hyatt]

Created attachment 82044 [details]
Patch (don't really review, just catching build errors)

[Dave Hyatt]

Created attachment 82058 [details]
Patch that applies (ignore for review, just testing builds)

[WebKit Review Bot]

Attachment 82058 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/WebCore.exp.in', u'Source/W..." exit_code: 1

Source/WebCore/rendering/InlineTextBox.cpp:938:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/rendering/InlineTextBox.cpp:939:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/rendering/InlineTextBox.cpp:1109:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/rendering/InlineTextBox.cpp:1110:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/rendering/HitTestResult.h:114:  The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/Font.h:97:  The parameter name "glyphOverflow" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/Font.h:98:  The parameter name "run" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 7 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dave Hyatt]

Created attachment 82062 [details]
Patch that applies (ignore for review, just testing builds)

[WebKit Review Bot]

Attachment 82062 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/WebCore.exp.in', u'Source/W..." exit_code: 1

Source/WebCore/rendering/InlineTextBox.cpp:938:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/rendering/InlineTextBox.cpp:939:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/rendering/InlineTextBox.cpp:1109:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/rendering/InlineTextBox.cpp:1110:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/rendering/HitTestResult.h:114:  The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/Font.h:97:  The parameter name "glyphOverflow" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/Font.h:98:  The parameter name "run" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 7 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 82058 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7884413

[WebKit Review Bot]

Attachment 82062 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7869499

[Build Bot]

Attachment 82058 [details] did not build on win:
Build output: http://queues.webkit.org/results/7869503

[Build Bot]

Attachment 82062 [details] did not build on win:
Build output: http://queues.webkit.org/results/7869507

[Early Warning System Bot]

Attachment 82062 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7884461

[WebKit Review Bot]

Attachment 82062 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7870574

<rdar://problem/8891123>

[Collabora GTK+ EWS bot]

Attachment 82062 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7869566

[Dave Hyatt]

Created attachment 82153 [details]
Patch that applies (ignore for review, just testing builds)

[WebKit Review Bot]

Attachment 82153 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7900003

[Dave Hyatt]

Created attachment 82161 [details]
Patch that applies (ignore for review, just testing builds)

[Build Bot]

Attachment 82153 [details] did not build on win:
Build output: http://queues.webkit.org/results/7893014

[Early Warning System Bot]

Attachment 82153 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7870746

[WebKit Review Bot]

Attachment 82161 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7870748

[Collabora GTK+ EWS bot]

Attachment 82161 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7870754

[Build Bot]

Attachment 82161 [details] did not build on win:
Build output: http://queues.webkit.org/results/7869739

[Early Warning System Bot]

Attachment 82161 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7867750

[Dave Hyatt]

Created attachment 82181 [details]
Patch that applies (ignore for review, just testing builds)

[WebKit Review Bot]

Attachment 82181 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7867766

[Dave Hyatt]

Created attachment 82187 [details]
Patch that applies (ignore for review, just testing builds)

[Collabora GTK+ EWS bot]

Attachment 82187 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7867771

[Dave Hyatt]

Created attachment 82194 [details]
Patch that applies (ignore for review, just testing builds)

[Build Bot]

Attachment 82194 [details] did not build on win:
Build output: http://queues.webkit.org/results/7884715

[Dave Hyatt]

Created attachment 82282 [details]
Patch that applies (ignore for review, just testing builds)

[Dave Hyatt]

Created attachment 82339 [details]
Patch that applies (ignore for review, just testing builds)

In pretty good shape now I think.  Fixed SVG dumping in this patch to not dump floats and also fixed a bug with vertical alignment created by some issues using the dumb PositionTop and PositionBottom constants.  I just yanked those since they aren't necessary.

[Dave Hyatt]

Created attachment 82347 [details]
Patch that applies (ignore for review, just testing builds)

Fix offsetForPosition, positionForOffset to be float-based to avoid rounding errors.  Patch localCaretRect to operate on floats to avoid truncation mistakes.

[Dave Hyatt]

Created attachment 82356 [details]
Ready for review!

Layout test changes too massive to include in the patch.  I'll post a followup attachment that provides a sample of how they're changing.

[Dave Hyatt]

Created attachment 82357 [details]
Sample of layout tests changes

[Build Bot]

Attachment 82347 [details] did not build on win:
Build output: http://queues.webkit.org/results/7909694

[Build Bot]

Attachment 82356 [details] did not build on win:
Build output: http://queues.webkit.org/results/7908713

Comment on attachment 82356 [details]
Ready for review!

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

r- because of EWS build failures. Some minor comments inside.

> Source/WebCore/ChangeLog:8
> +        hacks from the Font code and makes sure all Font APIS involving width measurement and width offsets use floats.

Typo: APIS

> Source/WebCore/platform/graphics/WidthIterator.cpp:173
> +                        float expansion = previousExpansion - m_expansion;

Don’t need this anymore, can just use m_expansionPerOpportunity. You don’t need previousExpansion at all I think.

> Source/WebCore/platform/graphics/mac/ComplexTextController.cpp:480
> +                            float expansion = previousExpansion - m_expansion;

Here too you can get rid of expansion and previousExpansion.

> Source/WebCore/platform/graphics/win/UniscribeController.cpp:-323
> -        // Match AppKit's rules for the integer vs. non-integer rendering modes.
> -        float roundedAdvance = roundf(advance);
> -        if (!m_font.isPrinterFont() && !fontData->isSystemFont()) {
> -            advance = roundedAdvance;
> -            offsetX = roundf(offsetX);
> -            offsetY = roundf(offsetY);
> -        }

Don’t we need this still?

> Source/WebCore/platform/graphics/win/UniscribeController.cpp:319
>                          float previousPadding = m_padding;
>                          m_padding -= m_padPerSpace;
> -                        advance += roundf(previousPadding) - roundf(m_padding);
> +                        advance += previousPadding - m_padding;

Again, no need for previousPadding anymore. Just add m_padPerSpace to advance and subtract m_padPerSpace from m_padding.

> Source/WebCore/rendering/InlineBox.cpp:109
> + 

Space!

> Source/WebCore/rendering/InlineBox.h:312
> +    

Spaces!

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:397
> +                    trailingSpaceRun->m_box->setLogicalWidth(max(0.f, trailingSpaceRun->m_box->logicalWidth() - totalLogicalWidth + availableLogicalWidth));

Another way to write this is max<float>(0, …)

> Source/WebCore/rendering/RenderText.cpp:509
> +    // Go ahead and round left to snap it to the nearest pixel.

:)

> Source/WebCore/rendering/RenderText.cpp:705
> +        const_cast<RenderText*>(this)->computePreferredLogicalWidths(0.f);

Is this .f needed?

> Source/WebCore/rendering/RenderText.cpp:713
> +        const_cast<RenderText*>(this)->computePreferredLogicalWidths(0.f);

Ditto.

> Source/WebCore/rendering/RenderTreeAsText.cpp:474
> +    // to detect any changes caused by the conversion to floating point. :(

The best thing you can do is generate pixel results for all tests on your machine w/o the patch, then apply the patch and run-webkit-tests --pixel and see if there are any significant differences.

[Dominic Cooney]

This patch will also fix bug 14956, at least for LTR content.

[Dave Hyatt]

(In reply to comment #37)
> This patch will also fix bug 14956, at least for LTR content.

Not sure it will.  I'm rounding to pixels when painting borders and backgrounds, so you'll probably have to test it to see if it really fixes that issue.

[Dave Hyatt]

Created attachment 82488 [details]
Address Dan's comments.

[Nikolas Zimmermann]

Comment on attachment 82488 [details]
Address Dan's comments.

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

Great, looking forward to this patch! I have another patch in the works, eliminating SVG pixel test diffs between 10.6.6 32bit/64bit machines, that will probably change enclosingIntRect, we might want to discuss that, to avoid several rebaselines...

> Source/WebCore/rendering/InlineBox.h:244
> +    int pixelSnappedLogicalLeft() const { return logicalLeft(); }
> +    int pixelSnappedLogicalRight() const { return ceilf(logicalRight()); }

Hm, you want to cast to int, no? and isn't a ceilf missing in the pixelSnappedLogicalLeft() function?

[Build Bot]

Attachment 82488 [details] did not build on win:
Build output: http://queues.webkit.org/results/7912899

[Nikolas Zimmermann]

(In reply to comment #40)
> (From update of attachment 82488 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82488&action=review
> 
> Great, looking forward to this patch! I have another patch in the works, eliminating SVG pixel test diffs between 10.6.6 32bit/64bit machines, that will probably change enclosingIntRect, we might want to discuss that, to avoid several rebaselines...
> 
> > Source/WebCore/rendering/InlineBox.h:244
> > +    int pixelSnappedLogicalLeft() const { return logicalLeft(); }
> > +    int pixelSnappedLogicalRight() const { return ceilf(logicalRight()); }

I wonder wheter we want to expose FloatRect's safeFloatToInt method here?

[Dave Hyatt]

(In reply to comment #40)
> (From update of attachment 82488 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82488&action=review
> 
> Great, looking forward to this patch! I have another patch in the works, eliminating SVG pixel test diffs between 10.6.6 32bit/64bit machines, that will probably change enclosingIntRect, we might want to discuss that, to avoid several rebaselines...
> 
> > Source/WebCore/rendering/InlineBox.h:244
> > +    int pixelSnappedLogicalLeft() const { return logicalLeft(); }
> > +    int pixelSnappedLogicalRight() const { return ceilf(logicalRight()); }
> 
> Hm, you want to cast to int, no? and isn't a ceilf missing in the pixelSnappedLogicalLeft() function?

I should perhaps rename these methods.  They're about returning the enclosing left and right (same as what enclosingIntRect would do to a rectangle that included these as the left and right sides).

"enclosingLogicalLeft" sounded dumb though when I tried it. :)

Basically the left side is floored and the right side is ceilinged so that a bounding box or overflow will enclose the whole run.

[Dave Hyatt]

Created attachment 82502 [details]
Update Windows since someone added new uses of string truncation.

[Eric Seidel (no email)]

Curious.  do you expect this to be a noticeable effect on memory usage for memory-constrained ports?

[Dave Hyatt]

(In reply to comment #45)
> Curious.  do you expect this to be a noticeable effect on memory usage for memory-constrained ports?

Shouldn't be... it's just int -> float not int -> double.

[Build Bot]

Attachment 82502 [details] did not build on win:
Build output: http://queues.webkit.org/results/7919033

[Eric Seidel (no email)]

(In reply to comment #46)
> (In reply to comment #45)
> > Curious.  do you expect this to be a noticeable effect on memory usage for memory-constrained ports?
> 
> Shouldn't be... it's just int -> float not int -> double.

Ah yes, my mistake.

[Dave Hyatt]

Created attachment 82533 [details]
Update for Windows again since code got added to WebKit2

Comment on attachment 82502 [details]
Update Windows since someone added new uses of string truncation.

r+ if you resolve the Windows build issue (or if there is no issue)

[Dave Hyatt]

Landed in r78846.

[WebKit Review Bot]

http://trac.webkit.org/changeset/78846 might have broken EFL Linux Release (Build)

[Marc-Antoine Ruel]

Comment on attachment 82533 [details]
Update for Windows again since code got added to WebKit2

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

> Source/WebCore/rendering/InlineBox.h:243
> +    int pixelSnappedLogicalLeft() const { return logicalLeft(); }

This code generates:
warning C4244: 'return' : conversion from 'float' to 'int', possible loss of data

It'd be better to do return static_cast<int>(logicalLeft()); to be clear about the intent.

There are warnings at line 245, 246, 249, 254, 256 and 266 (line numbers are slightly offset, I'm at r84935.