[webkit-reviews] review granted: [Bug 98566] Make the common table-layout cases a little faster with inlining : [Attachment 167400] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Eric Seidel
<eric at webkit.org>'s request for review:
Bug 98566: Make the common table-layout cases a little faster with inlining
https://bugs.webkit.org/show_bug.cgi?id=98566

Attachment 167400: Patch
https://bugs.webkit.org/attachment.cgi?id=167400&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
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).