Bug 95883

Created attachment 162314 [details] Patch

Comment on attachment 162314 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162314&action=review Seems like another step in the right direction, but we need to catch all the cases where we do this. > Source/WebCore/ChangeLog:10 > + No new tests, covered by existing tests if SATURATED_LAYOUT_ARITHMETIC is enabled. Can we not write a test case otherwise? Should it be gated by the same flag? > Source/WebCore/ChangeLog:14 > + (WebCore::snapSizeToPixel): Change to use long longs and roundRawValue. I'm a little concerned about performance here given we perform this operation a lot. > Source/WebCore/platform/FractionalLayoutUnit.h:798 > +inline long long roundRawValue(long long value) > +{ > + if (value > 0) > + return (value + (kFixedPointDenominator / 2)) / kFixedPointDenominator; > + return (value - (kFixedPointDenominator / 2)) / kFixedPointDenominator; > +} We need to change pixelSnappedIntRect (and anywhere else we may be implicitly doing this) to use this logic to be consistent.

Created attachment 162587 [details] Patch

Comment on attachment 162587 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162587&action=review > Source/WebCore/platform/FractionalLayoutUnit.h:238 > - m.m_value = std::numeric_limits<int>::max(); > + m.m_value = std::numeric_limits<int>::max() - kFixedPointDenominator / 2; This is confusing without a comment or named constant. > Source/WebCore/platform/FractionalLayoutUnit.h:244 > - m.m_value = std::numeric_limits<int>::min(); > + m.m_value = std::numeric_limits<int>::min() + kFixedPointDenominator / 2; Ditto.

Created attachment 162591 [details] Patch

Comment on attachment 162591 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162591&action=review > Source/WebCore/platform/FractionalLayoutUnit.h:239 > + // Reduce max value by kFixedPointDenominator / 2 to prevent max().round() from overflowing. > + m.m_value = std::numeric_limits<int>::max() - kFixedPointDenominator / 2; So there's some weird things to think about here... Now it's possible that FractionalLayoutUnit::max() + (1.0f / 60) > FractionalLayoutUnit::max()! Also, without actually double-checking the math, might it also be possible that max().round() > max()?

(In reply to comment #6) > (From update of attachment 162591 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=162591&action=review > > > Source/WebCore/platform/FractionalLayoutUnit.h:239 > > + // Reduce max value by kFixedPointDenominator / 2 to prevent max().round() from overflowing. > > + m.m_value = std::numeric_limits<int>::max() - kFixedPointDenominator / 2; > > So there's some weird things to think about here... Now it's possible that FractionalLayoutUnit::max() + (1.0f / 60) > FractionalLayoutUnit::max()! Also, without actually double-checking the math, might it also be possible that max().round() > max()? max() and max().round() have different types so that shouldn't matter.

(In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 162591 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=162591&action=review > > > > > Source/WebCore/platform/FractionalLayoutUnit.h:239 > > > + // Reduce max value by kFixedPointDenominator / 2 to prevent max().round() from overflowing. > > > + m.m_value = std::numeric_limits<int>::max() - kFixedPointDenominator / 2; > > > > So there's some weird things to think about here... Now it's possible that FractionalLayoutUnit::max() + (1.0f / 60) > FractionalLayoutUnit::max()! Also, without actually double-checking the math, might it also be possible that max().round() > max()? > > max() and max().round() have different types so that shouldn't matter. But FractionalLayoutUnit::max() + FractionalLayoutUnit::epsilon() do

(In reply to comment #8) > But FractionalLayoutUnit::max() + FractionalLayoutUnit::epsilon() do Uhm, no. epsilon returns a float and max returns a FractionalLayoutUnit. Either way I get what you are saying that max and min no longer represents the true maximum and minimum values but this seems like a better option than manually subtracting from the value when using them.

Comment on attachment 162591 [details] Patch Attachment 162591 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13776734 New failing tests: media/video-zoom-controls.html

Created attachment 162785 [details] Patch

Comment on attachment 162785 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162785&action=review > Source/WebCore/css/CSSPrimitiveValue.cpp:470 > + return Length(static_cast<float>(value > intMaxForLayoutUnit || value < intMinForLayoutUnit ? 0.0 : value), Fixed); This feels like a helper function. :) > LayoutTests/platform/chromium/fast/css/large-number-round-trip-expected.txt:1 > +PASS: read 90010000px back as 0px, read again as 0px What was our old behavior?

(In reply to comment #12) > (From update of attachment 162785 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=162785&action=review > > > Source/WebCore/css/CSSPrimitiveValue.cpp:470 > > + return Length(static_cast<float>(value > intMaxForLayoutUnit || value < intMinForLayoutUnit ? 0.0 : value), Fixed); > > This feels like a helper function. :) It does but it is only used in a single place right now. > > LayoutTests/platform/chromium/fast/css/large-number-round-trip-expected.txt:1 > > +PASS: read 90010000px back as 0px, read again as 0px > > What was our old behavior? computed style would return 90010000px but offsetWidth would not.

ping?

Created attachment 162872 [details] Patch

Comment on attachment 162872 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162872&action=review > Source/WebCore/platform/FractionalLayoutUnit.h:249 > + static const FractionalLayoutUnit safeMax() I think we could come up with a better name than "safeMax". Maybe "nearlyMax" or "almostMax"? > Source/WebCore/platform/graphics/FractionalLayoutRect.h:170 > + return FractionalLayoutRect(FractionalLayoutUnit::safeMin() / 2, FractionalLayoutUnit::safeMin() / 2, FractionalLayoutUnit::safeMax(), FractionalLayoutUnit::safeMax()); we should document here why this uses "safeMax", something like: infiniteRect is slightly smaller than INT_MAX, INT_MAX, to allow pixelSnapping to round up to the nearest value w/o overflowing.

Created attachment 162884 [details] Patch for landing

Comment on attachment 162884 [details] Patch for landing Clearing flags on attachment: 162884 Committed r127933: <http://trac.webkit.org/changeset/127933>

All reviewed patches have been landed. Closing bug.