Bug 71240

Summary: Switch background/border image back to Int
Product: WebKit Reporter: Emil A Eklund <eae>
Component: Layout and RenderingAssignee: Emil A Eklund <eae>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, leviw, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 63567    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

Emil A Eklund
Reported 2011-10-31 15:16:55 PDT
We went a bit too far with the LayoutRect/Point/Size conversion a while ago, it turns out that we want to keep image background and border rendering aligned to int boundaries to match up with device pixels.
Attachments
Patch (31.79 KB, patch)
2011-10-31 15:20 PDT, Emil A Eklund 		no flags
DetailsFormatted DiffDiff
Patch (31.81 KB, patch)
2011-10-31 17:25 PDT, Emil A Eklund 		no flags
DetailsFormatted DiffDiff
Patch (26.16 KB, patch)
2011-11-01 11:04 PDT, Emil A Eklund 		no flags
DetailsFormatted DiffDiff
Patch for landing (22.39 KB, patch)
2011-11-01 16:10 PDT, Emil A Eklund 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Emil A Eklund
Comment 1 2011-10-31 15:20:38 PDT
Created attachment 113091 [details] Patch
Emil A Eklund
Comment 2 2011-10-31 17:25:40 PDT
Created attachment 113113 [details] Patch
Daniel Bates
Comment 3 2011-10-31 19:24:40 PDT
Comment on attachment 113113 [details] Patch Attachment 113113 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/10245500
Emil A Eklund
Comment 4 2011-11-01 11:04:43 PDT
Created attachment 113197 [details] Patch
Darin Adler
Comment 5 2011-11-01 14:04:14 PDT
Comment on attachment 113197 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=113197&action=review This is a mix of two changes. About 2/3 of this is changing background and border back to int, and about 1/3 is changes for the newer version of LayoutUnit where implicit conversion from int is not allowed. It would be better if the change log told the truth about that. > Source/WebCore/rendering/RenderBoxModelObject.cpp:441 > - LayoutUnit xPos = (isBox() ? toRenderBox(this)->left() : 0); > - > + LayoutUnit xPos = (isBox() ? toRenderBox(this)->left() : LayoutUnit(0)); This change is not part of changing background and border back to int. > Source/WebCore/rendering/RenderBoxModelObject.cpp:475 > - LayoutUnit yPos = (isBox() ? toRenderBox(this)->top() : 0); > + LayoutUnit yPos = (isBox() ? toRenderBox(this)->top() : LayoutUnit(0)); Neither is this. > Source/WebCore/rendering/RenderBoxModelObject.cpp:657 > - LayoutUnit bLeft = includeLeftEdge ? borderLeft() : 0; > - LayoutUnit bRight = includeRightEdge ? borderRight() : 0; > - LayoutUnit pLeft = includeLeftEdge ? paddingLeft() : 0; > - LayoutUnit pRight = includeRightEdge ? paddingRight() : 0; > + LayoutUnit bLeft = includeLeftEdge ? borderLeft() : LayoutUnit(0); > + LayoutUnit bRight = includeRightEdge ? borderRight() : LayoutUnit(0); > + LayoutUnit pLeft = includeLeftEdge ? paddingLeft() : LayoutUnit(0); > + LayoutUnit pRight = includeRightEdge ? paddingRight() : LayoutUnit(0); Neither is this. > Source/WebCore/rendering/RenderBoxModelObject.cpp:679 > - LayoutRect clipRect = LayoutRect(scrolledPaintRect.x() + bLeft + (includePadding ? pLeft : 0), > - scrolledPaintRect.y() + borderTop() + (includePadding ? paddingTop() : 0), > - scrolledPaintRect.width() - bLeft - bRight - (includePadding ? pLeft + pRight : 0), > - scrolledPaintRect.height() - borderTop() - borderBottom() - (includePadding ? paddingTop() + paddingBottom() : 0)); > + LayoutRect clipRect = LayoutRect(scrolledPaintRect.x() + bLeft + (includePadding ? pLeft : LayoutUnit(0)), > + scrolledPaintRect.y() + borderTop() + (includePadding ? paddingTop() : LayoutUnit(0)), > + scrolledPaintRect.width() - bLeft - bRight - (includePadding ? pLeft + pRight : LayoutUnit(0)), > + scrolledPaintRect.height() - borderTop() - borderBottom() - (includePadding ? paddingTop() + paddingBottom() : LayoutUnit(0))); Neither is this.
Emil A Eklund
Comment 6 2011-11-01 15:37:02 PDT
(In reply to comment #5) > (From update of attachment 113197 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=113197&action=review > > This is a mix of two changes. About 2/3 of this is changing background and border back to int, and about 1/3 is changes for the newer version of LayoutUnit where implicit conversion from int is not allowed. It would be better if the change log told the truth about that. Good point, I'll update the ChangeLog to be explicit about this and try to avoid making unrelated changes going forward. Thanks.
Emil A Eklund
Comment 7 2011-11-01 16:10:28 PDT
Created attachment 113248 [details] Patch for landing
Emil A Eklund
Comment 8 2011-11-01 16:11:24 PDT
I reverted all changes unrelated to background and border images instead. Seemed like the better option.
WebKit Review Bot
Comment 9 2011-11-01 17:56:06 PDT
Comment on attachment 113248 [details] Patch for landing Clearing flags on attachment: 113248 Committed r99019: <http://trac.webkit.org/changeset/99019>
WebKit Review Bot
Comment 10 2011-11-01 17:56:10 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.