Bug 77485

Created attachment 124841 [details] Patch

Attachment 124841 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/AppUnit.h:76: wtf_ceil is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 124841 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124841&action=review > Source/WebCore/platform/AppUnit.h:289 > + if ((a.rawValue() >= 0 && b.rawValue() >= 0) || (a.rawValue() < 0 && b.rawValue() < 0)) { > + if (returnVal.rawValue() < 0) > + return AppUnit::max(); > + return returnVal; since rawValue is signed, seems we care about the case of -max * max == -max. > Source/WebCore/platform/AppUnit.h:298 > + AppUnit returnVal; "result" is shorter. Your call. > Source/WebCore/platform/AppUnit.h:300 > + returnVal.setRawValue((a.rawValue() / kFixedPointDenominator) * b.rawValue()); > + ASSERT((a.rawValue() >= 0 && b.rawValue() >= 0 && returnVal.rawValue() >= 0) I wonder if we should rename rawValue to value() given how much its typed here. :) > Source/WebCore/platform/AppUnit.h:314 > +inline float operator*(const AppUnit& a, float b) > +{ > + return a.toFloat() * b; > +} (float)(AppUnit(a) * b) != a * b.toFloat()... right? I'm realizing that if we're not careful we could have operators which are subtly different like this.

Comment on attachment 124841 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124841&action=review >> Source/WebCore/platform/AppUnit.h:289 >> + return returnVal; > > since rawValue is signed, seems we care about the case of -max * max == -max. Check out the if above. We only get into this case if both raw values are + or -. >> Source/WebCore/platform/AppUnit.h:300 >> + ASSERT((a.rawValue() >= 0 && b.rawValue() >= 0 && returnVal.rawValue() >= 0) > > I wonder if we should rename rawValue to value() given how much its typed here. :) I went with rawValue to ward of misuse. rawValue is public because there are a very small number of cases where we needed it outside of AppUnit.h. >> Source/WebCore/platform/AppUnit.h:314 >> +} > > (float)(AppUnit(a) * b) != a * b.toFloat()... right? I'm realizing that if we're not careful we could have operators which are subtly different like this. This is entirely true. When interacting with floating point, we opt to return floats with the assumption/hope that this is what's ultimately wanted and used. We'd end up doing a lot more work and losing precision in a case like this: float f = foo; AppUnit a = bar; float result = f * bar;

Comment on attachment 124841 [details] Patch AppUnit? Where does that name come from?

