Bug 80671 – Split the extra logical height distribution logic out of RenderTableSection::layoutRows

• Component: Tables
• Status: RESOLVED FIXED

[Julien Chaffraix]

Currently it's this logic is inlined in the function which makes it longer than it should be.

Also the naming doesn't help understanding what is going on in the function at the moment.

Patch forthcoming.

[Julien Chaffraix]

Created attachment 130956 [details]
Proposed refactoring.

[Adrienne Walker]

Comment on attachment 130956 [details]
Proposed refactoring.

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

In terms of correctness, this looks fine to me.

> Source/WebCore/rendering/RenderTableSection.cpp:475
> +    if (extraLogicalHeight > 0 && m_rowPos[totalRows]) {

nit: maybe make this an early out for extra clarity.

> Source/WebCore/rendering/RenderTableSection.cpp:479
> +        int tot = m_rowPos[totalRows];
> +        int add = 0;
> +        int prev = m_rowPos[0];

style nit: I realize you're just moving this code, but don't use abbreviations like "tot" and "prev".

> Source/WebCore/rendering/RenderTableSection.cpp:499
> +    if (m_rowPos[totalRows] || !nextSibling()) {

nit: maybe an early return here too.

> Source/WebCore/rendering/RenderTableSection.cpp:511
> +        remainingExtraLogicalHeight = distributeExtraLogicalHeightToPercentRows(remainingExtraLogicalHeight, totalPercent);
> +        remainingExtraLogicalHeight = distributeExtraLogicalHeightToAutoRows(remainingExtraLogicalHeight, autoRowsCount);

The previous code had checks for totalPercent == 0 and autoRowsCount == 0, which avoided a needless iteration through all of the rows.  Is that worth keeping?

[Julien Chaffraix]

Comment on attachment 130956 [details]
Proposed refactoring.

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

>> Source/WebCore/rendering/RenderTableSection.cpp:511
>> +        remainingExtraLogicalHeight = distributeExtraLogicalHeightToAutoRows(remainingExtraLogicalHeight, autoRowsCount);
> 
> The previous code had checks for totalPercent == 0 and autoRowsCount == 0, which avoided a needless iteration through all of the rows.  Is that worth keeping?

Yes, autoRowsCount == 0 is handled inside distributeExtraLogicalHeightToAutoRows, however I forgot to keep the totalPercent check while cleaning the code. As a side note, a table usually doesn't have a lot of sections AFAICT but it's better not to slow down in case it does.

[Julien Chaffraix]

Created attachment 131069 [details]
Same change, nits removed.

[Adrienne Walker]

Comment on attachment 131069 [details]
Same change, nits removed.

Looks good to me!

[WebKit Review Bot]

Comment on attachment 131069 [details]
Same change, nits removed.

Clearing flags on attachment: 131069

Committed r110336: <http://trac.webkit.org/changeset/110336>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.