Bug 78924 – Add FractionalLayoutRect for sub-pixel layout

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

[Emil A Eklund]

Add FractionalLayoutUnit implementation of rect class. This will use the new FractionalLayoutPoint and FractionalLayoutSize classes.

[Emil A Eklund]

Created attachment 127678 [details]
Patch

[Emil A Eklund]

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

[Eric Seidel (no email)]

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?

[Emil A Eklund]

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.

[Emil A Eklund]

Created attachment 128035 [details]
Patch for landing

[WebKit Review Bot]

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

[Eric Seidel (no email)]

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

[Emil A Eklund]

Created attachment 128079 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 128079 [details]
Patch for landing

Clearing flags on attachment: 128079

Committed r108423: <http://trac.webkit.org/changeset/108423>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Nikolas Zimmermann]

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?