[webkit-reviews] review granted: [Bug 76571] Add FractionalLayoutPoint/Size/Rect for sub-pixel layout : [Attachment 127066] 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 76571: Add FractionalLayoutPoint/Size/Rect for sub-pixel layout
https://bugs.webkit.org/show_bug.cgi?id=76571

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

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