Bug 128036 – Subpixel rendering: Make BorderEdge/RoundedRect::Radii LayoutUnit aware.

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[zalan]

ssia

[Radar WebKit Bug Importer]

<rdar://problem/15963574>

[zalan]

Created attachment 222875 [details]
Patch

[Build Bot]

Comment on attachment 222875 [details]
Patch

Attachment 222875 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6245396569391104

New failing tests:
svg/zoom/page/zoom-background-images.html
media/video-zoom.html
svg/zoom/page/zoom-svg-float-border-padding.xml
fast/css/bidi-override-in-anonymous-block.html
fast/multicol/span/anonymous-style-inheritance.html
ietestcenter/css3/bordersbackgrounds/border-top-left-radius-values-003.htm
svg/zoom/page/zoom-background-image-tiled.html
svg/zoom/page/zoom-svg-through-object-with-auto-size.html
tables/mozilla_expected_failures/bugs/bug1055-2.html
svg/zoom/page/zoom-img-preserveAspectRatio-support-1.html
ietestcenter/css3/bordersbackgrounds/border-radius-with-three-values-001.htm
ietestcenter/css3/bordersbackgrounds/border-radius-style-004.htm
css1/units/length_units.html
ietestcenter/css3/bordersbackgrounds/border-radius-with-two-values-001.htm
ietestcenter/css3/bordersbackgrounds/border-radius-initial-value-001.htm
fast/repaint/repaint-during-scroll-with-zoom.html
svg/custom/svg-fonts-in-html.html
ietestcenter/css3/bordersbackgrounds/border-radius-style-001.htm
ietestcenter/css3/bordersbackgrounds/border-radius-style-002.htm

[Build Bot]

Created attachment 222878 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 222875 [details]
Patch

Attachment 222875 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6390575053930496

New failing tests:
svg/zoom/page/zoom-background-images.html
media/video-zoom.html
svg/zoom/page/zoom-svg-float-border-padding.xml
fast/css/bidi-override-in-anonymous-block.html
fast/multicol/span/anonymous-style-inheritance.html
ietestcenter/css3/bordersbackgrounds/border-top-left-radius-values-003.htm
svg/zoom/page/zoom-background-image-tiled.html
svg/zoom/page/zoom-svg-through-object-with-auto-size.html
tables/mozilla_expected_failures/bugs/bug1055-2.html
svg/zoom/page/zoom-img-preserveAspectRatio-support-1.html
ietestcenter/css3/bordersbackgrounds/border-radius-with-three-values-001.htm
ietestcenter/css3/bordersbackgrounds/border-radius-style-004.htm
css1/units/length_units.html
ietestcenter/css3/bordersbackgrounds/border-radius-with-two-values-001.htm
ietestcenter/css3/bordersbackgrounds/border-radius-initial-value-001.htm
fast/repaint/repaint-during-scroll-with-zoom.html
svg/custom/svg-fonts-in-html.html
ietestcenter/css3/bordersbackgrounds/border-radius-style-001.htm
ietestcenter/css3/bordersbackgrounds/border-radius-style-002.htm

[Build Bot]

Created attachment 222879 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 222875 [details]
Patch

Attachment 222875 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6615476721418240

New failing tests:
svg/zoom/page/zoom-background-images.html
media/video-zoom.html
svg/zoom/page/zoom-svg-float-border-padding.xml
fast/css/bidi-override-in-anonymous-block.html
fast/multicol/span/anonymous-style-inheritance.html
ietestcenter/css3/bordersbackgrounds/border-top-left-radius-values-003.htm
svg/zoom/page/zoom-background-image-tiled.html
svg/zoom/page/zoom-svg-through-object-with-auto-size.html
tables/mozilla_expected_failures/bugs/bug1055-2.html
svg/zoom/page/zoom-img-preserveAspectRatio-support-1.html
ietestcenter/css3/bordersbackgrounds/border-radius-with-three-values-001.htm
ietestcenter/css3/bordersbackgrounds/border-radius-style-004.htm
css1/units/length_units.html
ietestcenter/css3/bordersbackgrounds/border-radius-with-two-values-001.htm
ietestcenter/css3/bordersbackgrounds/border-radius-initial-value-001.htm
fast/repaint/repaint-during-scroll-with-zoom.html
svg/custom/svg-fonts-in-html.html
ietestcenter/css3/bordersbackgrounds/border-radius-style-001.htm
ietestcenter/css3/bordersbackgrounds/border-radius-style-002.htm

[Build Bot]

Created attachment 222880 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 222875 [details]
Patch

Attachment 222875 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6437184039026688

