Bug 77485 - Add FractionalLayoutUnit type for sub-pixel layout
Summary: Add FractionalLayoutUnit type for sub-pixel layout
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Levi Weintraub
URL:
Keywords:
Depends on:
Blocks: 76571
  Show dependency treegraph
 
Reported: 2012-01-31 15:27 PST by Levi Weintraub
Modified: 2012-02-10 16:00 PST (History)
5 users (show)

See Also:


Attachments
Patch (17.72 KB, patch)
2012-01-31 16:10 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (20.11 KB, patch)
2012-02-07 12:21 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (19.52 KB, patch)
2012-02-08 14:31 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (19.55 KB, patch)
2012-02-08 17:52 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Levi Weintraub 2012-01-31 15:27:35 PST
Adding AppUnit type which is the basic unit for layout in sub-pixel layout.
Comment 1 Levi Weintraub 2012-01-31 16:10:45 PST
Created attachment 124841 [details]
Patch
Comment 2 WebKit Review Bot 2012-01-31 16:13:59 PST
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 3 Eric Seidel (no email) 2012-01-31 16:28:10 PST
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 4 Levi Weintraub 2012-01-31 16:43:38 PST
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 5 Darin Adler 2012-01-31 17:18:17 PST
Comment on attachment 124841 [details]
Patch

AppUnit? Where does that name come from?
Comment 6 Levi Weintraub 2012-01-31 17:27:53 PST
(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.
Comment 7 Levi Weintraub 2012-02-06 17:51:47 PST
(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.
Comment 8 Darin Adler 2012-02-07 11:19:07 PST
(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.
Comment 9 Darin Adler 2012-02-07 12:14:37 PST
Or maybe SubpixelLayoutUnit.
Comment 10 Eric Seidel (no email) 2012-02-07 12:17:56 PST
(In reply to comment #9)
> Or maybe SubpixelLayoutUnit.

Or SubpixelUnit?
Comment 11 Levi Weintraub 2012-02-07 12:21:47 PST
Created attachment 125898 [details]
Patch
Comment 12 WebKit Review Bot 2012-02-07 12:24:31 PST
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 13 Eric Seidel (no email) 2012-02-07 13:27:15 PST
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 14 Eric Seidel (no email) 2012-02-07 13:46:08 PST
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 15 Darin Adler 2012-02-07 17:04:10 PST
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 16 Darin Adler 2012-02-07 17:05:06 PST
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.
Comment 17 Eric Seidel (no email) 2012-02-07 17:08:11 PST
(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 ||.
Comment 18 Levi Weintraub 2012-02-08 14:31:24 PST
Created attachment 126154 [details]
Patch
Comment 19 WebKit Review Bot 2012-02-08 14:36:54 PST
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 20 Eric Seidel (no email) 2012-02-08 16:15:38 PST
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 21 Eric Seidel (no email) 2012-02-08 16:16:01 PST
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. :)
Comment 22 Levi Weintraub 2012-02-08 17:32:34 PST
(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 :)
Comment 23 Levi Weintraub 2012-02-08 17:52:07 PST
Created attachment 126210 [details]
Patch
Comment 24 WebKit Review Bot 2012-02-08 17:57:34 PST
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.
Comment 25 Emil A Eklund 2012-02-10 15:44:25 PST
Ping?
Comment 26 Eric Seidel (no email) 2012-02-10 15:48:56 PST
Comment on attachment 126210 [details]
Patch

OK.
Comment 27 Levi Weintraub 2012-02-10 16:00:47 PST
Comment on attachment 126210 [details]
Patch

Clearing flags on attachment: 126210

Committed r107453: <http://trac.webkit.org/changeset/107453>
Comment 28 Levi Weintraub 2012-02-10 16:00:52 PST
All reviewed patches have been landed.  Closing bug.