Bug 78924

Created attachment 127678 [details] Patch

This is the last of the new types, any chance you could find the time to review this today?

Comment on attachment 127678 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127678&action=review Seems OK. It's difficult to review this accurately as there is a lot of chance for copy/paste error here. Consider my suggestions for doing less math on the raw components (and hopefully then reducing the amount of code and amount of chance of copy/paste error.) > Source/WebCore/platform/graphics/FractionalLayoutRect.cpp:65 > + FractionalLayoutUnit newX = std::max(x(), other.x()); > + FractionalLayoutUnit newY = std::max(y(), other.y()); > + FractionalLayoutUnit newMaxX = std::min(maxX(), other.maxX()); > + FractionalLayoutUnit newMaxY = std::min(maxY(), other.maxY()); I might have done this math on points. FractionalLayoutPoint newLocation(max(x(), other.x()), max(y(), other.y())); FractionalLayoutPoint newMaxPoint(min(maxX(), other.maxX()), min(maxY(), other.maxY())); Then the rest of the function becomes slightly less verbose. > Source/WebCore/platform/graphics/FractionalLayoutRect.cpp:91 > + FractionalLayoutUnit newX = std::min(x(), other.x()); I thought we normally added using std; at the top of cpp's to avoid needing the std:: here? > Source/WebCore/platform/graphics/FractionalLayoutRect.cpp:128 > + m_location.setX(x() * s); > + m_location.setY(y() * s); > + m_size.setWidth(width() * s); > + m_size.setHeight(height() * s); point and size don't have scale() functions?

Comment on attachment 127678 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=127678&action=review Thanks! >> Source/WebCore/platform/graphics/FractionalLayoutRect.cpp:65 >> + FractionalLayoutUnit newMaxY = std::min(maxY(), other.maxY()); > > I might have done this math on points. > FractionalLayoutPoint newLocation(max(x(), other.x()), max(y(), other.y())); > FractionalLayoutPoint newMaxPoint(min(maxX(), other.maxX()), min(maxY(), other.maxY())); > > Then the rest of the function becomes slightly less verbose. Good idea, I'll update the intersect and unit methods accordingly. >> Source/WebCore/platform/graphics/FractionalLayoutRect.cpp:91 >> + FractionalLayoutUnit newX = std::min(x(), other.x()); > > I thought we normally added using std; at the top of cpp's to avoid needing the std:: here? Right, added using std::max/min.

Created attachment 128035 [details] Patch for landing

Comment on attachment 128035 [details] Patch for landing Rejecting attachment 128035 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: Source/WebCore/platform/graphics/FloatRect.cpp patching file Source/WebCore/platform/graphics/FloatRect.h patching file Source/WebCore/platform/graphics/FractionalLayoutRect.cpp patching file Source/WebCore/platform/graphics/FractionalLayoutRect.h patching file Source/WebCore/platform/graphics/IntRect.cpp patching file Source/WebCore/platform/graphics/IntRect.h Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/11559290

Comment on attachment 128035 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=128035&action=review > Source/WebCore/platform/graphics/FractionalLayoutRect.cpp:71 > + newLocation = FractionalLayoutPoint(0, 0); > + newMaxPoint = FractionalLayoutPoint(0, 0); Don't we have FractionalLayoutPoint::zero()?

Created attachment 128079 [details] Patch for landing

Comment on attachment 128079 [details] Patch for landing Clearing flags on attachment: 128079 Committed r108423: <http://trac.webkit.org/changeset/108423>

All reviewed patches have been landed. Closing bug.

Comment on attachment 128079 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=128079&action=review > Source/WebCore/platform/graphics/FractionalLayoutRect.cpp:145 > +FractionalLayoutRect enclosingFractionalLayoutRect(const FloatRect& rect) > +{ > + return FractionalLayoutRect(rect.x(), rect.y(), > + rect.maxX() - rect.x() + FractionalLayoutUnit::epsilon(), > + rect.maxY() - rect.y() + FractionalLayoutUnit::epsilon()); > +} Consider, FloatRect rect=(0.0, 0.0, 800.0, 600.0). This method returns a FractionaLayoutRect(0.0, 800.0 + eps, 600.0 + eps). enclosingIntRect(FractionalLayoutRect(rect)) yields 0,0 x 801,601. enclosingIntRect(rect) gives 0,0 x 800,600 as expected. Am I missing something, or is this a bug?