[Webkit-unassigned] [Bug 14858] <col> width ignored when not tied to a single cell
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Jan 24 10:52:36 PST 2010
https://bugs.webkit.org/show_bug.cgi?id=14858
David Kilzer (ddkilzer) <ddkilzer at webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #47295|review? |review-
Flag| |
--- Comment #5 from David Kilzer (ddkilzer) <ddkilzer at webkit.org> 2010-01-24 10:52:36 PST ---
(From update of attachment 47295)
I am not a rendering expert, so I can't review this patch for correctness.
However, I will give it a first-pass review.
Overall, this is a great first patch, Dmitry! Please consider the following
feedback and post another patch for review.
> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog (revision 53776)
> +++ WebCore/ChangeLog (working copy)
> @@ -1,3 +1,15 @@
> +2010-01-24 Dmitry Gorbik <socket.h at gmail.com>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + Fixed width calculation for cells with span when <col> is defined
> + https://bugs.webkit.org/show_bug.cgi?id=14858
> +
> + Test: fast/table/col-width-span-expand.html
> +
> + * rendering/RenderTableCell.cpp:
> + (WebCore::RenderTableCell::styleOrColWidth):
A description of what was changed in the styleOrColWidth() method would be
useful in the ChangeLog. It's not strictly required, but often helps future
bug fixers to understand why the code changes were made and what their intent
was. It can also help reviewers to understand what the bug fixer was thinking
when they wrote the patch.
> Index: WebCore/rendering/RenderTableCell.cpp
> -
> +
Please don't include gratuitous whitespace changes (unless you're removing
whitespace). See <http://webkit.org/coding/coding-style.html> and the
check-webkit-style script.
> 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 i = 1;
The 'i' variable should be declared in the scope of the loop if possible.
> + int iColSpan = colSpan();
A better variable name for this might be "colSpanCount".
> + Length colWidthSum = Length(0, Fixed);
> + while (i <= iColSpan) {
This could be written as a for() loop:
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
Comments that are sentences should end with a period.
> + if (!colWidth.isFixed()) {
> + if (iColSpan > 1)
> + return w;
> + return colWidth;
> + }
> +
> + colWidthSum = Length(colWidthSum.value() + colWidth.value(), Fixed);
> +
> + RenderObject * child = tableCol->nextSibling();
The '*' goes next to the type (RenderObject) in C++ code.
> + // If no next <col> tag found for the span we just return what we have for now
Comment needs a period.
> + if (child && child->isTableCol())
> + tableCol = toRenderTableCol(child);
> + else
> + break;
I think the preferred way to write this would be to use an "early break":
if (!child || !child->isTableCol())
break;
tableCol = toRenderTableCol(child);
> + i++;
This line of code moved into the for() loop.
> + }
>
> // Column widths specified on <col> apply to the border box of the cell.
> // Percentages don't need to be handled since they're always treated this way (even when specified on the cells).
> // See Bugzilla bug 8126 for details.
> - if (w.isFixed() && w.value() > 0)
> - w = Length(max(0, w.value() - borderLeft() - borderRight() - paddingLeft() - paddingRight()), Fixed);
> + if (colWidthSum.isFixed() && colWidthSum.value() > 0)
> + colWidthSum = Length(max(0, colWidthSum.value() - borderLeft() - borderRight() - paddingLeft() - paddingRight()), Fixed);
> + return colWidthSum;
> }
> +
> return w;
> }
> +
No need for an extra blank line after the method here.
> Index: LayoutTests/ChangeLog
> ===================================================================
> --- LayoutTests/ChangeLog (revision 53776)
> +++ LayoutTests/ChangeLog (working copy)
> @@ -1,3 +1,12 @@
> +2010-01-24 Dmitry Gorbik <socket.h at gmail.com>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + Fixed width calculation for cells with span when <col> is defined
> + https://bugs.webkit.org/show_bug.cgi?id=14858
> +
> + * fast/table/col-width-span-expand.html: Added.
It's great that you included at test case with this change, but the patch is
missing the expected results: both "text" results (*-expected.txt) and pixel
test results (*-expected.png, *-expected.checksum) since this is not a
"text-only" test case. Using "run-webkit-tests" to generate expected text
results (which should be a render tree dump), and add "--pixel" to generate
pixel test results.
> Index: LayoutTests/fast/table/col-width-span-expand.html
> ===================================================================
> --- LayoutTests/fast/table/col-width-span-expand.html (revision 0)
> +++ LayoutTests/fast/table/col-width-span-expand.html (revision 0)
> @@ -0,0 +1,40 @@
> +<!DOCTYPE html>
> +<html>
> + <head>
> + <title>WebKit Bug 14858: col width ignored when not tied to a single cell</title>
> + <link href="http://www.w3.org/TR/REC-CSS2/tables.html#q4" rel="help" />
> + <style type="text/css">
> +div, table {
> + left: 1em;
> + position: absolute;
> + top: 1em
> +}
> +div {
> + color: red;
> + left: 250px
> +}
> +table {border-spacing: 0}
> +td {
> + background-color: white;
> + padding: 0
> +}
> + </style>
> + </head>
> + <body>
> + <div>FAIL</div>
> + <table>
> + <col width="20" />
> + <col width="40" />
> + <col width="80" />
> + <col width="160" />
> + <tbody>
> + <tr>
> + <td> </td>
> + <td colspan="2"> </td>
> + <td> </td>
> + </tr>
> + </tbody>
> + </table>
> + </body>
> +</html>
This test from the bug is good, but could be improved by:
- Adding HTML to add a link to the bugs.webkit.org bug.
- Adding text to describe how the test passes (since passing means that you
can't see the red "FAIL" text any more, but no green "PASS" text appears).
- Using the CSS style definitions from
LayoutTests/fast/js/resources/js-test-style.css is usually preferred for
consistency, but not required.
- Instead of the last two items, switch the test to a text-only test case (see
other tests that use "layoutTestController.dumpAsText()" and which include
LayoutTests/fast/js/resources/js-test-pre.js and js-test-post.js) by testing
the computed style width of the actual <td> elements (which should be 50, 250,
and 200, I think). This is preferred because pixel tests can differ between
platforms, pixel tests are slower, and they take up more room on disk. The
js-test-* files also give you nice status output without you having to do much
besides providing a "console" for them to use.
Also note that the "bot status" on bugs.webkit.org only checks that the patch
builds, not that all of the layout tests pass. You need to run the layout
tests with your patch applied to make sure it doesn't cause any regressions:
./WebKitTools/Scripts/run-webkit-tests
Current layout test status for trunk: <http://build.webkit.org/>
r- to address the above issues. Thanks for working on this patch!
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list