Bug 85555 - Enable SUBPIXEL_LAYOUT feature flag on Chromium
Summary: Enable SUBPIXEL_LAYOUT feature flag on Chromium
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Levi Weintraub
URL:
Keywords:
Depends on: 86693 86775 86904 87099
Blocks: 85532
  Show dependency treegraph
 
Reported: 2012-05-03 16:41 PDT by Levi Weintraub
Modified: 2012-05-22 01:35 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.43 KB, patch)
2012-05-03 16:56 PDT, Levi Weintraub
eric: review+
Details | Formatted Diff | Diff
Patch (1.68 KB, patch)
2012-05-04 10:29 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (6.41 MB, application/zip)
2012-05-04 11:36 PDT, WebKit Review Bot
no flags Details
Patch (32.44 KB, patch)
2012-05-17 19:47 PDT, Levi Weintraub
eric: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (3.06 MB, application/zip)
2012-05-17 22:57 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from ec2-cr-linux-04 (3.06 MB, application/zip)
2012-05-17 23:53 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
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>