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.
Created attachment 82044 [details] Patch (don't really review, just catching build errors)
Created attachment 82058 [details] Patch that applies (ignore for review, just testing builds)
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.
Created attachment 82062 [details] Patch that applies (ignore for review, just testing builds)
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.
Attachment 82058 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7884413
Attachment 82062 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7869499
Attachment 82058 [details] did not build on win: Build output: http://queues.webkit.org/results/7869503
Attachment 82062 [details] did not build on win: Build output: http://queues.webkit.org/results/7869507
Attachment 82062 [details] did not build on qt: Build output: http://queues.webkit.org/results/7884461
Attachment 82062 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7870574
<rdar://problem/8891123>
Attachment 82062 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7869566
Created attachment 82153 [details] Patch that applies (ignore for review, just testing builds)
Attachment 82153 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7900003
Created attachment 82161 [details] Patch that applies (ignore for review, just testing builds)
Attachment 82153 [details] did not build on win: Build output: http://queues.webkit.org/results/7893014
Attachment 82153 [details] did not build on qt: Build output: http://queues.webkit.org/results/7870746
Attachment 82161 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7870748
Attachment 82161 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7870754
Attachment 82161 [details] did not build on win: Build output: http://queues.webkit.org/results/7869739
Attachment 82161 [details] did not build on qt: Build output: http://queues.webkit.org/results/7867750
Created attachment 82181 [details] Patch that applies (ignore for review, just testing builds)
Attachment 82181 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7867766
Created attachment 82187 [details] Patch that applies (ignore for review, just testing builds)
Attachment 82187 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7867771
Created attachment 82194 [details] Patch that applies (ignore for review, just testing builds)
Attachment 82194 [details] did not build on win: Build output: http://queues.webkit.org/results/7884715
Created attachment 82282 [details] Patch that applies (ignore for review, just testing builds)
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.
Created attachment 82357 [details] Sample of layout tests changes
Attachment 82347 [details] did not build on win: Build output: http://queues.webkit.org/results/7909694
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.
This patch will also fix bug 14956, at least for LTR content.
(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.
Created attachment 82488 [details] Address Dan's comments.
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?
Attachment 82488 [details] did not build on win: Build output: http://queues.webkit.org/results/7912899
(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.
Created attachment 82502 [details] Update Windows since someone added new uses of string truncation.
Curious. do you expect this to be a noticeable effect on memory usage for memory-constrained ports?
(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.
Attachment 82502 [details] did not build on win: Build output: http://queues.webkit.org/results/7919033
(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.
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)
Landed in r78846.
http://trac.webkit.org/changeset/78846 might have broken EFL Linux Release (Build)
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.