Bug 22759

Created attachment 25891 [details] reduced testcase

Following calculations calculate the expected results for the reduction HTML file attached to the issue. Root ListItem vertical size, "RLITotal" = 0.6em top padding + text line height + 0.7em bottom padding, where Text line height = 16px * 70/100 * 1.5 = 16.8 So "RLITotal" = 0.6 * 16 * 70 / 100 + 16.8 + 0.7 * 16 * 70 / 100 = 6.72 + 16.8 + 7.84 = 31.36 The absolute positioned popup list's vertical position = 2.8em = 2.8 * 16 * 70 /100 = 31.36 Though there is no gap between root element and popup, in Chrome we see some gap. What happens inside code during parsing the HTML file is that floating pointing number gets converted to integer without rounding off. So 6.72 becomes 6, 7.84 becomes 7 and 16.8 becomes 16 resulting in total for "RLITotal" as 29. And absolute vertical location for popup, 31.36 becomes 31. Clearly two pixel gap results because of this rounding off behaviour. Inside following function, int computedLineHeight() const { Length lh = lineHeight(); // Negative value means the line height is not set. Use the font's built-in spacing. if (lh.isNegative()) return font().lineSpacing(); if (lh.isPercent()) return lh.calcMinValue(fontSize(), true); return lh.value(); } The line, "return lh.calcMinValue(fontSize(), true);" was changed by adding true as second parameter, which lets the calcMinValue to round off the result to nearest integer. So in above example 16.82 becomes 17. But that will not be sufficient because we'll still have 1 pixel gap. So adding 0.5 to result before casting it to integer in the following function fixes the issue. This function is a generic function used during HTML/CSS file parsing. int CSSPrimitiveValue::computeLengthIntForLength(RenderStyle* style, double multiplier) { double result = computeLengthDouble(style, multiplier); // This conversion is imprecise, often resulting in values of, e.g., 44.99998. We // need to go ahead and round if we're really close to the next integer value. result += result < 0 ? -0.01 : +0.01; if (result > intMaxForLength || result < intMinForLength) return 0; return static_cast<int>(result); } I do not know why only 0.01 was added above, what happens if we add 0.5 is to be understood. I am not sure how to work further on this issue to take it to some sort of conclusion. Any help would be appreciated.

Created attachment 73438 [details] Patch

Comment on attachment 73438 [details] Patch This is based on the solution outlined by tvk in https://bugs.webkit.org/show_bug.cgi?id=22759#c2 It looks like a bunch of layout tests will need to be rebaselined. The standard procedure is to mark all the failing tests in the test_expectations files and then run the rebaselining script as described in http://trac.webkit.org/wiki/Rebaseline ? This is my first webkit patch so I want to make sure I do everything right.

Comment on attachment 73438 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=73438&action=review > WebCore/css/CSSPrimitiveValue.h:46 > - value += (value < 0) ? -0.01 : +0.01; > + value += (value < 0) ? -0.5 : +0.5; I’m pretty sure this is not acceptable. We discussed this when originally writing the code here and tried it and saw problems. Did you run regression tests? What tests did this affect? By the way, if we *do* want this rounding, then I think we want to use the appropriate rounding function, roundf or round, rather than rolling our own with +=.

Comment on attachment 73438 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=73438&action=review >> WebCore/css/CSSPrimitiveValue.h:46 >> + value += (value < 0) ? -0.5 : +0.5; > > I’m pretty sure this is not acceptable. We discussed this when originally writing the code here and tried it and saw problems. Did you run regression tests? What tests did this affect? > > By the way, if we *do* want this rounding, then I think we want to use the appropriate rounding function, roundf or round, rather than rolling our own with +=. There were ~170 unexpected failures. Most fail because of pixel differences unrelated to the tests' substance or are improvements on the existing results. Two failing tests were on point though: svg/custom/fractional-rects and fast/dom/length-attribute-mapping. They both expect lengths specified as floats to be rounded down. Firefox seems to agree with them. I'm going to work on this a bit more to get those two tests to pass along with the one I'm introducing here. Thanks for the feedback. On round(f): agreed.

Created attachment 76229 [details] patch Don't round lengths if specified units are pixels. Round everything else. I think this is more correct than the current behaviour but it moves a lot of stuff by 1 pixel. Including h2 and h3 elements that are seemingly ubiquitous in pixel tests, of which approximately 200 are affected. I've looked at all of them except some of the mozilla table tests. None indicate that this patch breaks anything, they're all either improvements (minority) or noise (majority).

