Bug 107208 – LayoutUnit should round half consistently, not away from zero

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

[Levi Weintraub]

The current implementation of the LayoutUnit::round() method rounds away from zero. Particularly when dealing with Layers, when we round the translate to the graphics context and pass only the sub-pixel fraction to the rendering tree, this leads to bad behavior.

[Levi Weintraub]

Created attachment 183334 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
LayoutTests/platform/chromium-linux/fast/regions/overflow-size-change-in-variable-width-regions-expected.png:0:  Have to enable auto props in the subversion config file (/home/alancutter/.subversion/config "enable-auto-props = yes"). Have to set the svn:mime-type in the subversion config file (/home/alancutter/.subversion/config "*.png = svn:mime-type=image/png").  [image/png] [5]
Total errors found: 1 in 10 files


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

[Eric Seidel (no email)]

Comment on attachment 183334 [details]
Patch

I assume this makes us more like the pre-subpixel behavior?

[Build Bot]

Comment on attachment 183334 [details]
Patch

Attachment 183334 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/15949106

New failing tests:
fast/regions/overflow-size-change-with-stacking-context.html
fast/regions/overflow-size-change-in-variable-width-regions.html
fast/regions/overflow-moving-below-floats-in-variable-width-regions.html

[Emil A Eklund]

Comment on attachment 183334 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=183334&action=review
> Source/WebCore/platform/LayoutUnit.h:231
> +        return saturatedSubtraction(rawValue(), 1 + kEffectiveFixedPointDenominator / 2) / kEffectiveFixedPointDenominator;

For non-saturated you do 1 + value - (denominator/2) yet for saturated you do value - 1 + (denominator/2). How about doing the operations in the same order for both?

return (saturatedSubtraction(rawValue(), kEffectiveFixedPointDenominator / 2) + 1) / kEffectiveFixedPointDenominator;
and
return (m_value - (kEffectiveFixedPointDenominator / 2) + 1) / kEffectiveFixedPointDenominator;

perhaps?

> Source/WebCore/rendering/RenderLayer.cpp:-3655
> -        if (this == localPaintingInfo.rootLayer)

Nice! Glad to see this go!

Also, you'll need to update Tools/TestWebKitAPI/Tests/WebCore/LayoutUnit.cpp

[Emil A Eklund]

Created attachment 183354 [details]
Suggested api test changes

[Build Bot]

Comment on attachment 183334 [details]
Patch

Attachment 183334 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15943286

New failing tests:
fast/regions/overflow-size-change-with-stacking-context.html
fast/regions/overflow-size-change-in-variable-width-regions.html
fast/regions/overflow-moving-below-floats-in-variable-width-regions.html

[Build Bot]

Comment on attachment 183334 [details]
Patch

Attachment 183334 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15940471

New failing tests:
fast/regions/overflow-size-change-with-stacking-context.html
fast/regions/overflow-size-change-in-variable-width-regions.html
fast/regions/overflow-moving-below-floats-in-variable-width-regions.html

[Levi Weintraub]

Created attachment 183510 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 183510 [details]
Patch for landing

Rejecting attachment 183510 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
xpectations:1286:  Path does not exist.  [test/expectations] [5]
Total errors found: 6 in 2 files

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/platform/chromium/TestExpectat..." exit_code: 1
s:1286:  expecting "[", "#", or end of line instead of "fast/regions/overflow-size-change-with-stacking-context.html"  [test/expectations] [5]
LayoutTests/platform/mac/TestExpectations:1286:  Path does not exist.  [test/expectations] [5]
Total errors found: 6 in 2 files

Full output: http://queues.webkit.org/results/15941771

[Levi Weintraub]

Created attachment 183521 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 183521 [details]
Patch for landing

Clearing flags on attachment: 183521

Committed r140192: <http://trac.webkit.org/changeset/140192>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.