[webkit-reviews] review denied: [Bug 104711] Col width is not honored when dynamically updated and it would make table narrower : [Attachment 221227] Patch

[bugzilla-daemon at webkit.org]

Antti Koivisto <koivisto at iki.fi> has denied gur.trio at gmail.com's request for
review:
Bug 104711: Col width is not honored when dynamically updated and it would make
table narrower
https://bugs.webkit.org/show_bug.cgi?id=104711

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

------- Additional Comments from Antti Koivisto <koivisto at iki.fi>
View in context: https://bugs.webkit.org/attachment.cgi?id=221227&action=review


> Source/WebCore/rendering/RenderTableCol.cpp:58
> +	       unsigned nEffCols = table->numEffCols();
> +	       for (unsigned j = 0; j < nEffCols; j++) {

Ideally this would be optimized by figuring out which columns this
RenderTableCol actually affects. This may not be critical to do now as
animating <col> width is likely rare.

> Source/WebCore/rendering/RenderTableCol.cpp:62
> +		   for (RenderObject* child = table->firstChild(); child; child
= child->nextSibling()) {
> +		       if (child->isTableSection()) {
> +			   RenderTableSection* section =
toRenderTableSection(child);
> +			   unsigned numRows = section->numRows();

You should use C++11 and child iterators here:

for (auto& section : childrenOfType<RenderTableSection>(*table)) { 
    unsigned rowCount = section.numRows()
    ....

> Source/WebCore/rendering/RenderTableCol.cpp:66
> +			       RenderTableSection::CellStruct current =
section->cellAt(i, j);
> +			       RenderTableCell* cell = current.primaryCell();
> +			       cell->setPreferredLogicalWidthsDirty(true);

You can just use section.primaryCellAt(i, j)

Are you sure the cell can't be null? Looking around it is always null tested in
similar cases.