[webkit-reviews] review denied: [Bug 78027] CSS 2.1 failure: fixed-table-layout-013 and fixed-table-layout-015 fail : [Attachment 126147] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has denied Robert Hogan
<robert at webkit.org>'s request for review:
Bug 78027: CSS 2.1 failure: fixed-table-layout-013 and fixed-table-layout-015
fail
https://bugs.webkit.org/show_bug.cgi?id=78027

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

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=126147&action=review


r- for the missing |else| as I would like to see the new patch before landing.

> Source/WebCore/rendering/FixedTableLayout.cpp:80
> +static RenderObject* nextCol(RenderObject* child)

Please use a verb in the function name: findNextCol for example is a good name.


As discussed on IRC, this function is equivalent to the previous code block and
is more readable. I don't think this area of the code is performance sensitive
so the change should be fine.

> Source/WebCore/rendering/FixedTableLayout.cpp:105
> +    for (;child && child->isTableCol(); child = nextCol(child)) {

I think you found a bug in our style checker (filed bug 78238) :-)

There should be a space after the first semi-colon. Also please just move the
initialization of |child| in the loop.

> Source/WebCore/rendering/FixedTableLayout.cpp:109
> +	   if (col->isTableColGroup())

I don't like using isTableColGroup() here as it sounds like one of the
RenderObject::isTable*() functions which are virtual. I would like to see it
renamed to isColGroup(), renderColIsColGroup() or equivalent.

> Source/WebCore/rendering/FixedTableLayout.cpp:130
> -		   } else {
> -		       if (span <
m_table->spanOfEffCol(currentEffectiveColumn)) {
> -			   m_table->splitColumn(currentEffectiveColumn, span);
> -			   nEffCols++;
> -			   m_width.append(Length());
> -		       }
> -		       spanInCurrentEffectiveColumn =
m_table->spanOfEffCol(currentEffectiveColumn);
>		   }

This branch has disappeared in your change. It is possible it is unneeded but
this is a separate change.

> Source/WebCore/rendering/RenderTableCol.h:46
> +    bool isTableColGroup() { return firstChild() ? true : false; }

return firstChild(); is enough here.