Bug 88813 – Change RenderTableSection::calcRowLogicalHeight to round rather than floor height

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

[Emil A Eklund]

Change RenderTableSection::calcRowLogicalHeight to round the logicalHeight instead of flooring it. This matches our rounding logic elsewhere and results in table rows better matching the expected height.

[Emil A Eklund]

Created attachment 146932 [details]
Patch

[Julien Chaffraix]

Comment on attachment 146932 [details]
Patch

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

> LayoutTests/fast/sub-pixel/table-rows-have-stable-height.html:65
> +                    var height = r((20 + i) * 0.93 + i);

Where did you find this formula? The whole test cases is based on this being right, yet it's super cryptic.

[Emil A Eklund]

(In reply to comment #2)
> (From update of attachment 146932 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146932&action=review
> 
> > LayoutTests/fast/sub-pixel/table-rows-have-stable-height.html:65
> > +                    var height = r((20 + i) * 0.93 + i);
> 
> Where did you find this formula? The whole test cases is based on this being right, yet it's super cryptic.

The formula isn't critical at all, it is just a way to have each row be a different height with subpixel precision.

[Emil A Eklund]

Expanding a bit. The purpose of the test is to check whether the reported height of a row matches the actual height and that it isn't depending on the location of the row. The way it does that is by setting the size of a row in an off-screen table to a subpixel value, measure the result resulting height and then use that height to size a row in the on screen table. This is explained in the test. I'd be happy to expand on it you feel it is still unclear.

[Emil A Eklund]

Julien, any chance you could take another look at this today?

[Julien Chaffraix]

Comment on attachment 146932 [details]
Patch

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

> LayoutTests/ChangeLog:8
> +        Add new test ensuring that rows are painted with the desired height when

It's not really 'painted' more 'laid out'.

> LayoutTests/fast/sub-pixel/table-rows-have-stable-height.html:45
> +        </p>

Nit: I like to have the bug number in the output.

>>> LayoutTests/fast/sub-pixel/table-rows-have-stable-height.html:65
>>> +                    var height = r((20 + i) * 0.93 + i);
>> 
>> Where did you find this formula? The whole test cases is based on this being right, yet it's super cryptic.
> 
> The formula isn't critical at all, it is just a way to have each row be a different height with subpixel precision.

I think a comment on why we are doing that would be welcome. Something like:

// Set the size to a non-pixel bound value. The exact formula isn't important, what matters is that each rows has a different size.

[Emil A Eklund]

Thanks for the review Julien, I'll make the changes you suggested!

[Emil A Eklund]

Created attachment 147601 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 147601 [details]
Patch for landing

Clearing flags on attachment: 147601

Committed r120354: <http://trac.webkit.org/changeset/120354>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.