Bug 85555 – Enable SUBPIXEL_LAYOUT feature flag on Chromium

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Levi Weintraub]

Enabling sub-pixel layout on Chromium. It'd be great to get this landed tomorrow morning.

[Levi Weintraub]

Created attachment 140126 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 140126 [details]
Patch

I wish to subscribe to your product and/or service.

[Eric Seidel (no email)]

You should probably CC whoever is gardening tomorrow. :)

[Levi Weintraub]

Created attachment 140266 [details]
Patch

Adding a patch that applies to see the results from the Chromium EWS bot.

[WebKit Review Bot]

Comment on attachment 140266 [details]
Patch

Attachment 140266 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12629228

New failing tests:
compositing/geometry/abs-position-inside-opacity.html
compositing/geometry/fixed-in-composited.html
animations/3d/matrix-transform-type-animation.html
animations/3d/state-at-end-event-transform.html
compositing/direct-image-compositing.html
compositing/geometry/fixed-position-transform-composited-page-scale-down.html
animations/additive-transform-animations.html
accessibility/aria-disabled.html
compositing/text-on-large-layer.html
compositing/layers-inside-overflow-scroll.html
compositing/animation/state-at-end-event-transform-layer.html
compositing/sibling-positioning.html
compositing/geometry/fixed-position-iframe-composited-page-scale.html
compositing/generated-content.html
compositing/self-painting-layers.html
animations/cross-fade-webkit-mask-box-image.html
animations/3d/change-transform-in-end-event.html
animations/missing-values-last-keyframe.html
animations/cross-fade-list-style-image.html
animations/missing-values-first-keyframe.html
compositing/geometry/fixed-position-composited-page-scale.html
animations/cross-fade-background-image.html
compositing/geometry/clipping-foreground.html
animations/cross-fade-border-image-source.html
compositing/geometry/composited-html-size.html
compositing/compositing-visible-descendant.html
animations/cross-fade-webkit-mask-image.html
compositing/iframes/composited-iframe-alignment.html
compositing/color-matching/image-color-matching.html
compositing/color-matching/pdf-image-match.html

[WebKit Review Bot]

Created attachment 140286 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Levi Weintraub]

Created attachment 142615 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 142615 [details]
Patch

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

> LayoutTests/css1/units/rounding.html:40
> +shouldBe('Math.round(divtop.bottom)', '31');
> +shouldBe('Math.round(divbottom.top)', '31');

Why not test their float values and give chromium separate expectations?

> LayoutTests/fast/borders/border-radius-huge-assert.html:19
> -    <div style="-webkit-border-radius: 926179103pt;"></div>
> +    <div style="-webkit-border-radius: 92617910pt;"></div>

I'm confused by this change.

> LayoutTests/fast/css/zoom-in-length-round-trip.html:30
> +        var rect = target.getBoundingClientRect();
> +        var width = Math.round(rect.right - rect.left);
> +        var height = Math.round(rect.bottom - rect.top);

Also not clear the reasoning for this change. :)

[WebKit Review Bot]

Comment on attachment 142615 [details]
Patch

Attachment 142615 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12724389

New failing tests:
css1/font_properties/font_size.html
css1/text_properties/text_indent.html
css1/text_properties/vertical_align.html
css1/formatting_model/inline_elements.html
css1/box_properties/margin_right.html
css1/box_properties/margin.html
css1/text_properties/line_height.html
css1/formatting_model/replaced_elements.html
compositing/direct-image-compositing.html
css1/box_properties/acid_test.html
css1/font_properties/font_weight.html
css1/box_properties/margin_top.html
css1/box_properties/clear_float.html
css1/basic/inheritance.html
css1/box_properties/padding.html
css1/box_properties/padding_bottom.html
compositing/reflections/reflection-in-composited.html
compositing/shadows/shadow-drawing.html
compositing/reflections/nested-reflection-transformed.html
css1/box_properties/float_on_text_elements.html
css1/color_and_background/background_attachment.html
compositing/visibility/visibility-image-layers-dynamic.html
css1/formatting_model/vertical_formatting.html
css1/units/percentage_units.html
css1/box_properties/margin_bottom.html
css1/font_properties/font.html
css1/formatting_model/floating_elements.html
css1/formatting_model/horizontal_formatting.html

[WebKit Review Bot]

Created attachment 142636 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[WebKit Review Bot]

Comment on attachment 142615 [details]
Patch

Attachment 142615 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12719756

