Bug 80671

Summary: Split the extra logical height distribution logic out of RenderTableSection::layoutRows
Product: WebKit Reporter: Julien Chaffraix <jchaffraix>
Component: TablesAssignee: Julien Chaffraix <jchaffraix>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, rniwa, robert, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Proposed refactoring.
none
Same change, nits removed. none

Description Julien Chaffraix 2012-03-08 19:37:49 PST 
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.
Comment 1 Julien Chaffraix 2012-03-08 19:43:53 PST 
Created attachment 130956 [details]
Proposed refactoring.
Comment 2 Adrienne Walker 2012-03-09 11:34:05 PST 
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?
Comment 3 Julien Chaffraix 2012-03-09 11:49:21 PST 
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.
Comment 4 Julien Chaffraix 2012-03-09 12:09:18 PST 
Created attachment 131069 [details]
Same change, nits removed.
Comment 5 Adrienne Walker 2012-03-09 12:10:54 PST 
Comment on attachment 131069 [details]
Same change, nits removed.

Looks good to me!
Comment 6 WebKit Review Bot 2012-03-09 14:58:22 PST 
Comment on attachment 131069 [details]
Same change, nits removed.

Clearing flags on attachment: 131069

Committed r110336: <http://trac.webkit.org/changeset/110336>
Comment 7 WebKit Review Bot 2012-03-09 14:58:26 PST 
All reviewed patches have been landed.  Closing bug.