Bug 88380 – Revert verticalPositionForBox to int

• Component: Layout and Rendering
• Status: RESOLVED INVALID

[Emil A Eklund]

Revert verticalPositionForBox to integers to ensure consistent alignment. Currently the vertical position for boxes inside a line is computed as a LayoutUnit and then pixel snapped at rendering time. This can cause the alignment on subsequent lines to alter. By always rounding (flooring) the values the same way we ensure consistent alignment between lines.

[Emil A Eklund]

Created attachment 145902 [details]
Patch

[Julien Chaffraix]

Comment on attachment 145902 [details]
Patch

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

The change looks good, I am concerned about the image.

> Source/WebCore/rendering/RootInlineBox.cpp:873
> +            verticalPosition += -static_cast<int>(fontMetrics.xHeight() / 2) - 
> +                floorToInt(renderer->lineHeight(firstLine, lineDirection)) / 2 +
> +                floorToInt(renderer->baselinePosition(baselineType(), firstLine, lineDirection));

Though there is no explicit preferred breaking rule for arithmetic operators, I would try to be consistent with the rule for logical expressions and put the sign at the beginning of the line.

> LayoutTests/fast/sub-pixel/consistent-alignment-across-lines-expected.html:12
> +        font-family: webkit-ahem;

Is this needed for the test to pass?

* If not, I would rather have this line removed as Ahem makes the output unreadable.
* If yes, just use font-family: Ahem; and remove the @font-face above as DRT should know where to fetch Ahem already.

> LayoutTests/fast/sub-pixel/consistent-alignment-across-lines-expected.html:18
> +       background-image: url(data:image/gif;base64,R0lGODlhEgANAOMKAAAAABUVFRoaGisrKzk5OUxMTGRkZLS0tM/Pz9/f3////////////////////////yH5BAEKAA8ALAAAAAASAA0AAART8Ml5Arg3nMkluQIhXMRUYNiwSceAnYAwAkOCGISBJC4mSKMDwpJBHFC/h+xhQAEMSuSo9EFRnSCmEzrDComAgBGbsuF0PHJq9WipnYJB9/UmFyIAOw==);
> +       background-repeat: no-repeat;

My understanding of copyright is that including this image creates a derivative work which means you either have the right to submit the image under license WebKit accepts or we cannot land such a patch. I am not a lawyer though so don't think this is a definite answer.

Please replace this image by a proper background-color if you need to color .class (or an existing image) as this simplifies the above legal implication.

> LayoutTests/fast/sub-pixel/consistent-alignment-across-lines-expected.html:30
> +    <div>

Could we have a short description of what you test? I would also recommend adding a link to this bug.

[Emil A Eklund]

Thanks for the review Julien.

The image is from a chromium so it should be fine. I'll remove it anyway though as it isn't needed.

It's actually really hard to come up with a good test for this, the one in the patch fails on certain platforms. I'll rethink it and upload a new one shortly.