Attachment 76229 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/css1/units/rounding-expected.txt', u'LayoutTests/css1/units/rounding.html', u'WebCore/css/CSSPrimitiveValue.cpp']" exit_code: 1 WebCore/css/CSSPrimitiveValue.cpp:42: Alphabetical sorting problem. [build/include_order] [4] WebCore/css/CSSPrimitiveValue.cpp:435: rounded_result is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] WebCore/css/CSSPrimitiveValue.cpp:438: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 3 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 76248 [details] patch Don't round lengths if specified units are pixels. Round everything else. I think this is more correct than the current behaviour but it moves a lot of stuff by 1 pixel. Including h2 and h3 elements that are seemingly ubiquitous in pixel tests, of which approximately 200 are affected. I've looked at all of them except some of the mozilla table tests. None indicate that this patch breaks anything, they're all either improvements (minority) or noise (majority). (same substance as attachment 76229 [details], fixed style errors)

Comment on attachment 76248 [details] patch Thanks for the patch, but all patches need a ChangeLog. See http://webkit.org/coding/contributing.html

Created attachment 76255 [details] patch Don't round lengths if specified units are pixels. Round everything else. I think this is more correct than the current behaviour but it moves a lot of stuff by 1 pixel. Including h2 and h3 elements that are seemingly ubiquitous in pixel tests, of which approximately 200 are affected. I've looked at all of them except some of the mozilla table tests. None indicate that this patch breaks anything, they're all either improvements (minority) or noise (majority). (same substance as attachment 76248 [details], now includes changelog)

(In reply to comment #5) > (From update of attachment 73438 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=73438&action=review > > > WebCore/css/CSSPrimitiveValue.h:46 > > - value += (value < 0) ? -0.01 : +0.01; > > + value += (value < 0) ? -0.5 : +0.5; > > I’m pretty sure this is not acceptable. We discussed this when originally writing the code here and tried it and saw problems. Did you run regression tests? What tests did this affect? Darin A, who might remember what those problems were? I didn't notice anything clearly problematic in the layout tests when they were run with the most recent patch. (I added you to the cc list of this bug because I'm not sure if you'll see this message otherwise. Apologies in advance if that's a faux pas.)

(In reply to comment #12) > (I added you to the cc list of this bug because I'm not sure if you'll see this message otherwise. Apologies in advance if that's a faux pas.) It’s always fine to cc me on any bug.

(In reply to comment #12) > (In reply to comment #5) > > > WebCore/css/CSSPrimitiveValue.h:46 > > > - value += (value < 0) ? -0.01 : +0.01; > > > + value += (value < 0) ? -0.5 : +0.5; > > > > I’m pretty sure this is not acceptable. We discussed this when originally writing the code here and tried it and saw problems. Did you run regression tests? What tests did this affect? > > Darin A, who might remember what those problems were? I didn't notice anything clearly problematic in the layout tests when they were run with the most recent patch. Not sure. Maybe Hyatt, maybe Mitz. Sorry I can’t remember.

Comment on attachment 76255 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=76255&action=review Change looks fine, but review- for a slight bit of unneeded work due to the way the code is structured. > WebCore/css/CSSPrimitiveValue.cpp:439 > + double roundedResult = result; > + if (shouldRound) > + roundedResult = round(result); > if (!applyZoomMultiplier || multiplier == 1.0) > - return result; > - > + return roundedResult; Why compute roundedResult outside the if statement where it’s being returned? That seems like extra work for no benefit. I would write: if (!applyZoomMultiplier || multiplier == 1.0) return shouldRound ? round(result) : result; Or the equivalent with two if statements.

Comment on attachment 76255 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=76255&action=review >> WebCore/css/CSSPrimitiveValue.cpp:439 >> + return roundedResult; > > Why compute roundedResult outside the if statement where it’s being returned? That seems like extra work for no benefit. I would write: > > if (!applyZoomMultiplier || multiplier == 1.0) > return shouldRound ? round(result) : result; > > Or the equivalent with two if statements. Agreed. It was an artifact of some debug code.

Created attachment 78083 [details] Patch updated to address Darin's comment.

Created attachment 84112 [details] Patch

Comment on attachment 84112 [details] Patch Ping and patch updated to latest code

Comment on attachment 78083 [details] Patch updated to address Darin's comment. It looks like a more recent version has been uploaded.

Comment on attachment 84112 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=84112&action=review > Source/WebCore/css/CSSPrimitiveValue.cpp:450 > double zoomedResult = result * multiplier; I'm not the right person to review this but speaking as someone who may read this code in the future, I find this patch confusing. Specifically, there is a variable "shouldRound" and it isn't followed in all return statements. Is it a bug that this isn't done on on the final return "return zoomedResult;"?

Comment on attachment 84112 [details] Patch I agree with levin. We need to make thsi consistent.

This is fixed in the chromium port because it uses subpixel layout.

> This is fixed in the chromium port because it uses subpixel layout. But the reduction still fails in WebKit ToT, because it doesn't. Not sure if the bug needs to be reopened though, because the site changed, and works fine now.