[webkit-reviews] review granted: [Bug 80039] Update usage of LayoutUnits in RenderBox : [Attachment 129730] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Levi Weintraub
<leviw at chromium.org>'s request for review:
Bug 80039: Update usage of LayoutUnits in RenderBox
https://bugs.webkit.org/show_bug.cgi?id=80039

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

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=129730&action=review


> Source/WebCore/rendering/RenderBox.cpp:483
> +    return snapSizeToPixel(clientWidth(), clientLeft());

It's unfortunate that snapSizeToPixel requires also the location as it makes
those call sites less readable and more error-prone but you can't avoid that to
properly snap the size :(

> Source/WebCore/rendering/RenderBox.h:131
> +    // FIXME: We shouldn't be returning this as a LayoutRect, since it loses
its position and won't properly pixel snap.
>      LayoutRect borderBoxRect() const { return LayoutRect(0, 0, width(),
height()); }

It took me some time to understand your comment as you can see the issue both
ways: if you properly snap your width() and height() here then you should be
fine. I think I agree with the gist that is that this shouldn't return
LayoutRect or properly take into account the position. I wonder if we just
couldn't just remove this method as it will be evil when sub-pixels are added.