Bug 76571 – Add FractionalLayoutPoint/Size/Rect for sub-pixel layout

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Emil A Eklund]

Add fixed point implementation (AppUnit) and Point/Size/Rect types using it.

[Emil A Eklund]

Created attachment 123002 [details]
Patch

Broke out data type changes (AppUnit, FixedPoint/Size/Rect) from subpixel patch.

[WebKit Review Bot]

Attachment 123002 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1

Source/WebCore/platform/graphics/FixedRect.cpp:37:  Use 'using namespace std;' instead of 'using std::max;'.  [build/using_std] [4]
Source/WebCore/platform/graphics/FixedRect.cpp:38:  Use 'using namespace std;' instead of 'using std::min;'.  [build/using_std] [4]
Source/WebCore/platform/graphics/IntRect.h:85:  The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/FloatSize.h:70:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/FloatSize.h:71:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/platform/graphics/FixedSize.h:214:  The parameter name "s" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/FixedSize.h:214:  The parameter name "p" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/FloatPoint.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/AppUnit.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/AppUnit.h:77:  wtf_ceil is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/AppUnit.h:86:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/platform/AppUnit.h:276:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebCore/platform/AppUnit.h:275:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/platform/AppUnit.h:345:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/platform/AppUnit.h:355:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/platform/AppUnit.h:360:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/platform/AppUnit.h:365:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/platform/AppUnit.h:370:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/platform/AppUnit.h:375:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/platform/AppUnit.h:380:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/platform/AppUnit.h:385:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/platform/graphics/FixedSize.cpp:38:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/FixedRect.h:92:  The parameter name "rect" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 23 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Emil A Eklund]

Created attachment 123005 [details]
Patch

Fixed style errors. The check-webkit-style script still (incorrectly?) complains about spacing around operators for declaration of operator overloading  and the wtf_ceil function.

[WebKit Review Bot]

Attachment 123005 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." 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]
Source/WebCore/platform/AppUnit.h:343:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/platform/AppUnit.h:353:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/platform/AppUnit.h:358:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/platform/AppUnit.h:363:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/platform/AppUnit.h:368:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/platform/AppUnit.h:373:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/platform/AppUnit.h:378:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/platform/AppUnit.h:383:  Missing spaces around /  [whitespace/operators] [3]
Total errors found: 9 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gyuyoung Kim]

Comment on attachment 123005 [details]
Patch

Attachment 123005 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11284373

[WebKit Review Bot]

Comment on attachment 123005 [details]
Patch

Attachment 123005 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11150379

[Emil A Eklund]

Created attachment 123034 [details]
Patch

[WebKit Review Bot]

Attachment 123034 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1

Source/WebCore/platform/AppUnit.h:77:  wtf_ceil is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebCore/platform/AppUnit.h:344:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/platform/AppUnit.h:354:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/platform/AppUnit.h:359:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/platform/AppUnit.h:364:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/platform/AppUnit.h:369:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/platform/AppUnit.h:374:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/platform/AppUnit.h:379:  Missing spaces around /  [whitespace/operators] [3]
Source/WebCore/platform/AppUnit.h:384:  Missing spaces around /  [whitespace/operators] [3]
Total errors found: 9 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Seidel (no email)]

Comment on attachment 123034 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=123034&action=review

I think I would land AppUnit first, in a separate bug.  Then I would add some/all of the things that depend on AppUnit.  AppUnit itself is a bit of reading. :)

> Source/WebCore/platform/AppUnit.h:47
> +    AppUnit(int value) { m_value = value * kFixedPointDenominator; }

Do we need overflow asserts here?

[Levi Weintraub]

(In reply to comment #9)
> (From update of attachment 123034 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123034&action=review
> 
> I think I would land AppUnit first, in a separate bug.  Then I would add some/all of the things that depend on AppUnit.  AppUnit itself is a bit of reading. :)
> 
> > Source/WebCore/platform/AppUnit.h:47
> > +    AppUnit(int value) { m_value = value * kFixedPointDenominator; }
> 
> Do we need overflow asserts here?

Pulling AppUnit out now. And we can definitely have overflow asserts there :)

[Emil A Eklund]

