Bug 124784

Created attachment 217909 [details] Patch Apply LayoutUnit::fromFloatCeil() consistently in RectangleShape::firstIncludedIntervalLogicalTop().

Comment on attachment 217909 [details] Patch r=me

Comment on attachment 217909 [details] Patch Clearing flags on attachment: 217909 Committed r159821: <http://trac.webkit.org/changeset/159821>

All reviewed patches have been landed. Closing bug.

The committed test case fails when subpixel layout is enabled. By looking at the expected test result, it seems that 1.3px is supposed to get rounded to 2px. However, with subpixel on, it's never the case. ceiledLayoutUnit() ceils to layout unit value and not to CSS pixel value (unless the 2 units are equal which is the case when subpixel is off) subpixel off: float 1.3 -> ceilLayoutUnit() -> ceil(1.3 * 1) -> 2 layout unit value -> 2/1 -> 2px CSS pixel value subpixel on: float 1.3 -> ceilLayoutUnit() -> ceil(1.3 * 64) -> ceil(83.2) -> 84 layout unit value -> 84 / 64 -> 1.312 -> snapped to 1px at painting time; If the intention is really to convert 1.3px -> 2px during layout (which we tend to avoid as it defeats the purpose of using subpixel values), then you need to come up with a function that ceils your float value to CSS px. We don't have such function yet and even the existing snapping functions (roundToInt(), floorToInt()) are mostly used at painting time. I don't know enough about shapes to tell whether ceiling the layout unit is wrong, but in general it's the opposite direction of what we do at other places. (Also, if the return value is expect to be ceiled, it's better to actually ceil right before returning as opposed to ceil earlier. Early conversion could have some unwanted side effect on other values)

Created attachment 221497 [details] Patch

The patch assumes that we don't actually need to ceil the float value to CSS px at layout time.

Comment on attachment 221497 [details] Patch Not sure I am qualified to review this. Maybe Simon Fraser or another layout expert needs to (Hyatt?).

(In reply to comment #8) > (From update of attachment 221497 [details]) > Not sure I am qualified to review this. Maybe Simon Fraser or another layout expert needs to (Hyatt?). Thanks Darin. I should probably ping some Adobe/shapes guy to see which way they wanna go.

(In reply to comment #7) > The patch assumes that we don't actually need to ceil the float value to CSS px at layout time. The use LayoutUnit:::fromFloatCeil(f) was to round up to the next layout unit, not a mistaken way to compute ceil(f). In the line that determines the result of this test, float minY = LayoutUnit::fromFloatCeil(std::max(bounds.y(), minIntervalTop)); the objective is to ensure that the result is within the rectangle. Since minIntervalTop is already a LayoutUnit, this could have been written: float minY = std::max(LayoutUnit::fromFloatCeil(bounds.y()), minIntervalTop)); I think the ...test.html should contain: #shape-inside { -webkit-shape-inside: rectangle(0px, 1.3px, 200px, 200px); font: 100px/1 Ahem, sans-serif; -webkit-font-smoothing: antialiased; color: green; background-color: grey; background-clip: padding-box; width: 200px; height: 200px; } And the ...test-expected.html should be: #shape-inside { font: 100px/1 Ahem, sans-serif; -webkit-font-smoothing: antialiased; color: green; background-color: grey; background-clip: padding-box; width: 200px; height: 198.7px; padding-top: 1.3px; } This produces an off-by one when subpixel layout is turned off, because the padding-top value causes the single Ahem character to be rendered at 1, but our "keep it within the rectangle" code puts it at 2, as you've pointed out. I'll be happy to fix this, roughly along the lines you've suggested, if you want.

(In reply to comment #10) > (In reply to comment #7) > > The patch assumes that we don't actually need to ceil the float value to CSS px at layout time. > > The use LayoutUnit:::fromFloatCeil(f) was to round up to the next layout unit, not a mistaken way to compute ceil(f). In the line that determines the result of this test, > > float minY = LayoutUnit::fromFloatCeil(std::max(bounds.y(), minIntervalTop)); > > the objective is to ensure that the result is within the rectangle. Since minIntervalTop is already a LayoutUnit, this could have been written: > > float minY = std::max(LayoutUnit::fromFloatCeil(bounds.y()), minIntervalTop)); > > I think the ...test.html should contain: > > #shape-inside { > -webkit-shape-inside: rectangle(0px, 1.3px, 200px, 200px); > font: 100px/1 Ahem, sans-serif; > -webkit-font-smoothing: antialiased; > color: green; > background-color: grey; > background-clip: padding-box; > width: 200px; > height: 200px; > } > > And the ...test-expected.html should be: > > #shape-inside { > font: 100px/1 Ahem, sans-serif; > -webkit-font-smoothing: antialiased; > color: green; > background-color: grey; > background-clip: padding-box; > width: 200px; > height: 198.7px; > padding-top: 1.3px; > } > > This produces an off-by one when subpixel layout is turned off, because the padding-top value causes the single Ahem character to be rendered at 1, but our "keep it within the rectangle" code puts it at 2, as you've pointed out. > > I'll be happy to fix this, roughly along the lines you've suggested, if you want. So it sounds like all it's needed is just to fine tune the expected result in my patch to go from 199px to 198.7px and 1px to 1.3px, which I should have done the first place considering the layout correctness vs. rendering correctness (layout is not css px snapped while rendering is) even though they produce the same result at this point. I'll do that.

(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #7) > So it sounds like all it's needed is just to fine tune the expected result in my patch to go from 199px to 198.7px and 1px to 1.3px, which I should have done the first place considering the layout correctness vs. rendering correctness (layout is not css px snapped while rendering is) even though they produce the same result at this point. I'll do that. Minor nit also: there's an extraneous "divX.offsetHeight;" line in the updated ...-expected.html test.

(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #10) > > > (In reply to comment #7) > > > So it sounds like all it's needed is just to fine tune the expected result in my patch to go from 199px to 198.7px and 1px to 1.3px, which I should have done the first place considering the layout correctness vs. rendering correctness (layout is not css px snapped while rendering is) even though they produce the same result at this point. I'll do that. > > Minor nit also: there's an extraneous "divX.offsetHeight;" line in the updated ...-expected.html test. Sorry, I also meant to say: yes that sounds like all that's needed.

(In reply to comment #12) > (In reply to comment #11) > > (In reply to comment #10) > > > (In reply to comment #7) > > > So it sounds like all it's needed is just to fine tune the expected result in my patch to go from 199px to 198.7px and 1px to 1.3px, which I should have done the first place considering the layout correctness vs. rendering correctness (layout is not css px snapped while rendering is) even though they produce the same result at this point. I'll do that. > > Minor nit also: there's an extraneous "divX.offsetHeight;" line in the updated ...-expected.html test. That's there to trigger sync layout.

Created attachment 221827 [details] Patch

This is blocked by bug 127404. The expected test case goes through the SimpleLineLayout code path, which needs some extra rounding. I'll cq the patch soon after I have a fix for the blocking bug. (the expected test case needs the '2px' adjustment as the 1.3px padding gets floored to 1px as the result of integer arithmetic. normal 'subpixel off' padding/margin/border behavior)

Comment on attachment 221827 [details] Patch Attachment 221827 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6427164685631488 New failing tests: fast/shapes/shape-inside/shape-inside-subpixel-rectangle-top.html

Created attachment 221866 [details] Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-13 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5

(In reply to comment #17) > (From update of attachment 221827 [details]) > Attachment 221827 [details] did not pass mac-wk2-ews (mac-wk2): > Output: http://webkit-queues.appspot.com/results/6427164685631488 > > New failing tests: > fast/shapes/shape-inside/shape-inside-subpixel-rectangle-top.html bug 127404

Comment on attachment 221827 [details] Patch Attachment 221827 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5153810498453504 New failing tests: fast/shapes/shape-inside/shape-inside-subpixel-rectangle-top.html

Created attachment 221867 [details] Archive of layout-test-results from webkit-ews-01 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.5

(In reply to comment #16) > This is blocked by bug 127404. The expected test case goes through the SimpleLineLayout code path, which needs some extra rounding. I'll cq the patch soon after I have a fix for the blocking bug. > > (the expected test case needs the '2px' adjustment as the 1.3px padding gets floored to 1px as the result of integer arithmetic. normal 'subpixel off' padding/margin/border behavior) Excellent, I'd noticed this as well. Glad to hear that a fix is coming.

Comment on attachment 221827 [details] Patch Attachment 221827 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6262856416755712 New failing tests: fast/shapes/shape-inside/shape-inside-subpixel-rectangle-top.html

Created attachment 221869 [details] Archive of layout-test-results from webkit-ews-04 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-04 Port: mac-mountainlion Platform: Mac OS X 10.8.5

Comment on attachment 221827 [details] Patch Rejecting attachment 221827 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 221827, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: cceeded at 1 with fuzz 3. patching file LayoutTests/fast/shapes/shape-inside/shape-inside-subpixel-rectangle-top-expected.html patching file LayoutTests/platform/mac/TestExpectations Original content of LayoutTests/platform/mac/fast/shapes/shape-inside/shape-inside-subpixel-rectangle-top-expected.png mismatches at /Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply line 265. Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.appspot.com/results/4686125402685440

Committed r162559: <http://trac.webkit.org/changeset/162559>