Bug 94027 - r125591 broke tests with SUBPIXEL_LAYOUT disabled
Summary: r125591 broke tests with SUBPIXEL_LAYOUT disabled
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Levi Weintraub
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-14 15:50 PDT by Levi Weintraub
Modified: 2012-08-14 18:39 PDT (History)
3 users (show)

See Also:


Attachments
Patch (3.04 KB, patch)
2012-08-14 16:06 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (3.98 KB, patch)
2012-08-14 17:47 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Levi Weintraub 2012-08-14 15:50:27 PDT
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.
Comment 1 Levi Weintraub 2012-08-14 16:06:04 PDT
Created attachment 158432 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-08-14 16:11:59 PDT
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?
Comment 3 Levi Weintraub 2012-08-14 16:15:35 PDT
(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.
Comment 4 Eric Seidel (no email) 2012-08-14 16:20:16 PDT
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?
Comment 5 Levi Weintraub 2012-08-14 16:27:39 PDT
(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.
Comment 6 Eric Seidel (no email) 2012-08-14 16:30:24 PDT
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.
Comment 7 Levi Weintraub 2012-08-14 16:38:53 PDT
(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.
Comment 8 Levi Weintraub 2012-08-14 17:47:58 PDT
Created attachment 158460 [details]
Patch
Comment 9 Eric Seidel (no email) 2012-08-14 17:49:05 PDT
Comment on attachment 158460 [details]
Patch

OK.
Comment 10 WebKit Review Bot 2012-08-14 18:39:36 PDT
Comment on attachment 158460 [details]
Patch

Clearing flags on attachment: 158460

Committed r125632: <http://trac.webkit.org/changeset/125632>
Comment 11 WebKit Review Bot 2012-08-14 18:39:40 PDT
All reviewed patches have been landed.  Closing bug.