Summary: | Split the extra logical height distribution logic out of RenderTableSection::layoutRows | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Julien Chaffraix <jchaffraix> | ||||||
Component: | Tables | Assignee: | 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
Julien Chaffraix
2012-03-08 19:37:49 PST
Created attachment 130956 [details]
Proposed refactoring.
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 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. Created attachment 131069 [details]
Same change, nits removed.
Comment on attachment 131069 [details]
Same change, nits removed.
Looks good to me!
Comment on attachment 131069 [details] Same change, nits removed. Clearing flags on attachment: 131069 Committed r110336: <http://trac.webkit.org/changeset/110336> All reviewed patches have been landed. Closing bug. |