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

First round of changes to correct exclusion tests that started failing when subpixel layout was turned off, and to correct some problems that disabling subpixel layout exposed.

This set of changes just corrects four tests. It also revises the subpixel-utils code to bring it into line with the most recent LayoutUnit et al changes and to simplify its use a little. The ExclusionPolygon appendArc() method was corrected, so that the 3rd of 5 additional vertices is in the center of the arc. The firstIncludedIntervalLogicalTop() methods now use ceiledLayoutUnit() to convert a float to a LayoutUnit within the shape. 

BUG=237568
R=leviw@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=151323

[Hans Muller, 2013-05-15 01:40:38]

When subpixel layout was disabled by default for webkit/mac, about a dozen
exclusions tests failed.  They've been corrected on webkit - see
https://bugs.webkit.org/show_bug.cgi?id=115292.  I'm now migrating the changes
to Blink.

[Hans Muller, 2013-05-20 16:19:19]

Welcome back from Google Input Output. I'd appreciate a review when you have a
chance.

[eseidel, 2013-05-20 19:36:30]

Definitely a levi/emil review.  I don't believe we ever plan to support
non-subpixel layout in Blink.  We can probably remove support for the
non-subpixel path going forward.  (That's not to say this change isn't a good
one.)

[leviw_travelin_and_unemployed, 2013-05-21 19:32:05]

https://codereview.chromium.org/14959014/diff/1/Source/core/rendering/exclusi...
File Source/core/rendering/exclusions/ExclusionPolygon.cpp (right):

https://codereview.chromium.org/14959014/diff/1/Source/core/rendering/exclusi...
Source/core/rendering/exclusions/ExclusionPolygon.cpp:122: const float
arcSegmentCount = 6; // An even number so that one arc vertex will be eactly
arcRadius from arcCenter.
I don't fully understand this change... Where did this number come from?

https://codereview.chromium.org/14959014/diff/1/Source/core/rendering/exclusi...
Source/core/rendering/exclusions/ExclusionPolygon.cpp:509: result =
ceiledLayoutUnit(firstFitRect.y());
Are these different on WebKit then? They're the same on Blink. We should
probably remove one (probably ceiledLayoutUnit, honestly) to be clearer.

inline LayoutUnit ceiledLayoutUnit(float value)
{
    return LayoutUnit::fromFloatCeil(value);
}

[Hans Muller, 2013-05-21 22:03:30]

On 2013/05/21 19:32:05, Levi wrote:
>
https://codereview.chromium.org/14959014/diff/1/Source/core/rendering/exclusi...
> File Source/core/rendering/exclusions/ExclusionPolygon.cpp (right):
> 
>
https://codereview.chromium.org/14959014/diff/1/Source/core/rendering/exclusi...
> Source/core/rendering/exclusions/ExclusionPolygon.cpp:122: const float
> arcSegmentCount = 6; // An even number so that one arc vertex will be eactly
> arcRadius from arcCenter.
> I don't fully understand this change... Where did this number come from?
> 
>
https://codereview.chromium.org/14959014/diff/1/Source/core/rendering/exclusi...
> Source/core/rendering/exclusions/ExclusionPolygon.cpp:509: result =
> ceiledLayoutUnit(firstFitRect.y());
> Are these different on WebKit then? They're the same on Blink. We should
> probably remove one (probably ceiledLayoutUnit, honestly) to be clearer.
> 
> inline LayoutUnit ceiledLayoutUnit(float value)
> {
>     return LayoutUnit::fromFloatCeil(value);
> }


>
https://codereview.chromium.org/14959014/diff/1/Source/core/rendering/exclusi...
> Source/core/rendering/exclusions/ExclusionPolygon.cpp:122: const float
> arcSegmentCount = 6; // An even number so that one arc vertex will be eactly
> arcRadius from arcCenter.
> I don't fully understand this change... Where did this number come from?

A polygon's padding boundary includes circular arc segments at reflex vertices
and a margin boundary similarly includes circular arc segments at non-reflex
vertices. The arcs span less than 180 degrees and we represent them with a
linear approximation of six segments. An even number of line segments ensures
that one arc vertex will be in the center of the arc. That's at least convenient
for testing, because it makes it easier to predict where horizontal-box line
segments will bump into a polygon's margin or padding boundary.

>
https://codereview.chromium.org/14959014/diff/1/Source/core/rendering/exclusi...
> Source/core/rendering/exclusions/ExclusionPolygon.cpp:509: result =
> ceiledLayoutUnit(firstFitRect.y());
> Are these different on WebKit then? They're the same on Blink. We should
> probably remove one (probably ceiledLayoutUnit, honestly) to be clearer.

The WebKit version avoids the fromFloatCeil computation when subpixel layout
isn't enabled, but I think it's effectively the same:

inline LayoutUnit ceiledLayoutUnit(float value)
{
#if ENABLE(SUBPIXEL_LAYOUT)
    return LayoutUnit::fromFloatCeil(value);
#else
    return ceilf(value);
#endif
}

It looks like ceiledLayoutUnit is used in 3 places aside from my calls.  I'd be
happy to erase all traces of it if you want. Hopefully a separate patch would be
OK?

[Hans Muller, 2013-05-21 22:04:47]

Sorry about the previous reply.  May actual reply  starts towards the middle. 
Cut and pasteO.

[leviw_travelin_and_unemployed, 2013-05-21 22:21:56]

