[webkit-reviews] review granted: [Bug 78924] Add FractionalLayoutRect for sub-pixel layout : [Attachment 127678] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Emil A Eklund <eae at chromium.org>'s
request for review:
Bug 78924: Add FractionalLayoutRect for sub-pixel layout
https://bugs.webkit.org/show_bug.cgi?id=78924

Attachment 127678: Patch
https://bugs.webkit.org/attachment.cgi?id=127678&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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?