(In reply to comment #5) > (From update of attachment 124841 [details]) > AppUnit? Where does that name come from? From Gecko: https://wiki.mozilla.org/Mozilla2:Units We're not wedded to the name, though, if you have any suggestions.

(In reply to comment #6) > (In reply to comment #5) > > (From update of attachment 124841 [details] [details]) > > AppUnit? Where does that name come from? > > From Gecko: https://wiki.mozilla.org/Mozilla2:Units > > We're not wedded to the name, though, if you have any suggestions. I should add that our plan is to rename AppUnit to LayoutUnit once we've landed sub-pixel layout, thereby giving it a better name and removing the current abstraction.

(In reply to comment #7) > I should add that our plan is to rename AppUnit to LayoutUnit once we've landed sub-pixel layout, thereby giving it a better name and removing the current abstraction. If that’s the plan, then I suggest FractionalLayoutUnit rather than AppUnit as the temporary name.

Or maybe SubpixelLayoutUnit.

(In reply to comment #9) > Or maybe SubpixelLayoutUnit. Or SubpixelUnit?

Created attachment 125898 [details] Patch

Attachment 125898 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/FractionalLayoutUnit.h:76: wtf_ceil is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 125898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125898&action=review > Source/WebCore/platform/FractionalLayoutUnit.h:81 > + return (m_value > 0 ? m_value + kFixedPointDenominator - 1 : m_value - kFixedPointDenominator + 1) / kFixedPointDenominator; might be clearer written out like round. Doens't really matter. > Source/WebCore/platform/FractionalLayoutUnit.h:112 > + return ::abs(value) < std::numeric_limits<int>::max() / 60; Why not use the denominator constant? > Source/WebCore/platform/FractionalLayoutUnit.h:148 > + return FractionalLayoutUnit(a) <= b; It's slightly odd to me that whenever you deal with floats you convert the unit to a float before comparison, but whenever dealing with ints you convert the int to a unit for comparision. That's probably right. But still strikes me as odd that they're different.

Comment on attachment 125898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125898&action=review I think we need another round. > Source/WebCore/platform/FractionalLayoutUnit.h:126 > +inline bool operator<=(const FractionalLayoutUnit& a, const FractionalLayoutUnit& b) Isn't this defined automagically as < || == ? I'm surpised you need to write all these. > Source/WebCore/platform/FractionalLayoutUnit.h:269 > +inline bool operator==(const FractionalLayoutUnit& a, int b) > +{ > + return a.toInt() == b; > +} > + > +inline bool operator==(const int a, const FractionalLayoutUnit& b) > +{ > + return a == b.toInt(); > +} This one is also off, since you tend to use FractionalLayoutUnit(int) instead? > Source/WebCore/platform/FractionalLayoutUnit.h:274 > +inline bool operator==(const FractionalLayoutUnit& a, float b) > +{ > + return a == FractionalLayoutUnit(b); > +} This seems to be the opposite of your normal pattern... > Source/WebCore/platform/FractionalLayoutUnit.h:285 > + returnVal.setRawValue((a.rawValue() / kFixedPointDenominator) * b.rawValue()); So this loses precison on a... but not on b? Odd. > Source/WebCore/platform/FractionalLayoutUnit.h:299 > + returnVal.setRawValue((a.rawValue() / kFixedPointDenominator) * b.rawValue()); Again, losing precision on a, but not b. I think you want to use long long temporaries. > Source/WebCore/platform/FractionalLayoutUnit.h:359 > + returnVal.setRawValue(kFixedPointDenominator * (a.rawValue() / b.rawValue())); (a/b) / (c/b) == (a/b) * (b/c) == a/c? So you could have just written this as: AppUnit returnVal(a.rawValue() / b.rawValue()) > Source/WebCore/platform/FractionalLayoutUnit.h:481 > + a = a + b.toFloat(); These toFloats() shouldn't be needed. > Source/WebCore/platform/FractionalLayoutUnit.h:499 > + a = a - b.toFloat(); Here too.

Comment on attachment 125898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125898&action=review >> Source/WebCore/platform/FractionalLayoutUnit.h:126 >> +inline bool operator<=(const FractionalLayoutUnit& a, const FractionalLayoutUnit& b) > > Isn't this defined automagically as < || == ? I'm surpised you need to write all these. It’s not. This is correct.

Comment on attachment 125898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125898&action=review > Source/WebCore/platform/FractionalLayoutUnit.h:264 > +inline bool operator==(const FractionalLayoutUnit& a, int b) > +{ > + return a.toInt() == b; > +} This doesn’t sound right. If you compare a double with an int then 0.1 is not equal to 0.

(In reply to comment #15) > (From update of attachment 125898 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=125898&action=review > > >> Source/WebCore/platform/FractionalLayoutUnit.h:126 > >> +inline bool operator<=(const FractionalLayoutUnit& a, const FractionalLayoutUnit& b) > > > > Isn't this defined automagically as < || == ? I'm surpised you need to write all these. > > It’s not. This is correct. It seems it might be useful to have a template base-class which did that sort of automagic operator defintions for you. :) The advantage of this approach is it's explicit (which can be useful for tricky comparisons), but I suspect most times we override <=, we'd rather just have an automagic <= defined as < or ||.

Created attachment 126154 [details] Patch

Attachment 126154 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/FractionalLayoutUnit.h:76: wtf_ceil is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 126154 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126154&action=review > Source/WebCore/platform/FractionalLayoutUnit.h:276 > +inline bool operator==(const FractionalLayoutUnit& a, float b) > +{ > + return a.toFloat() == b; > +} Did we decide that .toFloat() was correct here? You want this comparison to be exact, I assume?

Comment on attachment 126154 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126154&action=review > Source/WebCore/platform/FractionalLayoutUnit.h:289 > + return AppUnit::max(); Doubt this compiles. :)

(In reply to comment #20) > (From update of attachment 126154 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=126154&action=review > > > Source/WebCore/platform/FractionalLayoutUnit.h:276 > > +inline bool operator==(const FractionalLayoutUnit& a, float b) > > +{ > > + return a.toFloat() == b; > > +} > > Did we decide that .toFloat() was correct here? You want this comparison to be exact, I assume? Yes. And I tested these changes on our branch and found a bug as well :)

Created attachment 126210 [details] Patch

Attachment 126210 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/FractionalLayoutUnit.h:76: wtf_ceil is incorrectly named. Don't use underscores in your identifier names. [readability/naming] [4] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Ping?

Comment on attachment 126210 [details] Patch OK.

Comment on attachment 126210 [details] Patch Clearing flags on attachment: 126210 Committed r107453: <http://trac.webkit.org/changeset/107453>

All reviewed patches have been landed. Closing bug.