Bug 78027

Created attachment 125920 [details] Patch

Comment on attachment 125920 [details] Patch Can we just break this block out into a helper function? It't not immediately obvious to me why moving this code is correct here.

Comment on attachment 125920 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125920&action=review I agree with Darin, this change is good but could be cleaner and less confusing. > Source/WebCore/rendering/FixedTableLayout.cpp:91 > Length grpWidth; This variable is unused now, it should be removed. > Source/WebCore/rendering/FixedTableLayout.cpp:-129 > - col->computePreferredLogicalWidths(); I concur with Eric here. You are moving the code after the following block which is: 1) useless as the code you touch don't use |child| and you actually may end up doing more work (if you have column-group)! 2) makes your change look artificially bigger / scarier / different. > Source/WebCore/rendering/FixedTableLayout.cpp:103 > + if (col->firstChild()) Would be nice to make this an helper function in RenderTableCol (like isColumnGroup()). > Source/WebCore/rendering/FixedTableLayout.cpp:106 > + Length w = col->style()->logicalWidth(); It should be columnStyleLogicalWidth. > Source/WebCore/rendering/FixedTableLayout.cpp:107 > + int effWidth = 0; effectiveColumnLogicalWidth? > Source/WebCore/rendering/FixedTableLayout.cpp:108 > + if (w.isFixed() && w.value() > 0) Interestingly we don't handle percent width here which is a discrepancy with the check below... Sounds like a bug to file or a big FIXME.

Comment on attachment 125920 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=125920&action=review >> Source/WebCore/rendering/FixedTableLayout.cpp:-129 >> - col->computePreferredLogicalWidths(); > > I concur with Eric here. You are moving the code after the following block which is: > 1) useless as the code you touch don't use |child| and you actually may end up doing more work (if you have column-group)! > 2) makes your change look artificially bigger / scarier / different. I take this one back after sleeping on it. You need to make sure |child| is progressing in your loop, however it sounds like you could achieve the same result if you used a for loop now, which would make the logic cleared and more bullet proof.

Created attachment 126147 [details] Patch

(In reply to comment #3) > > Interestingly we don't handle percent width here which is a discrepancy with the check below... Sounds like a bug to file or a big FIXME. I'll take a closer look at this one in another bug. Addressing your comments has made the patch even harder to read unfortunately. :/

Comment on attachment 126147 [details] Patch 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.

(In reply to comment #7) > This branch has disappeared in your change. It is possible it is unneeded but this is a separate change. No, it's still there. It's just very fragmented in the diff. r+ ? :)

Comment on attachment 126147 [details] Patch > r+ ? :) Your deadly kawai eyes won me :-) (and the fact that if I had followed more closely the logic, I would have seen the change did not miss anything).

Committed r107970: <http://trac.webkit.org/changeset/107970>

(In reply to comment #10) > Committed r107970: <http://trac.webkit.org/changeset/107970> This patch was rolled out: Committed r108237: <http://trac.webkit.org/changeset/108237> It breaks column widths updates from javascript, (e.g. inspector's network panel). I am attaching a reduced reproduction.

Created attachment 127816 [details] column-width-problem Button should change column width from 100 to 50 and back. It does so in firefox and it did it in chrome before this patch.

(In reply to comment #12) > Created an attachment (id=127816) [details] > column-width-problem > > Button should change column width from 100 to 50 and back. > It does so in firefox and it did it in chrome before this patch. Thanks for spotting this and rolling out. Unfortunately, this regression was caused by a dumb mistake in the refactoring that wasn't caught by any layout-tests. So I need to write a few more tests before fixing and re-landing.

Created attachment 132119 [details] Patch

Comment on attachment 132119 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132119&action=review > Source/WebCore/rendering/FixedTableLayout.cpp:110 > + col->computePreferredLogicalWidths(); This was the culprit. :/

Comment on attachment 132119 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=132119&action=review > Source/WebCore/rendering/FixedTableLayout.cpp:105 > + for (;child && child->isTableCol(); child = nextCol(child)) { The initialization ("RenderObject* child = m_table->firstChild();") should be moved here too. Also space after ';' please. > Source/WebCore/rendering/FixedTableLayout.cpp:143 > col->computePreferredLogicalWidths(); AFAICT this doesn't depend on any of the computation we are doing here so it could just be hoisted before your |if| (around line 109) to avoid another breakage down the line.

Committed r111118: <http://trac.webkit.org/changeset/111118>