Don't round x-axis for text runs when subpixel font scaling is enabled

Don't round x-axis for text runs when subpixel font scaling is enabled

When subpixel font scaling is enabled text runs are positioned at
subpixel boundaries on the x-axis and thus there is no reason to snap
the x value.
We still round the y-axis to ensure consistent line heights.

BUG=164167
R=leviw@chromium.org, mstensho@opera.com

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

[leviw_travelin_and_unemployed, 2014-05-27 21:44:18]

lgtm

[eae, 2014-05-27 22:58:10]

The CQ bit was checked by eae@chromium.org

[commit-bot: I haz the power, 2014-05-27 22:58:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/eae@chromium.org/300623007/20001

[eae, 2014-05-27 22:58:39]

The CQ bit was unchecked by eae@chromium.org

[eae, 2014-05-27 23:31:48]

Hmm, this breaks two multicol ref tests on mac where the reference (but not the
actual results) are cut off. Not sure if this change fixed the implementation or
broke the reference. Unmarking for CQ while investigating.

[mstensho (USE GERRIT), 2014-05-28 13:51:17]

On 2014/05/27 23:31:48, eae wrote:
> Hmm, this breaks two multicol ref tests on mac where the reference (but not
the
> actual results) are cut off. Not sure if this change fixed the implementation
or
> broke the reference. Unmarking for CQ while investigating.

I assume you're talking about
fast/multicol/newmulticol/column-rules-fixed-height.html and
fast/multicol/newmulticol/layers-in-multicol.html (I see additional similar
multicol failures in the results too, although those are not reftests)? These
two tests compare rendering between the old/current and the new multicol
implementation. The -expected.html files use the old implementation, so the
patch causes trouble for the old/current implementation. The new multicol
implementation implements clipping more correctly, so it won't be much of a
problem there. The old implementation is indeed broken when it comes to
clipping, since it clips horizontally at the column edges rather than in the
middle of the column gaps, and this seems to be what's triggering the problem.
If the column width isn't a whole number, and its clip rectangle X is rounded up
to the nearest whole number, you lose some millipixels at the beginning.

Could it be this line:

context->clip(pixelSnappedIntRect(clipRect));

in RenderBlock::paintColumnContents() that causes trouble? Note that this code
has an evil sister in RenderLayer::paintChildLayerIntoColumns():

context->clip(pixelSnappedIntRect(colRect));

which is involved in the old implementation when a multicol container has
descendants that establish layers.

Use enclosingIntRect() instead?

Could it be that the font engine on Mac rounds down in this case, while the
others round to the nearest whole number? Looks like the font used when running
layout tests is unwilling to become anti-aliased, at least. I'm on Linux, BTW.

However, I really wonder if this is multicol-specific at all.
pixelSnappedIntRect() is used quite a lot in the code, e.g. in
RenderLayer::clipToRect(), which seems to be why the first letter on each line
in the following TC is clipped differently, depending on which direction
subpixels are rounded. Sure, there may be negative bearings to take into
consideration too, but this looks suspicious:

<style>div { overflow:scroll; height:5em; background:yellow; }</style>
<div style="margin-left:0.5px;">jj<br>bb<br>gg<br><br><br>x</div>
<div style="margin-left:0.45px;">jj<br>bb<br>gg<br><br><br>x</div>
<div style="margin-left:0px;">jj<br>bb<br>gg<br><br><br>x</div>

Would be interesting to see how LayoutTest on Mac renders this one.

[eae, 2014-05-28 16:21:54]

Great analysis Morten and spot on! Thank you! Saved me a couple of hours of
debugging time. Yay.

[eae, 2014-05-28 16:34:31]

PTAL

[mstensho (USE GERRIT), 2014-05-28 16:46:32]

non-owner lgtm on the old multicol parts, but what about the TC without multicol
in my previous comment? Worry about that in a separate CL?

[eae, 2014-05-28 16:54:51]

On 2014/05/28 16:46:32, Morten Stenshorne wrote:
> what about the TC without multicol in my previous comment?
> Worry about that in a separate CL?

While not ideal having the rounding effect clipping/alignment is hard to avoid.
One way to deal with this would be to align to device pixel boundaries (a
concept which we do not yet have in blink). However even that would not work in
all cases.

Take the following example:
<div>Abcdefg</div>
<div><span>Abcd</span><span>efg</span></div>

If we align the start of each run to the css or device pixel boundary we'd
render the two lines differently as the e on the second line would have a
different alignment than the e on the first list.

[leviw_travelin_and_unemployed, 2014-05-28 17:04:23]

On 2014/05/28 16:46:32, Morten Stenshorne wrote:
> non-owner lgtm on the old multicol parts, but what about the TC without
multicol
> in my previous comment? Worry about that in a separate CL?

I think Morten is spot on here. Let's put the multi-column part in a separate CL
and land it first.

[eae, 2014-05-28 17:18:31]

Split multicol changes into separate CL,
https://codereview.chromium.org/300843010/

[eae, 2014-05-28 20:34:17]

Committed patchset #4 manually as r174992 (presubmit successful).