Bug 115455 – [CSS Exclusions] Programmatic layout tests fail when subpixel layout is disabled

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Hans Muller]

The following programmatic layout tests failed when subpixel layout was disabled for the OSX port (see http://trac.webkit.org/changeset/149224)

fast/exclusions/shape-inside/shape-inside-polygon-layout.html
fast/exclusions/shape-inside/shape-inside-polygon-padding-003.html
fast/exclusions/shape-outside-floats/shape-outside-floats-diamond-margin-polygon.html
fast/exclusions/shape-outside-floats/shape-outside-floats-ellipse-margin-right.html
fast/exclusions/shape-outside-floats/shape-outside-floats-ellipse-margin-left.html

[Hans Muller]

Created attachment 200218 [details]
Patch

Note: shape-outside-floats-ellipse-margin-right.html shouldn't have been included in the list of failing programatic tests.  It has been updated to sync it up the other changes.

[Hans Muller]

Comment on attachment 200218 [details]
Patch

Last week on Friday, WebKit disabled subpixel layout for Mac, and 13 of the exclusions tests failed.  This turned out to be a bit of blessing in disguise, it exposed some implementation and test problems.  

This is the first patch of likely 2 or 3.  It restores 4 tests that were failing, and one that wasn't:

fast/exclusions/shape-inside/shape-inside-polygon-layout.html 
fast/exclusions/shape-inside/shape-inside-polygon-padding-003.html
fast/exclusions/shape-outside-floats/shape-outside-floats-diamond-margin-polygon.html
fast/exclusions/shape-outside-floats/shape-outside-floats-ellipse-margin-left.html

fast/exclusions/shape-inside/shape-inside-rounded-rectangle-fit-002.html

[Dirk Schulze]

Comment on attachment 200218 [details]
Patch

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

> LayoutTests/fast/exclusions/shape-inside/shape-inside-polygon-layout.html:61
> +  shouldBe("shapeInsideRect('a').top", String(lineATop), quiet);
> +  shouldBe("shapeInsideRect('b').top", String(lineATop + 15), quiet);
> +  shouldBe("shapeInsideRect('c').top", String(lineATop + 30), quiet);
> +  shouldBe("shapeInsideRect('d').top", String(lineATop + 45), quiet);

Why are they passing quietly?

> LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-ellipse-margin-right.html:78
> -SubPixelLayout.initSubPixelLayout();
> +var quiet = true; // PASS output depends on SubPixelLayout.isEnabled()

I see. You want to avoid different output if subpixels are enabled rather then not enabled. Do I understand that correctly? Why don't you use shouldBeCloseTo instead? I think you want to see pass messages.

[Hans Muller]

(In reply to comment #3)
> (From update of attachment 200218 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200218&action=review
> 
> > LayoutTests/fast/exclusions/shape-inside/shape-inside-polygon-layout.html:61
> > +  shouldBe("shapeInsideRect('a').top", String(lineATop), quiet);
> > +  shouldBe("shapeInsideRect('b').top", String(lineATop + 15), quiet);
> > +  shouldBe("shapeInsideRect('c').top", String(lineATop + 30), quiet);
> > +  shouldBe("shapeInsideRect('d').top", String(lineATop + 45), quiet);
> 
> Why are they passing quietly?
> 
> > LayoutTests/fast/exclusions/shape-outside-floats/shape-outside-floats-ellipse-margin-right.html:78
> > -SubPixelLayout.initSubPixelLayout();
> > +var quiet = true; // PASS output depends on SubPixelLayout.isEnabled()
> 
> I see. You want to avoid different output if subpixels are enabled rather then not enabled. Do I understand that correctly? Why don't you use shouldBeCloseTo instead? I think you want to see pass messages.

Yes, that's why I used the quiet parameter. I didn't use shouldBeCloseTo(), since it's for checking if a value is within a tolerance and its PASSED message reports the expected value.  Specifying a non-zero tolerance would work in these cases, but that would be a weaker test.

[Dirk Schulze]

Comment on attachment 200218 [details]
Patch

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

>>> LayoutTests/fast/exclusions/shape-inside/shape-inside-polygon-layout.html:61
>>> +  shouldBe("shapeInsideRect('d').top", String(lineATop + 45), quiet);
>> 
>> Why are they passing quietly?
> 
> Yes, that's why I used the quiet parameter. I didn't use shouldBeCloseTo(), since it's for checking if a value is within a tolerance and its PASSED message reports the expected value.  Specifying a non-zero tolerance would work in these cases, but that would be a weaker test.

As long as we don't possibly get a wrong positive, it is fine with me. r=me.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 200218 [details]:

media/video-layer-crash.html bug 114744 (authors: [e-mail], [e-mail], [e-mail], and [e-mail])
fast/frames/crash-remove-iframe-during-object-beforeload.html bug 115322 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 200218 [details]
Patch

Clearing flags on attachment: 200218

Committed r149457: <http://trac.webkit.org/changeset/149457>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

*** Bug 116947 has been marked as a duplicate of this bug. ***