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.
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.
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.
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.
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.
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.
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.
(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.
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?
(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?
(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.
(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.
(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.
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)
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.
2011-02-10 14:02 PST, Dave Hyatt
2011-02-10 15:06 PST, Dave Hyatt
2011-02-10 15:34 PST, Dave Hyatt
2011-02-11 11:30 PST, Dave Hyatt
2011-02-11 12:40 PST, Dave Hyatt
2011-02-11 14:40 PST, Dave Hyatt
2011-02-11 15:05 PST, Dave Hyatt
2011-02-11 15:29 PST, Dave Hyatt
2011-02-13 17:14 PST, Dave Hyatt
2011-02-14 10:53 PST, Dave Hyatt
2011-02-14 11:52 PST, Dave Hyatt
2011-02-14 13:19 PST, Dave Hyatt
2011-02-14 13:20 PST, Dave Hyatt
2011-02-15 11:01 PST, Dave Hyatt
2011-02-15 12:20 PST, Dave Hyatt
2011-02-15 15:16 PST, Dave Hyatt