Color bleeding with rounded rectangles on high dpi displays

[Justin Novosad]

Color bleeding with rounded rectangles on high dpi displays

[Justin Novosad]

Created attachment 181749 [details]
Patch

[Justin Novosad]

Created attachment 181754 [details]
Patch

[Eric Seidel (no email)]

I wonder if this is related to bug 103855?

[Simon Fraser (smfr)]

And bug 63952.

[Simon Fraser (smfr)]

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'

[Levi Weintraub]

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.

[Simon Fraser (smfr)]

Comment on attachment 181754 [details]
Patch

r=me with comments addressed

[Justin Novosad]

(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...

[Justin Novosad]

Created attachment 181781 [details]
Patch for landing

[Justin Novosad]

(In reply to comment #4)
> And bug 63952.

bug 63952 definitely looks related.  Not sure about bug 103855 though

[WebKit Review Bot]

Comment on attachment 181781 [details]
Patch for landing

Clearing flags on attachment: 181781

Committed r139137: <http://trac.webkit.org/changeset/139137>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dimitri Glazkov (Google)]

(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

[noel gordon]

New chromium-win pixel baseline for gradient-background-leakage-hidpi.html
http://trac.webkit.org/changeset/139204

[noel gordon]

New chromium-mac pixel baseline for gradient-background-leakage-hidpi.html
http://trac.webkit.org/changeset/139288

[Justin Novosad]

(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.

[Justin Novosad]

All baselines are fine now.  Case closed