New failing tests:
css1/font_properties/font_size.html
css1/text_properties/text_indent.html
css1/text_properties/vertical_align.html
css1/formatting_model/inline_elements.html
css1/box_properties/margin_right.html
css1/box_properties/margin.html
css1/text_properties/line_height.html
css1/formatting_model/replaced_elements.html
compositing/direct-image-compositing.html
css1/box_properties/acid_test.html
css1/font_properties/font_weight.html
css1/box_properties/margin_top.html
css1/box_properties/clear_float.html
css1/basic/inheritance.html
css1/box_properties/padding.html
css1/box_properties/padding_bottom.html
compositing/reflections/reflection-in-composited.html
compositing/shadows/shadow-drawing.html
compositing/reflections/nested-reflection-transformed.html
css1/box_properties/float_on_text_elements.html
css1/color_and_background/background_attachment.html
compositing/visibility/visibility-image-layers-dynamic.html
css1/formatting_model/vertical_formatting.html
css1/units/percentage_units.html
css1/box_properties/margin_bottom.html
css1/font_properties/font.html
css1/formatting_model/floating_elements.html
css1/formatting_model/horizontal_formatting.html

[WebKit Review Bot]

Created attachment 142648 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Emil A Eklund]

(In reply to comment #8)
> (From update of attachment 142615 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142615&action=review
> 
> > LayoutTests/css1/units/rounding.html:40
> > +shouldBe('Math.round(divtop.bottom)', '31');
> > +shouldBe('Math.round(divbottom.top)', '31');
> 
> Why not test their float values and give chromium separate expectations?

It seemed easier to deal with int values here as it checks that the bottom and the top of the two divs align. We have other tests that ensures that the actual values match.

> > LayoutTests/fast/borders/border-radius-huge-assert.html:19
> > -    <div style="-webkit-border-radius: 926179103pt;"></div>
> > +    <div style="-webkit-border-radius: 92617910pt;"></div>
> 
> I'm confused by this change.

926179103pt overflows a FractionalLayoutUnit causing the style rule to be ignored. 92617910pt is still huge but within bounds.

> 
> > LayoutTests/fast/css/zoom-in-length-round-trip.html:30
> > +        var rect = target.getBoundingClientRect();
> > +        var width = Math.round(rect.right - rect.left);
> > +        var height = Math.round(rect.bottom - rect.top);
> 
> Also not clear the reasoning for this change. :)

Easier than dealing with asserts that has 20 decimal places.

[Emil A Eklund]

Ping?

[Eric Seidel (no email)]

Comment on attachment 142615 [details]
Patch

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

OK.

> LayoutTests/fast/dom/Window/webkitConvertPoint.html:151
> +            if (hasSubpixelSupport) {
> +                runTest("Test 1",  "test1",  8, 13, 13, 53);
> +                runTest("Test 2",  "test2",  8, 51, 13, 91);
> +                runTest("Test 3",  "test3",  8, 85, 13, 125);
> +                runTest("Test 4",  "test4",  8, 119, 13, 159);
> +                runTest("Test 5",  "test5",  28, 153, 33, 193);
> +                runTest("Test 6",  "test6",  28, 187, 33, 227);
> +                runTest("Test 7",  "test7",  8, 239, 13, 279);
> +                runTest("Test 8",  "test8",  8, 273, 13, 313);
> +                runTest("Test 9",  "test9",  28, 291, 33, 331);
> +                runTest("Test 10", "test10", 28, 309, 33, 349);
> +                runTest("Test 11", "test11", 158, 355, 174, 373);
> +                runTest("Test 12", "test12", 168, 428, 184, 446);
> +                runTest("Test 13", "test13", 28, 487, 33, 527);
> +            } else {

This seems a pain to maintain.

[Emil A Eklund]

(In reply to comment #15)
> (From update of attachment 142615 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142615&action=review
> 
> OK.
> 
> > LayoutTests/fast/dom/Window/webkitConvertPoint.html:151
> > +            if (hasSubpixelSupport) {
> > +                runTest("Test 1",  "test1",  8, 13, 13, 53);
> > +                runTest("Test 2",  "test2",  8, 51, 13, 91);
> > +                runTest("Test 3",  "test3",  8, 85, 13, 125);
> > +                runTest("Test 4",  "test4",  8, 119, 13, 159);
> > +                runTest("Test 5",  "test5",  28, 153, 33, 193);
> > +                runTest("Test 6",  "test6",  28, 187, 33, 227);
> > +                runTest("Test 7",  "test7",  8, 239, 13, 279);
> > +                runTest("Test 8",  "test8",  8, 273, 13, 313);
> > +                runTest("Test 9",  "test9",  28, 291, 33, 331);
> > +                runTest("Test 10", "test10", 28, 309, 33, 349);
> > +                runTest("Test 11", "test11", 158, 355, 174, 373);
> > +                runTest("Test 12", "test12", 168, 428, 184, 446);
> > +                runTest("Test 13", "test13", 28, 487, 33, 527);
> > +            } else {
> 
> This seems a pain to maintain.

It is already, the numbers are different for all platforms so we have a bunch of expectations with FAIL in them checked in already. We should really re-work the test as some point (or change it to just report the numbers).

[Levi Weintraub]

Committed r117815: <http://trac.webkit.org/changeset/117815>