Bug 80039 - Update usage of LayoutUnits in RenderBox
Summary: Update usage of LayoutUnits in RenderBox
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Levi Weintraub
Depends on:
Blocks: 60318
  Show dependency treegraph
Reported: 2012-03-01 11:45 PST by Levi Weintraub
Modified: 2012-03-05 18:43 PST (History)
6 users (show)

See Also:

Patch (15.66 KB, patch)
2012-03-01 12:15 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch for landing (15.73 KB, patch)
2012-03-05 16:20 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]
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]

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]

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]

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.