Bug 128036

Summary:

Subpixel rendering: Make BorderEdge/RoundedRect::Radii LayoutUnit aware.

Product:

WebKit

Reporter:

alan <zalan>

Component:

Layout and Rendering

Assignee:

alan <zalan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, bunhere, cdumez, commit-queue, darin, esprehn+autocc, glenn, gyuyoung.kim, jonlee, kondapallykalyan, rniwa, simon.fraser, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

128056

Bug Blocks:

127524

Attachments:

Description	Flags
Patch	none
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2	none
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion	none
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2	none
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion	none
Patch	none
Patch	none

alan

Reported 2014-01-31 21:03:15 PST

ssia

Attachments
Patch (16.40 KB, patch) 2014-01-31 23:34 PST, alan	no flags	Details Formatted Diff Diff
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (2.03 MB, application/zip) 2014-02-01 00:36 PST, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (2.07 MB, application/zip) 2014-02-01 01:03 PST, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (2.03 MB, application/zip) 2014-02-01 01:43 PST, Build Bot	no flags	Details
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (2.07 MB, application/zip) 2014-02-01 02:02 PST, Build Bot	no flags	Details
Patch (25.14 KB, patch) 2014-02-01 21:01 PST, alan	no flags	Details Formatted Diff Diff
Patch (25.85 KB, patch) 2014-02-02 00:30 PST, alan	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2014-01-31 22:14:41 PST

<rdar://problem/15963574>

alan

Comment 2 2014-01-31 23:34:49 PST

Created attachment 222875 [details] Patch

Build Bot

Comment 3 2014-02-01 00:36:22 PST

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

Comment 4 2014-02-01 00:36:23 PST

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 5 2014-02-01 01:03:48 PST

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

Comment 6 2014-02-01 01:03:51 PST

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 7 2014-02-01 01:43:35 PST

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

Comment 8 2014-02-01 01:43:37 PST

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 9 2014-02-01 02:02:22 PST

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

Comment 10 2014-02-01 02:02:25 PST

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 11 2014-02-01 08:10:16 PST

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

Comment 12 2014-02-01 08:10:43 PST

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

alan

Comment 13 2014-02-01 08:32:01 PST

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

alan

Comment 14 2014-02-01 10:00:45 PST

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

alan

Comment 15 2014-02-01 19:04:24 PST

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

alan

Comment 16 2014-02-01 19:08:00 PST

LayoutUnit operator++ bug 128056

alan

Comment 17 2014-02-01 21:01:33 PST

Created attachment 222903 [details] Patch

alan

Comment 18 2014-02-01 21:03:12 PST

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.

alan

Comment 19 2014-02-02 00:30:48 PST

Created attachment 222915 [details] Patch

alan

Comment 20 2014-02-02 00:31:29 PST

Comment on attachment 222915 [details] Patch EWS testing.

WebKit Commit Bot

Comment 21 2014-02-02 08:10:36 PST

Comment on attachment 222915 [details] Patch Clearing flags on attachment: 222915 Committed r163262: <http://trac.webkit.org/changeset/163262>

WebKit Commit Bot

Comment 22 2014-02-02 08:10:42 PST

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.