Bug 106373

Summary: Color bleeding with rounded rectangles on high dpi displays
Product: WebKit Reporter: Justin Novosad <junov>
Component: New BugsAssignee: Justin Novosad <junov>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, dglazkov, eae, eric, leviw, noel.gordon, ojan.autocc, senorblanco, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing none

Description Justin Novosad 2013-01-08 13:43:43 PST
Color bleeding with rounded rectangles on high dpi displays
Comment 1 Justin Novosad 2013-01-08 13:52:19 PST
Created attachment 181749 [details]
Patch
Comment 2 Justin Novosad 2013-01-08 14:01:41 PST
Created attachment 181754 [details]
Patch
Comment 3 Eric Seidel (no email) 2013-01-08 14:26:58 PST
I wonder if this is related to bug 103855?
Comment 4 Simon Fraser (smfr) 2013-01-08 14:55:14 PST
And bug 63952.
Comment 5 Simon Fraser (smfr) 2013-01-08 14:56:15 PST
Comment on attachment 181754 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181754&action=review

> Source/WebCore/rendering/RenderBox.cpp:999
> +    // Because RoundedRect uses IntRect internally the inset applied by the 

Why don't we make it use FloatRect then?

> Source/WebCore/rendering/RenderBox.cpp:1011
> +    if (contextScaling.width() > 1.0f) 
> +        contextScaling.setWidth(1.0f);
> +    if (contextScaling.height() > 1.0f) 
> +        contextScaling.setHeight(1.0f);

Those can just all be '1'
Comment 6 Levi Weintraub 2013-01-08 15:00:11 PST
Comment on attachment 181754 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181754&action=review

> Source/WebCore/rendering/RenderBox.cpp:1007
> +    // This precaution will become obsolete if RoundedRect is ever promoted to
> +    // a sub-pixel representation.

We currently don't use sub-pixel rendering (absent transforms), even with sub-pixel layout. This is why we represent RoundedRect with an IntRect.
Comment 7 Simon Fraser (smfr) 2013-01-08 15:03:03 PST
Comment on attachment 181754 [details]
Patch

r=me with comments addressed
Comment 8 Justin Novosad 2013-01-08 15:11:31 PST
(In reply to comment #5)
> (From update of attachment 181754 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181754&action=review
> 
> > Source/WebCore/rendering/RenderBox.cpp:999
> > +    // Because RoundedRect uses IntRect internally the inset applied by the 
> 
> Why don't we make it use FloatRect then?

I'd rather address that separately because it is kind of a big deal. It raises important questions. 
My main worry is how such a change might affect anti-aliased border rendering. Perhaps we should be snapping to integer coordinates in screen-space (post CTM)? As long as the CTM is only translation+scale?  We need to give this some thought...
Comment 9 Justin Novosad 2013-01-08 15:18:44 PST
Created attachment 181781 [details]
Patch for landing
Comment 10 Justin Novosad 2013-01-08 15:33:23 PST
(In reply to comment #4)
> And bug 63952.

bug 63952 definitely looks related.  Not sure about bug 103855 though
Comment 11 WebKit Review Bot 2013-01-08 17:18:41 PST
Comment on attachment 181781 [details]
Patch for landing

Clearing flags on attachment: 181781

Committed r139137: <http://trac.webkit.org/changeset/139137>
Comment 12 WebKit Review Bot 2013-01-08 17:18:45 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Dimitri Glazkov (Google) 2013-01-09 08:58:50 PST
(In reply to comment #12)
> All reviewed patches have been landed.  Closing bug.

Were you planning to update expectations for other platforms? I see a bunch of MISSING on the dashboard:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=fast%2Fbackgrounds%2Fgradient-background-leakage-hidpi.html
Comment 14 noel gordon 2013-01-09 23:27:31 PST
New chromium-win pixel baseline for gradient-background-leakage-hidpi.html
http://trac.webkit.org/changeset/139204
Comment 15 noel gordon 2013-01-09 23:30:44 PST
New chromium-mac pixel baseline for gradient-background-leakage-hidpi.html
http://trac.webkit.org/changeset/139288
Comment 16 Justin Novosad 2013-01-10 06:29:17 PST
(In reply to comment #13)
> 
> Were you planning to update expectations for other platforms? I see a bunch of MISSING on the dashboard:

Sorry about that, looks like I accidentally clobbered my temporary TestExpectations edits. Noel, thanks for the gardening.  I think we have all we need now.  I'll just wait for green on the remaining mac dbg bots before re-closing the bug.
Comment 17 Justin Novosad 2013-01-22 13:06:10 PST
All baselines are fine now.  Case closed