Make the common table-layout cases a little faster with inlining
Created attachment 167400 [details] Patch
Comment on attachment 167400 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167400&action=review I think the change makes sense from a performance perspective. It also cleans up this code a bit which is nice. r=me with comments. > Source/WebCore/ChangeLog:3 > + Make the common table-layout cases a little faster with inlining It's probably the no-column case that you are talking about here. > Source/WebCore/rendering/RenderTable.cpp:-780 > - Nit: I like those extra spaces before returns as it helps see the different blocks so I would rather keep this one. > Source/WebCore/rendering/RenderTable.h:276 > + RenderTableCol* slowColElement(unsigned col, bool* startEdge = 0, bool* endEdge = 0) const; Not default arguments as you only call it from colElement with all the arguments. > Source/WebCore/rendering/RenderTableCell.cpp:153 > + Length colWidthSum = Length(0, Fixed); While touching this code, you may just end up making colWidthSum an int and explicitly use Length::intValue() as we only ever add fixed length to it. This would save on the Length creation and copying. > Source/WebCore/rendering/RenderTableCell.cpp:176 > + if (colWidthSum.isFixed() && colWidthSum.value() > 0) AFAICT the isFixed() check will always be true as we explicitly make colWidthSum a Fixed Length. > Source/WebCore/rendering/RenderTableCell.h:248 > + Length logicalWidthFromTableColumn(RenderTableCol* firstColForThisCell, Length widthFromStyle) const; I feel that the name could be improved a bit. You find your logical widths from your columns so it should probably be logicalWidthFromColumns (I would drop the 'Table' part but it's a matter of preference).
Comment on attachment 167400 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=167400&action=review >> Source/WebCore/rendering/RenderTable.cpp:-780 >> - > > Nit: I like those extra spaces before returns as it helps see the different blocks so I would rather keep this one. Fixed. >> Source/WebCore/rendering/RenderTable.h:276 >> + RenderTableCol* slowColElement(unsigned col, bool* startEdge = 0, bool* endEdge = 0) const; > > Not default arguments as you only call it from colElement with all the arguments. Fixed. >> Source/WebCore/rendering/RenderTableCell.cpp:153 >> + Length colWidthSum = Length(0, Fixed); > > While touching this code, you may just end up making colWidthSum an int and explicitly use Length::intValue() as we only ever add fixed length to it. This would save on the Length creation and copying. Great suggestion. It made obvious that we're not applying border/padding in teh negative-width case. I added a FIXME. >> Source/WebCore/rendering/RenderTableCell.cpp:176 >> + if (colWidthSum.isFixed() && colWidthSum.value() > 0) > > AFAICT the isFixed() check will always be true as we explicitly make colWidthSum a Fixed Length. Yeah, and the > 0 doesn't make much sense, nor does the max? Unless it's specified that border/padding can never make width negative? >> Source/WebCore/rendering/RenderTableCell.h:248 >> + Length logicalWidthFromTableColumn(RenderTableCol* firstColForThisCell, Length widthFromStyle) const; > > I feel that the name could be improved a bit. You find your logical widths from your columns so it should probably be logicalWidthFromColumns (I would drop the 'Table' part but it's a matter of preference). Done.
Created attachment 167631 [details] Patch for landing
Comment on attachment 167631 [details] Patch for landing Clearing flags on attachment: 167631 Committed r130698: <http://trac.webkit.org/changeset/130698>
All reviewed patches have been landed. Closing bug.