Created attachment 124846 [details]
Patch

[Philippe Normand]

Comment on attachment 124846 [details]
Patch

Attachment 124846 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11396212

[Early Warning System Bot]

Comment on attachment 124846 [details]
Patch

Attachment 124846 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11389196

[WebKit Review Bot]

Comment on attachment 124846 [details]
Patch

Attachment 124846 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11394194

[Gyuyoung Kim]

Comment on attachment 124846 [details]
Patch

Attachment 124846 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11386333

[Eric Seidel (no email)]

Comment on attachment 124846 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=124846&action=review

It also starts to feel to me like we may need a PointBase/RectBase/SizeBase classes (not necssarily in this patch, but at some point in the future) to share code between our now 3 types.

> Source/WebCore/platform/graphics/FixedPoint.h:53
> +#if USE(CG) || USE(SKIA_ON_MAC_CHROMIUM)
> +typedef struct CGPoint CGPoint;
> +#endif
> +
> +
> +#if OS(DARWIN) && !PLATFORM(QT)
> +#ifdef NSGEOMETRY_TYPES_SAME_AS_CGGEOMETRY_TYPES
> +typedef struct CGPoint NSPoint;
> +#else
> +typedef struct _NSPoint NSPoint;
> +#endif
> +#endif

As discussed in person, seems we should drop the ability to convert directly to native types from Fixed*.  That makes it explicit in teh few places we want to convert (and should substantially reduce the code in this patch).

[Emil A Eklund]

Created attachment 125951 [details]
Patch

[Philippe Normand]

Comment on attachment 125951 [details]
Patch

Attachment 125951 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11459190

[Early Warning System Bot]

Comment on attachment 125951 [details]
Patch

Attachment 125951 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11449188

[Gyuyoung Kim]

Comment on attachment 125951 [details]
Patch

Attachment 125951 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11462181

[WebKit Review Bot]

Comment on attachment 125951 [details]
Patch

Attachment 125951 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11464186

[Emil A Eklund]

Created attachment 126608 [details]
Patch

[Emil A Eklund]

Created attachment 126789 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 126789 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126789&action=review

> Source/WebCore/platform/FractionalLayoutUnit.h:118
> +        return value < static_cast<unsigned>(std::numeric_limits<int>::max()) / kFixedPointDenominator;

I'm confused.  Why not use std::numeric_limits<unsigned>::max()?

> Source/WebCore/platform/graphics/FloatPoint.h:71
> +    FloatPoint(const FractionalLayoutPoint&);

I don't think we want transparent conversion?

[Emil A Eklund]

Comment on attachment 126789 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126789&action=review

>> Source/WebCore/platform/FractionalLayoutUnit.h:118
>> +        return value < static_cast<unsigned>(std::numeric_limits<int>::max()) / kFixedPointDenominator;
> 
> I'm confused.  Why not use std::numeric_limits<unsigned>::max()?

FractionalLayoutUnit stores the value as an int, thus we need to use INT_MAX, not UNSIGNED_MAX.

>> Source/WebCore/platform/graphics/FloatPoint.h:71
>> +    FloatPoint(const FractionalLayoutPoint&);
> 
> I don't think we want transparent conversion?

We have implicit conversion from int to float so it made sense to also have implicit fixed to float. If you feel strongly about it I'd be happy to make it explicit.

[Eric Seidel (no email)]

Comment on attachment 126789 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=126789&action=review

> Source/WebCore/platform/graphics/FloatPoint.cpp:44
> +FloatPoint::FloatPoint(const FractionalLayoutPoint& p) : m_x(p.x()), m_y(p.y())

This will go through operator float() and get the float value from the AppUnit, correct?

> Source/WebCore/platform/graphics/FloatPoint.h:260
> +inline FloatPoint toPoint(const FloatSize& size)
> +{
> +    return FloatPoint(size.width(), size.height());
> +}

Does FloatSize not have a toPoint() method?

> Source/WebCore/platform/graphics/FloatSize.h:72
> +        return abs(width() - s.width()) < 0.1 && abs(height() - s.height()) < 0.1;

How did you decide on the 0.1 constant?

