Bug 79283 – Add roundToInt method for LayoutUnits

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

[Levi Weintraub]

As specified in https://trac.webkit.org/wiki/LayoutUnit, there are cases where we want to round LayoutUnits to integers. Adding an inline stub function for when we transition to sub-pixel positioning.

[Levi Weintraub]

Created attachment 128310 [details]
Patch

[Philippe Normand]

Comment on attachment 128310 [details]
Patch

Attachment 128310 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11561657

[Levi Weintraub]

Created attachment 128320 [details]
Patch

[Levi Weintraub]

Pinging reviewers.

[Eric Seidel (no email)]

Comment on attachment 128320 [details]
Patch

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

> Source/WebCore/dom/MouseRelatedEvent.cpp:217
> +    return roundToInt(m_offsetLocation.x());

I think we want this to be a member function once that's practical.  x().round() reads better to me than roundToInt(x()).

> Source/WebCore/rendering/RenderBoxModelObject.h:64
> +    int pixelSnappedOffsetLeft() const { return roundToInt(offsetLeft()); }
> +    int pixelSnappedOffsetTop() const { return roundToInt(offsetTop()); }

Are these really pixel snapped? For left/top these seem OK, but writing right/left accessors the same would clearly be wrong, no?  Shouldn't we always work from a rect when possible isntead of individual accessors?

[Levi Weintraub]

Comment on attachment 128320 [details]
Patch

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

>> Source/WebCore/dom/MouseRelatedEvent.cpp:217
>> +    return roundToInt(m_offsetLocation.x());
> 
> I think we want this to be a member function once that's practical.  x().round() reads better to me than roundToInt(x()).

I agree, and am happy to volunteer to do that when this conversion is done. I'd be curious to hear Darin's opinion though, as he's felt strongly about these sorts of things in the past.

>> Source/WebCore/rendering/RenderBoxModelObject.h:64
>> +    int pixelSnappedOffsetTop() const { return roundToInt(offsetTop()); }
> 
> Are these really pixel snapped? For left/top these seem OK, but writing right/left accessors the same would clearly be wrong, no?  Shouldn't we always work from a rect when possible isntead of individual accessors?

You're picking up correctly that this is okay explicitly because it's top/left. It's definitely worth spending some time fixing up callers to avoid breaking rects apart.

[Eric Seidel (no email)]

(In reply to comment #6)
> (From update of attachment 128320 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=128320&action=review
> >> Source/WebCore/rendering/RenderBoxModelObject.h:64
> >> +    int pixelSnappedOffsetTop() const { return roundToInt(offsetTop()); }
> > 
> > Are these really pixel snapped? For left/top these seem OK, but writing right/left accessors the same would clearly be wrong, no?  Shouldn't we always work from a rect when possible isntead of individual accessors?
> 
> You're picking up correctly that this is okay explicitly because it's top/left. It's definitely worth spending some time fixing up callers to avoid breaking rects apart.

We have this general problem all over the rendering tree... which is why it was so great that you and eae started by deploying rect/size wider.  Unfortunately that work is still not really "done" and we'll need to do more of it after we flip over to fixed point.  I suspect we may not even be able to get the snapping perfect until we plumb whole rects more thoroughly through the system?

[Eric Seidel (no email)]

Then again I may just be riding this pet issue too long, and we already have rect/size sufficiently deployed.  You two would know much better than I these days.

[Levi Weintraub]

(In reply to comment #8)
> Then again I may just be riding this pet issue too long, and we already have rect/size sufficiently deployed.  You two would know much better than I these days.

I agree there's likely more work to do in preserving rects throughout rendering, but for what it's worth, offering pixelSnapped versions can potentially lessen misuse. We actually only use these methods in our branch in PrintContext, from outside rendering code. That sort of use is probably okay.

[Levi Weintraub]

(In reply to comment #9)
> (In reply to comment #8)
> > Then again I may just be riding this pet issue too long, and we already have rect/size sufficiently deployed.  You two would know much better than I these days.
> 
> I agree there's likely more work to do in preserving rects throughout rendering, but for what it's worth, offering pixelSnapped versions can potentially lessen misuse. We actually only use these methods in our branch in PrintContext, from outside rendering code. That sort of use is probably okay.

Is this going to prevent a positive review?

[Eric Seidel (no email)]

Nope, just one of many patches forgotten in the queue.  I'll take another look this afternoon.

[Eric Seidel (no email)]

Comment on attachment 128320 [details]
Patch

OK.

[Levi Weintraub]

Comment on attachment 128320 [details]
Patch

Thanks Eric!

[WebKit Review Bot]

Comment on attachment 128320 [details]
Patch

Rejecting attachment 128320 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/11779522

[WebKit Review Bot]

The commit-queue encountered the following flaky tests while processing attachment 128320 [details]:

perf/array-reverse.html bug 78593 (author: [e-mail])
The commit-queue is continuing to process your patch.

[Levi Weintraub]

(In reply to comment #14)
> (From update of attachment 128320 [details])
> Rejecting attachment 128320 [details] from commit-queue.
> 
> Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1
> 
> ERROR: /mnt/git/webkit-commit-queue/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
> 
> Full output: http://queues.webkit.org/results/11779522

Whoa... it has the line to be filled in. Tools failure? I guess I'll land manually...

[Levi Weintraub]

Comment on attachment 128320 [details]
Patch

Clearing flags on attachment: 128320

Committed r109378: <http://trac.webkit.org/changeset/109378>

[Levi Weintraub]

All reviewed patches have been landed.  Closing bug.