Bug 79283

Summary: Add roundToInt method for LayoutUnits
Product: WebKit Reporter: Levi Weintraub <leviw>
Component: Layout and RenderingAssignee: Levi Weintraub <leviw>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, eae, eric, gns, pnormand, simon.fraser, tkent, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: https://trac.webkit.org/wiki/LayoutUnit
Bug Depends on:    
Bug Blocks: 60318    
Attachments:
Description Flags
Patch
none
Patch none

Description Levi Weintraub 2012-02-22 15:18:17 PST
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.
Comment 1 Levi Weintraub 2012-02-22 15:54:57 PST
Created attachment 128310 [details]
Patch
Comment 2 Philippe Normand 2012-02-22 16:13:54 PST
Comment on attachment 128310 [details]
Patch

Attachment 128310 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11561657
Comment 3 Levi Weintraub 2012-02-22 16:16:52 PST
Created attachment 128320 [details]
Patch
Comment 4 Levi Weintraub 2012-02-23 13:32:15 PST
Pinging reviewers.
Comment 5 Eric Seidel (no email) 2012-02-23 13:54:23 PST
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?
Comment 6 Levi Weintraub 2012-02-23 14:14:15 PST
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.
Comment 7 Eric Seidel (no email) 2012-02-23 14:21:14 PST
(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?
Comment 8 Eric Seidel (no email) 2012-02-23 14:22:04 PST
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.
Comment 9 Levi Weintraub 2012-02-27 08:42:23 PST
(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.
Comment 10 Levi Weintraub 2012-02-29 13:33:00 PST
(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?
Comment 11 Eric Seidel (no email) 2012-02-29 13:37:38 PST
Nope, just one of many patches forgotten in the queue.  I'll take another look this afternoon.
Comment 12 Eric Seidel (no email) 2012-02-29 17:06:22 PST
Comment on attachment 128320 [details]
Patch

OK.
Comment 13 Levi Weintraub 2012-03-01 08:01:04 PST
Comment on attachment 128320 [details]
Patch

Thanks Eric!
Comment 14 WebKit Review Bot 2012-03-01 09:17:15 PST
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
Comment 15 WebKit Review Bot 2012-03-01 09:17:51 PST
The commit-queue encountered the following flaky tests while processing attachment 128320 [details]:

perf/array-reverse.html bug 78593 (author: ojan@chromium.org)
The commit-queue is continuing to process your patch.
Comment 16 Levi Weintraub 2012-03-01 09:19:15 PST
(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...
Comment 17 Levi Weintraub 2012-03-01 11:33:51 PST
Comment on attachment 128320 [details]
Patch

Clearing flags on attachment: 128320

Committed r109378: <http://trac.webkit.org/changeset/109378>
Comment 18 Levi Weintraub 2012-03-01 11:33:59 PST
All reviewed patches have been landed.  Closing bug.