Bug 76571

Summary: Add FractionalLayoutPoint/Size/Rect for sub-pixel layout
Product: WebKit Reporter: Emil A Eklund <eae>
Component: Layout and RenderingAssignee: Emil A Eklund <eae>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben, dglazkov, eoconnor, eric, gustavo, hyatt, leviw, pnormand, rakuco, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 77485, 78835, 78852, 78913, 78924    
Bug Blocks: 60318    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Emil A Eklund 2012-01-18 14:18:25 PST
Add fixed point implementation (AppUnit) and Point/Size/Rect types using it.
Comment 1 Emil A Eklund 2012-01-18 14:48:07 PST
Created attachment 123002 [details]
Patch

Broke out data type changes (AppUnit, FixedPoint/Size/Rect) from subpixel patch.
Comment 2 WebKit Review Bot 2012-01-18 14:53:38 PST
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.
Comment 3 Emil A Eklund 2012-01-18 15:05:10 PST
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.
Comment 4 WebKit Review Bot 2012-01-18 15:07:41 PST
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.
Comment 5 Gyuyoung Kim 2012-01-18 15:37:06 PST
Comment on attachment 123005 [details]
Patch

Attachment 123005 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11284373
Comment 6 WebKit Review Bot 2012-01-18 15:40:40 PST
Comment on attachment 123005 [details]
Patch

Attachment 123005 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11150379
Comment 7 Emil A Eklund 2012-01-18 16:37:00 PST
Created attachment 123034 [details]
Patch
Comment 8 WebKit Review Bot 2012-01-18 16:41:08 PST
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.
Comment 9 Eric Seidel (no email) 2012-01-31 15:10:03 PST
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?
Comment 10 Levi Weintraub 2012-01-31 15:17:39 PST
(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 :)
Comment 11 Emil A Eklund 2012-01-31 16:43:39 PST
Created attachment 124846 [details]
Patch
Comment 12 Philippe Normand 2012-01-31 16:55:36 PST
Comment on attachment 124846 [details]
Patch

Attachment 124846 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11396212
Comment 13 Early Warning System Bot 2012-01-31 17:00:24 PST
Comment on attachment 124846 [details]
Patch

Attachment 124846 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11389196
Comment 14 WebKit Review Bot 2012-01-31 17:04:47 PST
Comment on attachment 124846 [details]
Patch

Attachment 124846 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11394194
Comment 15 Gyuyoung Kim 2012-01-31 22:19:21 PST
Comment on attachment 124846 [details]
Patch

Attachment 124846 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11386333
Comment 16 Eric Seidel (no email) 2012-02-07 14:07:04 PST
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).
Comment 17 Emil A Eklund 2012-02-07 16:16:01 PST
Created attachment 125951 [details]
Patch
Comment 18 Philippe Normand 2012-02-07 17:21:01 PST
Comment on attachment 125951 [details]
Patch

Attachment 125951 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11459190
Comment 19 Early Warning System Bot 2012-02-07 18:00:22 PST
Comment on attachment 125951 [details]
Patch

Attachment 125951 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11449188
Comment 20 Gyuyoung Kim 2012-02-07 18:06:49 PST
Comment on attachment 125951 [details]
Patch

Attachment 125951 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11462181
Comment 21 WebKit Review Bot 2012-02-07 18:41:17 PST
Comment on attachment 125951 [details]
Patch

Attachment 125951 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11464186
Comment 22 Emil A Eklund 2012-02-10 17:46:18 PST
Created attachment 126608 [details]
Patch
Comment 23 Emil A Eklund 2012-02-13 10:15:57 PST
Created attachment 126789 [details]
Patch
Comment 24 Eric Seidel (no email) 2012-02-13 10:51:54 PST
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?
Comment 25 Emil A Eklund 2012-02-13 11:08:43 PST
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.
Comment 26 Eric Seidel (no email) 2012-02-14 15:03:41 PST
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?)
Comment 27 Emil A Eklund 2012-02-14 15:28:43 PST
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.
Comment 28 Emil A Eklund 2012-02-14 15:41:46 PST
Created attachment 127066 [details]
Patch
Comment 29 Emil A Eklund 2012-02-15 11:23:31 PST
Ping?
Comment 30 Eric Seidel (no email) 2012-02-15 14:00:43 PST
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. :)
Comment 31 Emil A Eklund 2012-02-15 15:13:49 PST
(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.
Comment 32 Emil A Eklund 2012-02-15 15:20:12 PST
(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
Comment 33 Emil A Eklund 2012-02-15 16:00:58 PST
Created attachment 127262 [details]
Patch for landing
Comment 34 WebKit Review Bot 2012-02-15 22:40:49 PST
Comment on attachment 127262 [details]
Patch for landing

Clearing flags on attachment: 127262

Committed r107887: <http://trac.webkit.org/changeset/107887>
Comment 35 WebKit Review Bot 2012-02-15 22:40:57 PST
All reviewed patches have been landed.  Closing bug.
Comment 36 Adam Roben (:aroben) 2012-02-16 06:07:04 PST
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.
Comment 37 Adam Roben (:aroben) 2012-02-16 06:07:48 PST
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?
Comment 38 Adam Roben (:aroben) 2012-02-16 06:11:31 PST
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.
Comment 39 Emil A Eklund 2012-02-16 08:15:43 PST
(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.
Comment 40 Adam Roben (:aroben) 2012-02-16 10:12:56 PST
Rolled out in r107954.
Comment 41 Emil A Eklund 2012-02-16 10:21:16 PST
(In reply to comment #40)
> Rolled out in r107954.

Thanks!