Bug 80039

Summary: Update usage of LayoutUnits in RenderBox
Product: WebKit Reporter: Levi Weintraub <leviw>
Component: Layout and RenderingAssignee: Levi Weintraub <leviw>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, eae, eric, jchaffraix, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 60318    
Attachments:
Description Flags
Patch
none
Patch for landing none

Description Levi Weintraub 2012-03-01 11:45:03 PST
Updating the usage of ints/LayoutUnits in RenderBox in preparation for subpixel positioning.

Also adding some more stub methods for use with FractionalLayoutUnits when we make the switch. These will allow us to further shrink the size of the final patch that switches us over.
Comment 1 Levi Weintraub 2012-03-01 12:15:51 PST
Created attachment 129730 [details]
Patch
Comment 2 Levi Weintraub 2012-03-05 10:59:33 PST
Pinging reviewers.
Comment 3 Julien Chaffraix 2012-03-05 15:10:17 PST
Comment on attachment 129730 [details]
Patch

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.
Comment 4 Levi Weintraub 2012-03-05 15:13:01 PST
Comment on attachment 129730 [details]
Patch

Thanks for taking the time to understand and review this patch, Julien!
Comment 5 WebKit Review Bot 2012-03-05 15:33:23 PST
Comment on attachment 129730 [details]
Patch

Rejecting attachment 129730 [details] from commit-queue.

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

Last 500 characters of output:
ceeded at 1484 (offset 10 lines).
Hunk #7 succeeded at 3895 (offset 11 lines).
Hunk #8 succeeded at 3906 (offset 11 lines).
Hunk #9 succeeded at 3927 (offset 11 lines).
patching file Source/WebCore/rendering/RenderBox.h
Hunk #1 FAILED at 127.
1 out of 5 hunks FAILED -- saving rejects to file Source/WebCore/rendering/RenderBox.h.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Julien Cha..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/11836017
Comment 6 Levi Weintraub 2012-03-05 16:20:38 PST
Created attachment 130223 [details]
Patch for landing
Comment 7 WebKit Review Bot 2012-03-05 18:43:15 PST
Comment on attachment 130223 [details]
Patch for landing

Clearing flags on attachment: 130223

Committed r109835: <http://trac.webkit.org/changeset/109835>
Comment 8 WebKit Review Bot 2012-03-05 18:43:20 PST
All reviewed patches have been landed.  Closing bug.