Bug 76571

Created attachment 123002 [details] Patch Broke out data type changes (AppUnit, FixedPoint/Size/Rect) from subpixel patch.

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.

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.

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 on attachment 123005 [details] Patch Attachment 123005 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11284373

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

Created attachment 123034 [details] Patch

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 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?

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

Created attachment 124846 [details] Patch

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

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

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

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

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

Created attachment 125951 [details] Patch

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

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

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

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

Created attachment 126608 [details] Patch

Created attachment 126789 [details] Patch

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

Created attachment 127066 [details] Patch

Ping?

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

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

(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

Created attachment 127262 [details] Patch for landing

Comment on attachment 127262 [details] Patch for landing Clearing flags on attachment: 127262 Committed r107887: <http://trac.webkit.org/changeset/107887>

All reviewed patches have been landed. Closing bug.

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 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?

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.

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

Rolled out in r107954.

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