[webkit-reviews] review denied: [Bug 14858] <col> width ignored when not tied to a single cell : [Attachment 49318] proposed fix v2.5

Fri Mar 19 18:18:54 PDT 2010

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied Dmitry Gorbik
<socket.h at gmail.com>'s request for review:
Bug 14858: <col> width ignored when not tied to a single cell
https://bugs.webkit.org/show_bug.cgi?id=14858

Attachment 49318: proposed fix v2.5
https://bugs.webkit.org/attachment.cgi?id=49318&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
This looks great!  Just a couple nits below:

> Index: WebCore/ChangeLog
> +	   Fixed width calculation for cells with span when <col> is defined

You need to include a link to the bug in the ChangeLog entry.

> +	   * rendering/RenderTableCell.cpp:
> +	   (WebCore::RenderTableCell::styleOrColWidth): added the calculation
of cell width
> +	   in case of <col> defined and span>1.

Nit:  Change "span>1" to "span > 1".

> Index: WebCore/rendering/RenderTable.cpp
> +    RenderTableCol* colElem = toRenderTableCol(child);
> +    while (colElem) {
> +	   int span = colElem->span();
> +	   if (!colElem->firstChild()) {
> +	       int startCol = cCol;
> +	       int endCol = cCol + span - 1;
> +	       cCol += span;
> +	       if (cCol > col) {
> +		   if (startEdge)
> +		       *startEdge = startCol == col;
> +		   if (endEdge)
> +		       *endEdge = endCol == col;
> +		   return colElem;
> +	       }
> +	   }
> +	   colElem = nextColElement(colElem);
> +	   if (!colElem)
>	       break;
>      }

You can actually leave off the final "if (!colElem) break;" statements in the
while loop since the next iteration through the loop will catch this condition
and then return 0.

> Index: WebCore/rendering/RenderTableCell.cpp
> ===================================================================
> --- WebCore/rendering/RenderTableCell.cpp	(revision 53844)
> +++ WebCore/rendering/RenderTableCell.cpp	(working copy)
> @@ -83,18 +83,42 @@ void RenderTableCell::updateFromElement(
>  Length RenderTableCell::styleOrColWidth() const
>  {
>      Length w = style()->width();
> -    if (colSpan() > 1 || !w.isAuto())
> +    if (!w.isAuto())
>	   return w;
> +
>      RenderTableCol* tableCol = table()->colElement(col());
> +
>      if (tableCol) {
> -	   w = tableCol->style()->width();
> -	   
> +	   int colSpanCount = colSpan();
> +
> +	   Length colWidthSum = Length(0, Fixed);
> +	   for (int i = 1; i <= colSpanCount; i++) {
> +	       Length colWidth = tableCol->style()->width();
> +
> +	       // Percentage value should be returned only for colSpan=1.
> +	       // Otherwise we return original width for the cell.

Why is this true?  Is this part of the standard?  Is this covered by an
existing test case?  (If not, you need to add a test case for this condition as
well.)

Nit:  "colSpan=1" should be "colSpan == 1" to be clearer that it's an equality
check.

> +	       if (!colWidth.isFixed()) {
> +		   if (colSpanCount > 1)
> +		       return w;
> +		   return colWidth;
> +	       }
> Index: LayoutTests/ChangeLog
> +2010-01-27  Dmitry Gorbik  <socket.h at gmail.com>
> +
> +	   Reviewed by NOBODY (OOPS!).
> +
> +	   Fixed width calculation for cells with span when <col> is defined

You need to include a link to the bug in the ChangeLog entry.

> Index: LayoutTests/fast/table/col-width-span-expand.html
> +	<head>

There is a tab on this line that should be 8 spaces.

r- to answer the question about the new code in
RenderTableCell::styleOrColWidth().

Everything else looks good.  An explanation for the code in
RenderTableCell::styleOrColWidth() and/or a layout test that covers that case
will get an r+.

Thanks!