Bug 94027 – r125591 broke tests with SUBPIXEL_LAYOUT disabled

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Levi Weintraub]

Thanks to abarth for bringing this to my attention.  We incorrectly started ceiling instead of truncating some values when using integer layout. The fix is easy but unfortunate... I'll upload momentarily.

[Levi Weintraub]

Created attachment 158432 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 158432 [details]
Patch

This doesn't make sense to me.  Why do we truncate in one case and ceil in teh other?

[Levi Weintraub]

(In reply to comment #2)
> (From update of attachment 158432 [details])
> This doesn't make sense to me.  Why do we truncate in one case and ceil in teh other?

Because we're actually ceiling to the next LayoutUnit when sub-pixel layout is enabled, which ensures that we have enough room for margins for example when actually laying out the lines in the block. When we don't have sub-pixel layout enabled, we truncate in both cases.

[Eric Seidel (no email)]

That explanation still doesn't make sense to me.  With layout units, we have to be careful when to ciel vs. when to floor.  Why is the old code, which always operates on integers correct to floor here?

[Levi Weintraub]

(In reply to comment #4)
> That explanation still doesn't make sense to me.  With layout units, we have to be careful when to ciel vs. when to floor.  Why is the old code, which always operates on integers correct to floor here?

It comes down to safely mirroring what we do at layout time. When we layout with sub-pixel on, we need to make sure we've set preferred widths that contain the error of the content, hence the ceiling. When we layout without sub-pixel, we truncate. Hence, we have to truncate here as well.

[Eric Seidel (no email)]

So what you're saying is that in the non-subpixel case, the rule is to *always* truncate, everywhere.  Is that correct?  But with subpixel enabled, things are slightly more tricky.

If so, we should update the comment to explain why "adjust" ciels in one case and truncates in the other.  Similarly, since subpixel has to be careful when to ciel vs. truncate, we should probably explain when it's OK to use that "adjust" method.

[Levi Weintraub]

(In reply to comment #6)
> So what you're saying is that in the non-subpixel case, the rule is to *always* truncate, everywhere.  Is that correct?  But with subpixel enabled, things are slightly more tricky.
> 
> If so, we should update the comment to explain why "adjust" ciels in one case and truncates in the other.  Similarly, since subpixel has to be careful when to ciel vs. truncate, we should probably explain when it's OK to use that "adjust" method.

Naming is hard. It's very duplicitous behavior, given it's flooring in one case, and finding a LayoutUnit that's at least as big as the float in another. I'd love a suggestion, if you have one, for how to name this thing.

We don't always truncate everywhere. We ceil the width of lines to contain text for blocks (see updatePreferredWidth), but not for Lengths, even when those lengths are used in the line box tree.

[Levi Weintraub]

Created attachment 158460 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 158460 [details]
Patch

OK.

[WebKit Review Bot]

Comment on attachment 158460 [details]
Patch

Clearing flags on attachment: 158460

Committed r125632: <http://trac.webkit.org/changeset/125632>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.