https://codereview.chromium.org/14959014/diff/1/LayoutTests/fast/exclusions/r...
File LayoutTests/fast/exclusions/resources/subpixel-utils.js (right):

https://codereview.chromium.org/14959014/diff/1/LayoutTests/fast/exclusions/r...
LayoutTests/fast/exclusions/resources/subpixel-utils.js:2: var enabled =
undefined;
I understand you're doing this for consistency with the WebKit implementation,
which is fine. I just want to comment that Blink will only support running with
sub-pixel layout enabled.

https://codereview.chromium.org/14959014/diff/1/LayoutTests/fast/exclusions/r...
LayoutTests/fast/exclusions/resources/subpixel-utils.js:22:
ceilSnapToLayoutUnit: function(f) {
I find this name confusing, as "snap" in the context of LayoutUnits is a very
specific action, and one that differs from what you're doing here. You want to
ceil to the nearest layout unit, no? Is there a reason you don't just say
ceilToLayoutUnit?

https://codereview.chromium.org/14959014/diff/1/LayoutTests/fast/exclusions/s...
File
LayoutTests/fast/exclusions/shape-inside/shape-inside-polygon-padding-003.html
(right):

https://codereview.chromium.org/14959014/diff/1/LayoutTests/fast/exclusions/s...
LayoutTests/fast/exclusions/shape-inside/shape-inside-polygon-padding-003.html:74:
var quiet = true; // PASS output depends on SubPixelLayout.isEnabled()
This is a sad change.

https://codereview.chromium.org/14959014/diff/1/Source/core/rendering/exclusi...
File Source/core/rendering/exclusions/ExclusionPolygon.cpp (right):

https://codereview.chromium.org/14959014/diff/1/Source/core/rendering/exclusi...
Source/core/rendering/exclusions/ExclusionPolygon.cpp:509: result =
ceiledLayoutUnit(firstFitRect.y());
Avoid this change

https://codereview.chromium.org/14959014/diff/1/Source/core/rendering/exclusi...
File Source/core/rendering/exclusions/ExclusionRectangle.cpp (right):

https://codereview.chromium.org/14959014/diff/1/Source/core/rendering/exclusi...
Source/core/rendering/exclusions/ExclusionRectangle.cpp:216: result =
ceiledLayoutUnit(bounds.y() + cornerIntercept.y());
Avoid this churn since this makes no change. I'll handle getting rid of
ceiledLayoutUnit in a separate patch.

[Hans Muller, 2013-05-21 22:30:00]

>
https://codereview.chromium.org/14959014/diff/1/LayoutTests/fast/exclusions/r...
> LayoutTests/fast/exclusions/resources/subpixel-utils.js:22:
> ceilSnapToLayoutUnit: function(f) {
> I find this name confusing, as "snap" in the context of LayoutUnits is a very
> specific action, and one that differs from what you're doing here. You want to
> ceil to the nearest layout unit, no? Is there a reason you don't just say
> ceilToLayoutUnit?

You're right, I was getting a little snap happy (ha).  I'll change the
function's name.

>
https://codereview.chromium.org/14959014/diff/1/Source/core/rendering/exclusi...
> Source/core/rendering/exclusions/ExclusionPolygon.cpp:509: result =
> ceiledLayoutUnit(firstFitRect.y());
> Avoid this change

OK.

>
https://codereview.chromium.org/14959014/diff/1/Source/core/rendering/exclusi...
> Source/core/rendering/exclusions/ExclusionRectangle.cpp:216: result =
> ceiledLayoutUnit(bounds.y() + cornerIntercept.y());
> Avoid this churn since this makes no change. I'll handle getting rid of
ceiledLayoutUnit in a separate patch.

OK.

[leviw_travelin_and_unemployed, 2013-05-23 17:56:55]

Thanks. lgtm.

[commit-bot: I haz the power, 2013-05-23 18:00:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hmuller@adobe.com/14959014/14001

[commit-bot: I haz the power, 2013-05-23 19:28:34]

Retried try job too often on mac_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[commit-bot: I haz the power, 2013-05-23 21:37:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hmuller@adobe.com/14959014/14001

[commit-bot: I haz the power, 2013-05-23 23:15:51]

Retried try job too often on mac_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[commit-bot: I haz the power, 2013-05-24 01:00:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hmuller@adobe.com/14959014/14001

[commit-bot: I haz the power, 2013-05-24 02:16:48]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-05-24 15:45:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hmuller@adobe.com/14959014/14001

[commit-bot: I haz the power, 2013-05-24 17:03:23]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-05-24 17:19:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hmuller@adobe.com/14959014/14001

[commit-bot: I haz the power, 2013-05-24 18:18:29]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-05-24 18:23:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hmuller@adobe.com/14959014/14001

[commit-bot: I haz the power, 2013-05-24 22:22:13]

Retried try job too often on win_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[commit-bot: I haz the power, 2013-05-24 22:49:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hmuller@adobe.com/14959014/14001

[commit-bot: I haz the power, 2013-05-25 03:04:38]

Retried try job too often on win_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[commit-bot: I haz the power, 2013-05-28 15:11:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hmuller@adobe.com/14959014/14001

[commit-bot: I haz the power, 2013-05-28 17:00:19]

Retried try job too often on win_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout...

[commit-bot: I haz the power, 2013-05-28 18:42:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hmuller@adobe.com/14959014/14001

[commit-bot: I haz the power, 2013-05-28 19:33:01]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[Hans Muller, 2013-05-28 22:44:48]

Committed patchset #2 manually as r151323 (presubmit successful).