Bug 97265 – snapToSize rounds the incorrectly for negative locations

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

[Emil A Eklund]

In snapToSize we use the fraction of the location rather than the full location when adjusting the width. This results in the sign being lost which in turn affects rounding.


Downstream chromium bug: http://code.google.com/p/chromium/issues/detail?id=150802

[Emil A Eklund]

Created attachment 165002 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 165002 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165002&action=review

> Source/WebCore/platform/FractionalLayoutUnit.h:810
> +    FractionalLayoutUnit fraction = (location - location.floor()) * (location < 0 ? -1 : 1);

I this the most elegant way to do this?  It seems you're undoing floor's magic, which presumably ignores sign.

[Emil A Eklund]

(In reply to comment #2)
> (From update of attachment 165002 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=165002&action=review
> 
> > Source/WebCore/platform/FractionalLayoutUnit.h:810
> > +    FractionalLayoutUnit fraction = (location - location.floor()) * (location < 0 ? -1 : 1);
> 
> I this the most elegant way to do this?  It seems you're undoing floor's magic, which presumably ignores sign.

I'm sure it is not but it is the best I've managed to come up with so far.
If you have any suggestions I'm all ears.

[Levi Weintraub]

Comment on attachment 165002 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165002&action=review

>>> Source/WebCore/platform/FractionalLayoutUnit.h:810
>>> +    FractionalLayoutUnit fraction = (location - location.floor()) * (location < 0 ? -1 : 1);
>> 
>> I this the most elegant way to do this?  It seems you're undoing floor's magic, which presumably ignores sign.
> 
> I'm sure it is not but it is the best I've managed to come up with so far.
> If you have any suggestions I'm all ears.

Isn't the fraction just location.rawValue() % kFixedPointDenominator?

[WebKit Review Bot]

Comment on attachment 165002 [details]
Patch

Attachment 165002 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13932472

New failing tests:
compositing/reflections/reflection-in-composited.html
fast/writing-mode/Kusa-Makura-background-canvas.html
css3/filters/nested-filter.html
media/video-zoom-controls.html
fast/multicol/vertical-lr/float-multicol.html

[Emil A Eklund]

Created attachment 165195 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 165195 [details]
Patch

LGTM.

[Eric Seidel (no email)]

Comment on attachment 165195 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165195&action=review

> Source/WebCore/platform/FractionalLayoutUnit.h:811
> +    fraction.setRawValue(location.rawValue() % kFixedPointDenominator);

A comment about why we do it this way might help, but this is also OK.

[Emil A Eklund]

Created attachment 165199 [details]
Patch for landing

[Emil A Eklund]

Thanks Eric. Added a comment explaining why we do it this way.

[WebKit Review Bot]

Comment on attachment 165199 [details]
Patch for landing

Rejecting attachment 165199 [details] from commit-queue.

New failing tests:
WebFilterOperationsTest.saveAndRestore
media/video-zoom-controls.html
Full output: http://queues.webkit.org/results/13956808

[Emil A Eklund]

Committed r129370: <http://trac.webkit.org/changeset/129370>