When computing inline preferred logical widths, we accumulate our children's widths using floating point numbers, but when adding the width of floating elements, we can truncate up to 1/60th of a pixel. We should be using the nearest larger float instead of the nearest smaller one. You can see the result of this wrapping in the top-nav bar on developer.apple.com.
Created attachment 157338 [details] Patch
Ping.
Interesting problem, but I think we might want to consider if we want to solve this instead by changing the fixed-point divisor to 64. By using a power of 2, it is not only much faster, but it means we will never loose precision when converting to float. From a API point of view it is just much less confusing if we can safely assume that float will be more precise than fixed points (for most of the used range anyway).
(In reply to comment #3) > Interesting problem, but I think we might want to consider if we want to solve this instead by changing the fixed-point divisor to 64. By using a power of 2, it is not only much faster, but it means we will never loose precision when converting to float. > > From a API point of view it is just much less confusing if we can safely assume that float will be more precise than fixed points (for most of the used range anyway). 60 was used because it's easily dived by 2, 3, 4, 5, 6, and 10. I'm not opposed to changing it, but we shouldn't write code that's dependent upon the divisor (it could be changed later), and I believe we'd want to do some testing with 64 before settling on it.
(In reply to comment #4) > 60 was used because it's easily dived by 2, 3, 4, 5, 6, and 10. I'm not opposed to changing it, but we shouldn't write code that's dependent upon the divisor (it could be changed later), and I believe we'd want to do some testing with 64 before settling on it. I only brought it up because this bug highlighted a consequence I had never considered before of using divisors that are not powers of 2, and I just don't like the fact that we might lose precision in places we didn't consider before. If we want to keep the divisors, but want to ensure we never lose precision like that, another alternative could be get rid of float, but would either require fully enabling subpixel layout, or make fixed point available when explicitly used even without subpixel layout enabled. Btw, it is not that I think there is anything wrong with your solution, this line of thinking it mostly to think of ways to avoid similar problems elsewhere.
(In reply to comment #5) > (In reply to comment #4) > > 60 was used because it's easily dived by 2, 3, 4, 5, 6, and 10. I'm not opposed to changing it, but we shouldn't write code that's dependent upon the divisor (it could be changed later), and I believe we'd want to do some testing with 64 before settling on it. > > I only brought it up because this bug highlighted a consequence I had never considered before of using divisors that are not powers of 2, and I just don't like the fact that we might lose precision in places we didn't consider before. > > If we want to keep the divisors, but want to ensure we never lose precision like that, another alternative could be get rid of float, but would either require fully enabling subpixel layout, or make fixed point available when explicitly used even without subpixel layout enabled. > > Btw, it is not that I think there is anything wrong with your solution, this line of thinking it mostly to think of ways to avoid similar problems elsewhere. Your thoughts are always appreciated, don't worry :) I just wanted to give background on the current value. We'd considered other values, of course, and had particularly figured that 64 could be used for Mobile, where the performance is more critical. Perhaps I'm ignorant, but are you implying that we won't be subject to lost precision if we went with a power of two? It seems to me that when we're accumulating large numbers and switching between fixed and floating point we'll always be subject to lost precision.
(In reply to comment #6) > Perhaps I'm ignorant, but are you implying that we won't be subject to lost precision if we went with a power of two? It seems to me that when we're accumulating large numbers and switching between fixed and floating point we'll always be subject to lost precision. If we use a power of two divisor most numbers that can be represented by our layout units can also be represented accurately by floating point, because floating point always uses divisors of two. Floating point can on the other hand not represent division by 10 accurately, so our layout unit can currently represent 0.1 accurately, which is something even a double can not. Which is why we end up losing accuracy when converting from fixed point to floating point, even for ordinary size numbers. Of course even if we switch to 64 instead of 60, there will be some numbers where fixed point and floating point doesn't match. Fixed point will have better accuracy at higher numbers right up till they overflows, while floating point will have increasingly worse accuracy at higher numbers at the benefit of not overflowing. So we could end up with similar problems like this one for very large numbers, but only in constructed laboratory cases I think.
Comment on attachment 157338 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157338&action=review Looks fine. > Source/WebCore/rendering/RenderBlock.cpp:3750 > + // FIXME: LayoutUnit::epsilon is probably only necessary here due to lost precision elsewhere > + while (logicalRightOffsetForLine(logicalTopOffset, logicalRightOffset, false, &heightRemainingRight) - floatLogicalLeft + LayoutUnit::epsilon() < floatLogicalWidth) { How do we fix/track down this fixme? > Source/WebCore/rendering/RenderBlock.cpp:3765 > + // FIXME: LayoutUnit::epsilon is probably only necessary here due to lost precision elsewhere Same question. What's the plan for fixing this?
Created attachment 158368 [details] Patch for landing
Comment on attachment 158368 [details] Patch for landing Clearing flags on attachment: 158368 Committed r125591: <http://trac.webkit.org/changeset/125591>
All reviewed patches have been landed. Closing bug.