> Source/WebCore/platform/graphics/FractionalLayoutPoint.h:67
> +        return FractionalLayoutPoint(m_x > other.m_x ? m_x : other.m_x,
> +            m_y > other.m_y ? m_y : other.m_y);

Isn't this what std::max is for?

> Source/WebCore/platform/graphics/FractionalLayoutPoint.h:73
> +        return FractionalLayoutPoint(m_x < other.m_x ? m_x : other.m_x,
> +            m_y < other.m_y ? m_y : other.m_y);

std::min?

> Source/WebCore/platform/graphics/FractionalLayoutPoint.h:79
> +    void clampNegativeToZero()
> +    {
> +        *this = expandedTo(zero());
> +    }

I don't understand this.

> Source/WebCore/platform/graphics/FractionalLayoutPoint.h:84
> +    FractionalLayoutPoint transposedPoint() const
> +    {
> +        return FractionalLayoutPoint(m_y, m_x);
> +    }

Why is this useful?

> Source/WebCore/platform/graphics/FractionalLayoutPoint.h:167
> +#if PLATFORM(QT)
> +inline QDataStream& operator<<(QDataStream& stream, const FractionalLayoutPoint& point)
> +{
> +    stream << point.x().toFloat() << point.y().toFloat();
> +    return stream;
> +}

Should we wait and have Qt add these if they need?

> Source/WebCore/platform/graphics/FractionalLayoutRect.cpp:42
> +    : m_location(FractionalLayoutPoint(r.x(), r.y()))
> +    , m_size(FractionalLayoutSize(r.width(), r.height()))

We don't have FractionalLayoutPoint(FloatPoint?) (even if explicit?)

[Emil A Eklund]

Comment on attachment 126789 [details]
Patch

Thanks for the review, please see my replies inline.

View in context: https://bugs.webkit.org/attachment.cgi?id=126789&action=review

>> Source/WebCore/platform/graphics/FloatPoint.cpp:44
>> +FloatPoint::FloatPoint(const FractionalLayoutPoint& p) : m_x(p.x()), m_y(p.y())
> 
> This will go through operator float() and get the float value from the AppUnit, correct?

Correct

>> Source/WebCore/platform/graphics/FloatPoint.h:260
>> +}
> 
> Does FloatSize not have a toPoint() method?

It does not.

>> Source/WebCore/platform/graphics/FloatSize.h:72
>> +        return abs(width() - s.width()) < 0.1 && abs(height() - s.height()) < 0.1;
> 
> How did you decide on the 0.1 constant?

I recently removed the last use of this function from the branch so I'll go ahead and remove the function all together.

>> Source/WebCore/platform/graphics/FractionalLayoutPoint.h:67
>> +            m_y > other.m_y ? m_y : other.m_y);
> 
> Isn't this what std::max is for?

Changed to use std::max

>> Source/WebCore/platform/graphics/FractionalLayoutPoint.h:73
>> +            m_y < other.m_y ? m_y : other.m_y);
> 
> std::min?

Changed to use std::min

>> Source/WebCore/platform/graphics/FractionalLayoutPoint.h:79
>> +    }
> 
> I don't understand this.

It calls expandedTo with a zero point to ensure that the values are not negative.

>> Source/WebCore/platform/graphics/FractionalLayoutPoint.h:84
>> +    }
> 
> Why is this useful?

The same way that FloatPoint::transposedPoint and IntPoint::transposedPoint is. It's used in RenderBlock and RenderFlexibleBox.

>> Source/WebCore/platform/graphics/FractionalLayoutPoint.h:167
>> +}
> 
> Should we wait and have Qt add these if they need?

My bad, this should not be here. Merge fail.

>> Source/WebCore/platform/graphics/FractionalLayoutRect.cpp:42
>> +    , m_size(FractionalLayoutSize(r.width(), r.height()))
> 
> We don't have FractionalLayoutPoint(FloatPoint?) (even if explicit?)

We do, changed accordingly.

[Emil A Eklund]

Created attachment 127066 [details]
Patch

[Emil A Eklund]

Ping?

[Eric Seidel (no email)]

Comment on attachment 127066 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127066&action=review

