Bug 81038 – Update LayoutUnit usage in RenderListBox

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

[Levi Weintraub]

Rounding paint offsets, snapping painted rects, and rounding mouse coordinates, as laid out in https://trac.webkit.org/wiki/LayoutUnit.

[Levi Weintraub]

Created attachment 131727 [details]
Patch

[Julien Chaffraix]

Comment on attachment 131727 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131727&action=review

> Source/WebCore/ChangeLog:19
> +        (WebCore::RenderListBox::panScroll): EventHandler::currentMousePosition is an IntPoint,
> +        so it doesn't require additional rounding in RenderListBox.

It would be nice to make such conversion fail at compile time when you change the type of LayoutUnit to catch such unneeded copying.

> Source/WebCore/rendering/RenderListBox.cpp:425
> +    paintInfo.context->drawBidiText(itemFont, textRun, roundedIntPoint(r.location()));

I was going to ask about why rounding and then I saw that's what pixelSnappedIntRect does. I wonder if we could not make it more obvious that pixelSnappedIntRect is doing a roundedIntPoint on the location...

[Levi Weintraub]

Comment on attachment 131727 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131727&action=review

>> Source/WebCore/ChangeLog:19
>> +        so it doesn't require additional rounding in RenderListBox.
> 
> It would be nice to make such conversion fail at compile time when you change the type of LayoutUnit to catch such unneeded copying.

Making IntPoint->LayoutPoint an explicit conversion would be a good idea, you're right.

>> Source/WebCore/rendering/RenderListBox.cpp:425
>> +    paintInfo.context->drawBidiText(itemFont, textRun, roundedIntPoint(r.location()));
> 
> I was going to ask about why rounding and then I saw that's what pixelSnappedIntRect does. I wonder if we could not make it more obvious that pixelSnappedIntRect is doing a roundedIntPoint on the location...

We could call it pixelSnappedIntPoint, but I feel like that doesn't actually make things clearer...

[WebKit Review Bot]

Comment on attachment 131727 [details]
Patch

Clearing flags on attachment: 131727

Committed r111040: <http://trac.webkit.org/changeset/111040>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.