[Webkit-unassigned] [Bug 13598] <td> tag can cause Webkit to generate empty space

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat Sep 8 11:26:25 PDT 2012


--- Comment #18 from Julien Chaffraix <jchaffraix at webkit.org>  2012-09-08 11:26:41 PST ---
(From update of attachment 162956)
View in context: https://bugs.webkit.org/attachment.cgi?id=162956&action=review

> Source/WebCore/ChangeLog:10
> +        For tables with colspan, the algorithm for distribution of width between
> +        the spanned columns is improper in case spanned cells with percent widths are
> +        specified.

This is a pattern I have seen in your ChangeLogs: I can't figure out what is wrong just by reading them. "improper" is *not* a useful word as it could mean *anything*. Please use technically relevant details and explain the 'why' as much as you can. I would have given a try at this description but there is too many holes for me to fill at the moment.

> Source/WebCore/ChangeLog:21
> +        After allocating the extra width over the columns with fixed width, the remaining
> +        extra width should then be distributed amongst percent width columns (if any) before
> +        prioritizing the auto or relative width columns.

I don't think I agree with your algorithm for spreading the extra logical width here. It would make way more sense to do the prioritization differently: 'auto' width first as they have no constrain on them, then 'percent' to try to keep the requested ratio and finally fixed widths ('fixed', viewport relative, calc). How does other browsers handle that?

> Source/WebCore/ChangeLog:24
> +        For this, we need to first take the minimum logical width consumed by the auto or relative
> +        width columns if any, and the extra width after taking out this value should be

relative width has a meaning in CSS that is likely not the one you are thinking of. 'em', 'ex' are relative units as they depends on your font.

> Source/WebCore/rendering/AutoTableLayout.cpp:420
> +                int autoMinLogicalWidth = 0;
> +                int widthForPercentAllocation = 0;

WebKit style is to move variable definitions to where they are needed.

> Source/WebCore/rendering/AutoTableLayout.cpp:422
>                  // Give min to variable first, to fixed second, and to others third.

This comment should be updated to outline the new spreading algorithm.

> Source/WebCore/rendering/AutoTableLayout.cpp:434
> +                // Take into account the minimum width for auto cells.

This comment is a 'what' comment and not super useful. On top of that, it's wrong as you recognized yourself as you bundle other non-handled length type together under the 'auto' umbrella.

> Source/WebCore/rendering/AutoTableLayout.cpp:437
> +                        autoMinLogicalWidth += m_layoutStruct[pos].effectiveMinLogicalWidth;

> For that I wish to get the cumulative of the remaining columns i.e. of auto and relative type (or other types, if any).

I don't think it's correct to say 'or other types, if any' without thinking about those cases. Using explicitly checking for what you want is more correct and future proof. Again maybe it's right but the absence of good justification makes it hard to see.

> Source/WebCore/rendering/AutoTableLayout.cpp:440
> +                // Next, allocate for the percent cells.

Another mostly useless 'what' comment. We prefer 'why' comments.

> LayoutTests/ChangeLog:17
> +        The existing test case result needs modification as it is affected by the change.

Another example of a useless sentence. How can anybody evaluate if your rebaselining is correct? Helper the maintainers (and the reviewers) by stating what is the difference and why it is correct.

Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.

More information about the webkit-unassigned mailing list