Bug 85555

Summary: Enable SUBPIXEL_LAYOUT feature flag on Chromium
Product: WebKit Reporter: Levi Weintraub <leviw>
Component: Layout and RenderingAssignee: Levi Weintraub <leviw>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, eae, eric, fishd, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 86693, 86775, 86904, 87099    
Bug Blocks: 85532    
Attachments:
Description Flags
Patch
eric: review+
Patch
none
Archive of layout-test-results from ec2-cr-linux-04
none
Patch
eric: review+, webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-03
none
Archive of layout-test-results from ec2-cr-linux-04 none

Description Levi Weintraub 2012-05-03 16:41:43 PDT
Enabling sub-pixel layout on Chromium. It'd be great to get this landed tomorrow morning.
Comment 1 Levi Weintraub 2012-05-03 16:56:33 PDT
Created attachment 140126 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-05-03 17:03:06 PDT
Comment on attachment 140126 [details]
Patch

I wish to subscribe to your product and/or service.
Comment 3 Eric Seidel (no email) 2012-05-03 17:03:45 PDT
You should probably CC whoever is gardening tomorrow. :)
Comment 4 Levi Weintraub 2012-05-04 10:29:00 PDT
Created attachment 140266 [details]
Patch

Adding a patch that applies to see the results from the Chromium EWS bot.
Comment 5 WebKit Review Bot 2012-05-04 11:36:39 PDT
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
Comment 6 WebKit Review Bot 2012-05-04 11:36:44 PDT
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
Comment 7 Levi Weintraub 2012-05-17 19:47:27 PDT
Created attachment 142615 [details]
Patch
Comment 8 Eric Seidel (no email) 2012-05-17 20:48:28 PDT
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. :)
Comment 9 WebKit Review Bot 2012-05-17 22:56:52 PDT
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
Comment 10 WebKit Review Bot 2012-05-17 22:57:00 PDT
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
Comment 11 WebKit Review Bot 2012-05-17 23:53:36 PDT
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
Comment 12 WebKit Review Bot 2012-05-17 23:53:45 PDT
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
Comment 13 Emil A Eklund 2012-05-18 09:25:33 PDT
(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.
Comment 14 Emil A Eklund 2012-05-21 09:41:21 PDT
Ping?
Comment 15 Eric Seidel (no email) 2012-05-21 11:26:03 PDT
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.
Comment 16 Emil A Eklund 2012-05-21 11:32:32 PDT
(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).
Comment 17 Levi Weintraub 2012-05-21 14:17:41 PDT
Committed r117815: <http://trac.webkit.org/changeset/117815>