Add FractionalLayoutUnit type for sub-pixel layout

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Levi Weintraub]

Adding AppUnit type which is the basic unit for layout in sub-pixel layout.

[Levi Weintraub]

Created attachment 124841 [details]
Patch

[WebKit Review Bot]

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.

[Eric Seidel (no email)]

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.

[Levi Weintraub]

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;

[Darin Adler]

Comment on attachment 124841 [details]
Patch

AppUnit? Where does that name come from?

[Levi Weintraub]

(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.

[Levi Weintraub]

(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.

[Darin Adler]

(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.

[Darin Adler]

Or maybe SubpixelLayoutUnit.

[Eric Seidel (no email)]

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

Or SubpixelUnit?

[Levi Weintraub]

Created attachment 125898 [details]
Patch

[WebKit Review Bot]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Darin Adler]

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.

[Darin Adler]

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.

[Eric Seidel (no email)]

(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 ||.

[Levi Weintraub]

Created attachment 126154 [details]
Patch

[WebKit Review Bot]

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.

[Eric Seidel (no email)]

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?

[Eric Seidel (no email)]

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. :)

[Levi Weintraub]

(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 :)

[Levi Weintraub]

Created attachment 126210 [details]
Patch

[WebKit Review Bot]

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.

[Emil A Eklund]

Ping?

[Eric Seidel (no email)]

Comment on attachment 126210 [details]
Patch

OK.

[Levi Weintraub]

Comment on attachment 126210 [details]
Patch

Clearing flags on attachment: 126210

Committed r107453: <http://trac.webkit.org/changeset/107453>

[Levi Weintraub]

All reviewed patches have been landed.  Closing bug.