New failing tests:
svg/zoom/page/zoom-background-images.html
media/video-zoom.html
svg/zoom/page/zoom-svg-float-border-padding.xml
fast/css/bidi-override-in-anonymous-block.html
fast/multicol/span/anonymous-style-inheritance.html
ietestcenter/css3/bordersbackgrounds/border-top-left-radius-values-003.htm
svg/zoom/page/zoom-background-image-tiled.html
svg/zoom/page/zoom-svg-through-object-with-auto-size.html
tables/mozilla_expected_failures/bugs/bug1055-2.html
svg/zoom/page/zoom-img-preserveAspectRatio-support-1.html
ietestcenter/css3/bordersbackgrounds/border-radius-with-three-values-001.htm
ietestcenter/css3/bordersbackgrounds/border-radius-style-004.htm
css1/units/length_units.html
ietestcenter/css3/bordersbackgrounds/border-radius-with-two-values-001.htm
ietestcenter/css3/bordersbackgrounds/border-radius-initial-value-001.htm
fast/repaint/repaint-during-scroll-with-zoom.html
svg/custom/svg-fonts-in-html.html
ietestcenter/css3/bordersbackgrounds/border-radius-style-001.htm
ietestcenter/css3/bordersbackgrounds/border-radius-style-002.htm

[Build Bot]

Created attachment 222881 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Darin Adler]

Comment on attachment 222875 [details]
Patch

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

> Source/WebCore/rendering/RenderBoxModelObject.cpp:1459
>          if (fullWidth % 3 == 2)
> -            outerWidth += 1;
> +            outerWidth += LayoutUnit::fromPixel(1);
>  
>          if (fullWidth % 3 == 1)
> -            innerWidth += 1;
> +            innerWidth += LayoutUnit::fromPixel(1);
>      }

Why is this change needed? There’s a += operator on LayoutUnit that works with integers and does fromPixel, right?

> Source/WebCore/rendering/RenderBoxModelObject.cpp:2064
> +                LayoutUnit inflateValue = LayoutUnit::fromPixel(1);
> +                outerBorderTopWidth+= inflateValue;
> +                outerBorderBottomWidth+= inflateValue;
> +                outerBorderLeftWidth+= inflateValue;
> +                outerBorderRightWidth+= inflateValue;

Why does this need to change at all? LayoutUnit already has an operator++ that should do the right thing.

Also, formatting mistakes here.

[Darin Adler]

But all those failing tests means maybe this can’t land?

[zalan]

(In reply to comment #12)
> But all those failing tests means maybe this can’t land?

Apparently some pixel snapping is required to make this patch pass. (as it removed implicit integer flooring by going from int to LayoutUnit)

[zalan]

(In reply to comment #11)
> (From update of attachment 222875 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222875&action=review
> 
> > Source/WebCore/rendering/RenderBoxModelObject.cpp:1459
> >          if (fullWidth % 3 == 2)
> > -            outerWidth += 1;
> > +            outerWidth += LayoutUnit::fromPixel(1);
> >  
> >          if (fullWidth % 3 == 1)
> > -            innerWidth += 1;
> > +            innerWidth += LayoutUnit::fromPixel(1);
> >      }
> 
> Why is this change needed? There’s a += operator on LayoutUnit that works with integers and does fromPixel, right?
You're right. Probably there's no need to be explicit here about the LayoutUnit conversion. I'll remove both.

[zalan]

> > Source/WebCore/rendering/RenderBoxModelObject.cpp:2064
> > +                LayoutUnit inflateValue = LayoutUnit::fromPixel(1);
> > +                outerBorderTopWidth+= inflateValue;
> > +                outerBorderBottomWidth+= inflateValue;
> > +                outerBorderLeftWidth+= inflateValue;
> > +                outerBorderRightWidth+= inflateValue;
> 
> Why does this need to change at all? LayoutUnit already has an operator++ that should do the right thing.
Apparently LayoutUnit has broken operator++. It defines the post-increment operator++ only, but the implementation is pre-increment. I'll fix that by adding the pre-increment operator++ and change the post-increment implementation.

[zalan]

LayoutUnit operator++ bug 128056

[zalan]

Created attachment 222903 [details]
Patch

[zalan]

Comment on attachment 222903 [details]
Patch

EWS will fail as this code depends on bug 128056. I'll test it against EWS soon after the LayoutUnit patch gets landed.

[zalan]

Created attachment 222915 [details]
Patch

[zalan]

Comment on attachment 222915 [details]
Patch

EWS testing.

[WebKit Commit Bot]

Comment on attachment 222915 [details]
Patch

Clearing flags on attachment: 222915

Committed r163262: <http://trac.webkit.org/changeset/163262>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.