Bug 78924 - Add FractionalLayoutRect for sub-pixel layout
Summary: Add FractionalLayoutRect for sub-pixel layout
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Emil A Eklund
URL:
Keywords:
Depends on: 78852 78913
Blocks: 76571
  Show dependency treegraph
 
Reported: 2012-02-17 13:28 PST by Emil A Eklund
Modified: 2012-07-08 10:06 PDT (History)
5 users (show)

See Also:


Attachments
Patch (28.88 KB, patch)
2012-02-17 17:07 PST, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch for landing (28.48 KB, patch)
2012-02-21 13:38 PST, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch for landing (26.78 KB, patch)
2012-02-21 16:28 PST, Emil A Eklund
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Emil A Eklund 2012-02-17 13:28:09 PST
Add FractionalLayoutUnit implementation of rect class. This will use the new FractionalLayoutPoint and FractionalLayoutSize classes.
Comment 1 Emil A Eklund 2012-02-17 17:07:53 PST
Created attachment 127678 [details]
Patch
Comment 2 Emil A Eklund 2012-02-21 11:27:42 PST
This is the last of the new types, any chance you could find the time to review this today?
Comment 3 Eric Seidel (no email) 2012-02-21 11:56:42 PST
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 4 Emil A Eklund 2012-02-21 12:12:03 PST
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.
Comment 5 Emil A Eklund 2012-02-21 13:38:30 PST
Created attachment 128035 [details]
Patch for landing
Comment 6 WebKit Review Bot 2012-02-21 15:35:00 PST
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 7 Eric Seidel (no email) 2012-02-21 16:20:49 PST
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()?
Comment 8 Emil A Eklund 2012-02-21 16:28:26 PST
Created attachment 128079 [details]
Patch for landing
Comment 9 WebKit Review Bot 2012-02-21 17:18:59 PST
Comment on attachment 128079 [details]
Patch for landing

Clearing flags on attachment: 128079

Committed r108423: <http://trac.webkit.org/changeset/108423>
Comment 10 WebKit Review Bot 2012-02-21 17:19:08 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Nikolas Zimmermann 2012-07-08 10:06:19 PDT
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?