Bug 71240 - Switch background/border image back to Int
Summary: Switch background/border image back to Int
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Emil A Eklund
URL:
Keywords:
Depends on:
Blocks: 63567
  Show dependency treegraph
 
Reported: 2011-10-31 15:16 PDT by Emil A Eklund
Modified: 2011-11-01 17:56 PDT (History)
3 users (show)

See Also:


Attachments
Patch (31.79 KB, patch)
2011-10-31 15:20 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (31.81 KB, patch)
2011-10-31 17:25 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch (26.16 KB, patch)
2011-11-01 11:04 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff
Patch for landing (22.39 KB, patch)
2011-11-01 16:10 PDT, Emil A Eklund
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Emil A Eklund 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.
Comment 1 Emil A Eklund 2011-10-31 15:20:38 PDT
Created attachment 113091 [details]
Patch
Comment 2 Emil A Eklund 2011-10-31 17:25:40 PDT
Created attachment 113113 [details]
Patch
Comment 3 Daniel Bates 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
Comment 4 Emil A Eklund 2011-11-01 11:04:43 PDT
Created attachment 113197 [details]
Patch
Comment 5 Darin Adler 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.
Comment 6 Emil A Eklund 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.
Comment 7 Emil A Eklund 2011-11-01 16:10:28 PDT
Created attachment 113248 [details]
Patch for landing
Comment 8 Emil A Eklund 2011-11-01 16:11:24 PDT
I reverted all changes unrelated to background and border images instead. Seemed like the better option.
Comment 9 WebKit Review Bot 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>
Comment 10 WebKit Review Bot 2011-11-01 17:56:10 PDT
All reviewed patches have been landed.  Closing bug.