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

Sun Mar 21 12:44:36 PDT 2010

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has granted  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>
(In reply to comment #22)
> (From update of attachment 49318 [details])
> 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.)

I reviewed the new logic again per Comment #23, and I agree it preserves the
previous behavior, so this is no longer an issue.

> 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=me!  I'll clean up the nits above and land this patch.