I think we should hav done this in smaller pieces.  That would have been faster.  One class at a time.  But this is OK.  I suspect that we need to think more about using the compiler to help us find bugs in code which uses these types.  Making conversions explicit.  Possibly typedeffing differnet types for differnet parts of the tree (ClipRect, LayerRect, etc.)  It needs to be made more clear, when it is correct to use what type.

> Source/WebCore/platform/graphics/FractionalLayoutSize.h:136
> +inline bool operator==(const FractionalLayoutSize& a, const FractionalLayoutSize& b)
> +{
> +    return a.width() == b.width() && a.height() == b.height();
> +}
> +
> +inline bool operator!=(const FractionalLayoutSize& a, const FractionalLayoutSize& b)
> +{
> +    return a.width() != b.width() || a.height() != b.height();
> +}

One of these could be defined in terms of the other, but this is OK.

> Source/WebCore/platform/graphics/FractionalLayoutSize.h:140
> +    return IntSize(s.width().toInt(), s.height().toInt());

makes me wonder why we don't call that floor(). :)

> Source/WebCore/platform/graphics/FractionalLayoutSize.h:148
> +IntSize pixelSnappedIntSize(const FractionalLayoutSize&, const FractionalLayoutPoint&);

Since you're including IntSize.h in this header already, I'm surprised this is not inline. :)

[Emil A Eklund]

(In reply to comment #30)
> (From update of attachment 127066 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127066&action=review
> 
> I think we should hav done this in smaller pieces.  That would have been faster.  One class at a time.  But this is OK.  I suspect that we need to think more about using the compiler to help us find bugs in code which uses these types.  Making conversions explicit.  Possibly typedeffing differnet types for differnet parts of the tree (ClipRect, LayerRect, etc.)  It needs to be made more clear, when it is correct to use what type.

Might have made sense, given that they depend on each other it might have taken more iterations though but it's definitively something worth considering for next time.

Thanks for your patience in reviewing this.

> Since you're including IntSize.h in this header already, I'm surprised this is not inline. :)

I'll make it inline and remove FractionalLayoutSize.cpp before landing.

[Emil A Eklund]

(In reply to comment #31)
> I'll make it inline and remove FractionalLayoutSize.cpp before landing.

...or not as that would create a circular dependency between FractionalLayoutSize.h and FractionalLayoutPoint.h

[Emil A Eklund]

Created attachment 127262 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 127262 [details]
Patch for landing

Clearing flags on attachment: 127262

Committed r107887: <http://trac.webkit.org/changeset/107887>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Roben (:aroben)]

Looks like this broke the 32-bit build:


In file included from Source/WebCore/platform/graphics/FloatPoint.cpp:31:
In file included from Source/WebCore/platform/graphics/FractionalLayoutPoint.h:35:
In file included from Source/WebCore/platform/graphics/FractionalLayoutSize.h:35:
Source/WebCore/platform/FractionalLayoutUnit.h:67:61: error: implicit conversion loses integer precision: 'long long' to 'int' [-Werror,-Wshorten-64-to-32]
    inline void setRawValue(long long value) { ASSERT(::abs(value) < std::numeric_limits<int>::max()); m_value = value; }
                                               ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I'll take a stab at fixing it.

[Adam Roben (:aroben)]

Comment on attachment 127262 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=127262&action=review

> Source/WebCore/ChangeLog:22
> +        * platform/FractionalLayoutUnit.h:

Shouldn't this file be in platform/graphics?

[Adam Roben (:aroben)]

Fixing the 32-bit build will be a little tricky. We need to be using llabs(), but that function doesn't exist on Windows (don't know about other platforms), so we'll need some cover function that does the right thing everywhere. I'm just going to roll this out for now.

[Emil A Eklund]

(In reply to comment #38)
> Fixing the 32-bit build will be a little tricky. We need to be using llabs(), but that function doesn't exist on Windows (don't know about other platforms), so we'll need some cover function that does the right thing everywhere. I'm just going to roll this out for now.

Ok, seems reasonable.

[Adam Roben (:aroben)]

Rolled out in r107954.

[Emil A Eklund]

(In reply to comment #40)
> Rolled out in r107954.

Thanks!