Bug 86065

Created attachment 141105 [details] Patch

Comment on attachment 141105 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141105&action=review Some good catches :) > Source/WebCore/ChangeLog:3 > + FractionalUnit minor math bugs FractionalLayoutUnit :) > Source/WebCore/platform/FractionalLayoutUnit.h:572 > + a = a - b.toFloat(); Is this actually a change in behavior? > Source/WebCore/platform/FractionalLayoutUnit.h:586 > + float value = a.toFloat() * b; > + a = value; > + return a; Why would we want to do something other than the standard overload of float * FractionalLayoutUnit? Why not build this out of the already-defined operators?

(In reply to comment #2) > (From update of attachment 141105 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141105&action=review > > Some good catches :) > > > Source/WebCore/ChangeLog:3 > > + FractionalUnit minor math bugs > > FractionalLayoutUnit :) > Right :D > > Source/WebCore/platform/FractionalLayoutUnit.h:572 > > + a = a - b.toFloat(); > > Is this actually a change in behavior? > No. > > Source/WebCore/platform/FractionalLayoutUnit.h:586 > > + float value = a.toFloat() * b; > > + a = value; > > + return a; > > Why would we want to do something other than the standard overload of float * FractionalLayoutUnit? Why not build this out of the already-defined operators? To avoid the float being cast to fractionallayoutunit before the multiplication, I found this after experimenting with setting the FractionalLayoutUnit(float) constructor to explicit, specifically to see if any floats were converted this way losing precision. It turned out setting the constructor to explicit is not a good idea because it is used implicitly all over the place, but I did find a few places it shouldn't have been used.

Created attachment 141345 [details] Patch

Comment on attachment 141345 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141345&action=review Some good changes here, but some others that I believe are not helpful. > Source/WebCore/platform/FractionalLayoutUnit.h:152 > - static float epsilon() { return 1 / kFixedPointDenominator; } > + static float epsilon() { return 1. / kFixedPointDenominator; } The comment says this should use “float division”, but this will use double-precision division and then round to float. That’s probably OK because it happens at compile time. > Source/WebCore/platform/FractionalLayoutUnit.h:348 > - long long rawVal = static_cast<long long>(a.rawValue()) * b.rawValue() / kFixedPointDenominator; > + long long rawVal = (static_cast<long long>(a.rawValue()) * b.rawValue()) / kFixedPointDenominator; Given my understanding of the C rules for operators and parentheses and order of operations, I believe adding these parentheses has no effect. Please do not add them unless you have evidence I am wrong. > Source/WebCore/platform/FractionalLayoutUnit.h:416 > - long long rawVal = static_cast<long long>(kFixedPointDenominator) * a.rawValue() / b.rawValue(); > + long long rawVal = (static_cast<long long>(kFixedPointDenominator) * a.rawValue()) / b.rawValue(); Ditto. > Source/WebCore/platform/FractionalLayoutUnit.h:572 > - a = a - b; > + a = a - b.toFloat(); I don’t think this change adds value. > Source/WebCore/platform/FractionalLayoutUnit.h:584 > + a = a.toFloat() * b; Why does the explicit cast here yield higher precision? It should not. Please do not make this change unless it’s actually helpful. > Source/WebCore/platform/FractionalLayoutUnit.h:590 > + a = a * b.toFloat(); The explicit call to toFloat here should not be needed.

Comment on attachment 141345 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141345&action=review >> Source/WebCore/platform/FractionalLayoutUnit.h:572 >> + a = a - b.toFloat(); > > I don’t think this change adds value. OK, I guess I was wrong about this one. It seems that without the explicit conversion the math is done by converting the float to a FractionLayoutUnit. I think that it’s nice to fix it, but we need this fixed for the plain old operator-, not just for operator-=. We have a design problem where when we do math with a float and a FractionalLayoutUnit it is getting done as FractionalLayoutUnit math. >> Source/WebCore/platform/FractionalLayoutUnit.h:584 >> + a = a.toFloat() * b; > > Why does the explicit cast here yield higher precision? It should not. Please do not make this change unless it’s actually helpful. Same comment applies. I was wrong. But fixing this in *= is not sufficient. We need the * operator to work properly too. >> Source/WebCore/platform/FractionalLayoutUnit.h:590 >> + a = a * b.toFloat(); > > The explicit call to toFloat here should not be needed. Same comment applies. I was wrong. But fixing this in *= is not sufficient. We need the * operator to work properly too.

(In reply to comment #5) > (From update of attachment 141345 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141345&action=review > > Some good changes here, but some others that I believe are not helpful. > > > Source/WebCore/platform/FractionalLayoutUnit.h:348 > > - long long rawVal = static_cast<long long>(a.rawValue()) * b.rawValue() / kFixedPointDenominator; > > + long long rawVal = (static_cast<long long>(a.rawValue()) * b.rawValue()) / kFixedPointDenominator; > > Given my understanding of the C rules for operators and parentheses and order of operations, I believe adding these parentheses has no effect. Please do not add them unless you have evidence I am wrong. > Division in mathematical expressions has higher precedence than multiplications so A * B / C is calculated A * (B / C) in this case no only dividing in int instead of long long, but losing precision due to integer division before multiplication. Though to admit I am assuming C is using standard precedence rules here. > > Source/WebCore/platform/FractionalLayoutUnit.h:572 > > - a = a - b; > > + a = a - b.toFloat(); > I don’t think this change adds value. Coding style. The problem is that if you use an operator combination that we haven't explicitly made a new version of then C++ is going to always cast the float to FractionalLayoutUnit (int without SUBPIXEL_LAYOUT set), and then perform the operation in FractionalLayoutUnit. Therefore it is good practise to always use toFloat() whenever we intend the operation to be performed in float. This is already been done in all other implementations except these two. So it brings the good practice and coding style to these two remaining cases.

Created attachment 143053 [details] Patch Remove changes to a*b/c expressions. Darin is right, extra parenthesises are redundant. Removed the explicit .toFloat() casts as well, and instead implemented a full set of operator X= operators. I have verified that this ensures the operations are performed in the correct types (at least for the combinations of FractionalLayoutUnit, int and float).

This patch will likely clash with committed changes in bug 88259.

(In reply to comment #9) > This patch will likely clash with committed changes in bug 88259. Yep, they had the same problem this is supposed to prevent.

(In reply to comment #10) > (In reply to comment #9) > > This patch will likely clash with committed changes in bug 88259. > > Yep, they had the same problem this is supposed to prevent. I'd love to see this fixed, but I haven't had the chance to actually test for its impact on our tests, and the previous patch was uploaded before sub-pixel layout was enabled in Chromium. If you'd please update your patch and re-upload, the cr-linux bot will validate that there aren't any unforeseen results of this change.

(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > This patch will likely clash with committed changes in bug 88259. > > > > Yep, they had the same problem this is supposed to prevent. > > I'd love to see this fixed, but I haven't had the chance to actually test for its impact on our tests, and the previous patch was uploaded before sub-pixel layout was enabled in Chromium. If you'd please update your patch and re-upload, the cr-linux bot will validate that there aren't any unforeseen results of this change. Most of the issues have now been solved in different patches. I will rebase the rest though and reupload it.

Created attachment 151695 [details] Patch

I think I finally figued out why some of the these operations are performed in the wrong type when they shouldn't. Take the case: FractionalLayoutUnit a, b; a = a + b / 2; This will cause a compile error because the statement is ambigious, cause it CAN NOT be performed in FractionalLayoutUnit, and instead has to be performed in int or floats. The problem is that all the operators take 'const FractionalLayoutUnit&' as argument, and this prevents them from operating on the result of a previous operator, because return values are R-values, and you can only take references on L-values. I don't think the solution is to use C++11 R-value references though. The FractionalLayoutUnit is only one int, so call by reference is a completely unnecessary optimization, that will only cause subtle bugs like this. Does that make sense?

(In reply to comment #14) > Does that make sense? I guess not. Sorry for the confusion.

Comment on attachment 151695 [details] Patch This does make sense, thanks for explaining it and taking the time to help us get it right!

(In reply to comment #14) > I don't think the solution is to use C++11 R-value references though. The FractionalLayoutUnit is only one int, so call by reference is a completely unnecessary optimization, that will only cause subtle bugs like this. Mind filing a bug to cover this?

Comment on attachment 151695 [details] Patch Clearing flags on attachment: 151695 Committed r124253: <http://trac.webkit.org/changeset/124253>

All reviewed patches have been landed. Closing bug.

(In reply to comment #17) > (In reply to comment #14) > > I don't think the solution is to use C++11 R-value references though. The FractionalLayoutUnit is only one int, so call by reference is a completely unnecessary optimization, that will only cause subtle bugs like this. > > Mind filing a bug to cover this? No, because I was wrong about the subtle bugs part. As long as it a const reference it can take references of R-values anyway. The reference is still unnecessary, but it is also unnecessary to change it.