Col width is not honored when dynamically updated and it would not make table narrower

Col width is not honored when dynamically updated and it would not make table narrower

Increasing the table width by increasing the colgroup width was working
but decreasing the table width by decreasing the colgroup width is not working.

When colgroup width is defined table cell should adjust according to that.
On decreasing colgroup width the cells maxPreferredLogicalWidth was
still set to the earlier value. Setting the setPreferredLogicalWidthsDirty
to true so that cells pref width is calculated again.

R=jchaffraix@chromium.org,mstensho@opera.com
BUG=340252, 104316
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188289

[Gurpreet, 2014-03-24 08:35:43]

Hi Julien. Can you please review? Thanks.

[mstensho (USE GERRIT), 2014-03-24 08:44:31]

Does this fix https://code.google.com/p/chromium/issues/detail?id=104316 ?

[Gurpreet, 2014-03-24 10:21:56]

On 2014/03/24 08:44:31, Morten Stenshorne wrote:
> Does this fix https://code.google.com/p/chromium/issues/detail?id=104316 ?

Yes it does.

[Julien - ping for review, 2014-03-25 00:04:02]

On 2014/03/24 10:21:56, Gurpreet wrote:
> On 2014/03/24 08:44:31, Morten Stenshorne wrote:
> > Does this fix https://code.google.com/p/chromium/issues/detail?id=104316 ?
> 
> Yes it does.

You can put multiple bugs in the BUG= line (separate them with commas).
Alternatively, we should duplicate this bug against 340252.

Conceptually, you should land the crash fix with this change. This avoids having
a period where ClusterFuzz will wrongly report the same crash.

[Gurpreet, 2014-03-26 13:30:19]

Hi Julien done with the changes as you suggested. Please have a look. Thanks.

[Julien - ping for review, 2014-03-29 01:04:21]

https://codereview.chromium.org/208263013/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderTableSection.cpp (right):

https://codereview.chromium.org/208263013/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderTableSection.cpp:1546: m_grid.clear();
As stated in codereview.chromium.org/208263012 (based on the comments in
https://bugs.webkit.org/show_bug.cgi?id=129561), this doesn't seem to be the
right fix. It shouldn't solve a crasher (or if it does, I would like to have
some investigation as to why) but is still a good defense-in-depth pattern.

[Gurpreet, 2014-04-01 14:39:39]

On 2014/03/29 01:04:21, Julien Chaffraix - PST wrote:
>
https://codereview.chromium.org/208263013/diff/20001/Source/core/rendering/Re...
> File Source/core/rendering/RenderTableSection.cpp (right):
> 
>
https://codereview.chromium.org/208263013/diff/20001/Source/core/rendering/Re...
> Source/core/rendering/RenderTableSection.cpp:1546: m_grid.clear();
> As stated in codereview.chromium.org/208263012 (based on the comments in
> https://bugs.webkit.org/show_bug.cgi?id=129561), this doesn't seem to be the
> right fix. It shouldn't solve a crasher (or if it does, I would like to have
> some investigation as to why) but is still a good defense-in-depth pattern.

Hi Julien. Sorry for the late reply. With the above patch I tested the test
cases attached at the end in
https://code.google.com/p/chromium/issues/detail?id=351729. Its working fine. 
So you suggest this fix is not complete or not right?

[Julien - ping for review, 2014-04-03 16:54:49]

On 2014/04/01 14:39:39, Gurpreet wrote:
> On 2014/03/29 01:04:21, Julien Chaffraix - PST wrote:
> >
>
https://codereview.chromium.org/208263013/diff/20001/Source/core/rendering/Re...
> > File Source/core/rendering/RenderTableSection.cpp (right):
> > 
> >
>
https://codereview.chromium.org/208263013/diff/20001/Source/core/rendering/Re...
> > Source/core/rendering/RenderTableSection.cpp:1546: m_grid.clear();
> > As stated in codereview.chromium.org/208263012 (based on the comments in
> > https://bugs.webkit.org/show_bug.cgi?id=129561), this doesn't seem to be the
> > right fix. It shouldn't solve a crasher (or if it does, I would like to have
> > some investigation as to why) but is still a good defense-in-depth pattern.
> 
> Hi Julien. Sorry for the late reply. With the above patch I tested the test
> cases attached at the end in
> https://code.google.com/p/chromium/issues/detail?id=351729. Its working fine. 
> So you suggest this fix is not complete or not right?

The fix seems to work but I don't buy the rationale for it or at least it's
unclear *why* it is fixing anything. Also as written, the change adds some
duplication of code between the dirtying / recomputing sections that I don't
like.

[Gurpreet, 2014-04-07 11:08:07]

Hi Julien. In RenderTableCol::styleDidChange before setting the preferred
logical widths dirty table->recalcSectionsIfNeeded(); is being called and this
would recalc the sections so that old data doesnot cause the crash.

[Gurpreet, 2014-04-30 07:58:32]

Hi Julien. In RenderTableCol::styleDidChange before setting the preferred
logical widths dirty table->recalcSectionsIfNeeded(); is being called and this
would recalc the sections so that old data doesnot cause the crash.

[mstensho (USE GERRIT), 2014-08-27 11:40:01]

I'm not sure I understand how the crash relates to everything else, because I'm
not able to reproduce any crash with the attachments from
https://code.google.com/p/chromium/issues/detail?id=351729#c8 . Not with this
patch, not without it, not even with half of it (without patch set 2).

But anyway, would it be an idea to just walk the render tree (for each table
cell in each table row in each table section) instead of using primaryCellAt()?
As long as we're not trying to limit the set of cells to the column in question
anyway, that should be just as fast, and then there should be no need for all
the additional calls to recalcCellsIfNeeded(). Right? There shouldn't even be
any need for recalcSectionsIfNeeded() then.

[Gurpreet, 2014-10-09 12:18:14]

Hi sorry about the late reply. You mean considering patch 1 there is no crash in
the latest code and it works fine?

[mstensho (USE GERRIT), 2014-10-10 11:31:14]

On 2014/10/09 12:18:14, Gurpreet wrote:
> Hi sorry about the late reply. You mean considering patch 1 there is no crash
in
> the latest code and it works fine?

Yes.

[Gurpreet, 2014-10-10 12:01:57]

Oh thats good.
Hi Julien. What do you suggest then?

[mstensho (USE GERRIT), 2015-01-07 10:06:11]

Maybe we could finish this? I think the crash fixes should be removed from this
CL. I didn't find any crashers, so I don't understand why it's necessary to fix
anything.

[Gurpreet, 2015-01-07 10:09:56]

Will submit new patch within a day or two i.e what was submitted in Patch Set 1.
Fine right?

[mstensho (USE GERRIT), 2015-01-07 10:19:01]

On 2015/01/07 10:09:56, Gurpreet wrote:
> Will submit new patch within a day or two i.e what was submitted in Patch Set
1.
> Fine right?

Yes, and please remove the ClusterFuzz paragraph from the commit message.

[Gurpreet, 2015-01-09 12:38:03]

Done. Please review.

[mstensho (USE GERRIT), 2015-01-09 21:21:36]

https://codereview.chromium.org/208263013/diff/80001/LayoutTests/fast/table/r...
File
LayoutTests/fast/table/resize-table-using-col-height-vertical-writing-mode.html
(right):

https://codereview.chromium.org/208263013/diff/80001/LayoutTests/fast/table/r...
LayoutTests/fast/table/resize-table-using-col-height-vertical-writing-mode.html:2:
<html>
Please omit unnecessary HTML, HEAD and BODY tags here and in all the other
files.

https://codereview.chromium.org/208263013/diff/80001/LayoutTests/fast/table/r...
LayoutTests/fast/table/resize-table-using-col-height-vertical-writing-mode.html:5:
col {
Indentation is off, here, and in all the other files at the same place.

https://codereview.chromium.org/208263013/diff/80001/LayoutTests/fast/table/r...
LayoutTests/fast/table/resize-table-using-col-height-vertical-writing-mode.html:14:
function runTest() {
To get rid of the BODY tag, you could rewrite this as "onload = function() {"

https://codereview.chromium.org/208263013/diff/80001/LayoutTests/fast/table/r...
LayoutTests/fast/table/resize-table-using-col-height-vertical-writing-mode.html:18:
window.checkLayout("td", document.getElementById("test-output"));
No need for "window.".

https://codereview.chromium.org/208263013/diff/80001/LayoutTests/fast/table/r...
LayoutTests/fast/table/resize-table-using-col-height-vertical-writing-mode.html:31:
<td id="colWidth" data-expected-height="100">Hello World</td>
Unused id.

https://codereview.chromium.org/208263013/diff/80001/LayoutTests/fast/table/r...
File LayoutTests/fast/table/resize-table-width-using-col-colgroup-span.html
(right):

https://codereview.chromium.org/208263013/diff/80001/LayoutTests/fast/table/r...
LayoutTests/fast/table/resize-table-width-using-col-colgroup-span.html:17:
window.checkLayout("#td1", document.getElementById("test-output"));
No need for "window.".

I'd also store document.getElementById("test-output") as a variable.

Similar issue in other files.

https://codereview.chromium.org/208263013/diff/80001/Source/core/rendering/Re...
File Source/core/rendering/RenderTableCol.cpp (right):

https://codereview.chromium.org/208263013/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderTableCol.cpp:65: // the col width should have
preferred logical widths recomputed.
-"should".

https://codereview.chromium.org/208263013/diff/80001/Source/core/rendering/Re...
Source/core/rendering/RenderTableCol.cpp:66: table->recalcSectionsIfNeeded();
See https://codereview.chromium.org/208263013/#msg11

How about walking the render tree (to find each cell) here, instead of using
things that rely on section ordering and grids being up-to-date? That way you
won't have to call recalcSectionsIfNeeded().

[Gurpreet, 2015-01-12 08:45:48]

Thanks a lot for the review. I have modified the test cases as per your
comments.

RenderTableCol calling recalcSectionsIfNeeded() is actually not required. The
original patch also doesnot have this. 

Also your suggestion was to walk the render tree to find each cell but in that
case wil have to find section, row and then cells. Wont that end up to the same
thing? Correct me if I am wrong.

[mstensho (USE GERRIT), 2015-01-12 09:05:50]

> RenderTableCol calling recalcSectionsIfNeeded() is actually not required. The
> original patch also doesnot have this. 

If you want to work with functions that depend on section data being up to date,
you better make sure that they *are* up to date.

Try this and it will now assert:

<!DOCTYPE html>
<table style="background:pink;">
    <col id="elm" style="width:200px;">
    <tr>
        <td>click me</td>
    </tr>
    <tfoot id="section" style="display:none;">
        <tr>
            <td>hello</td>
        </tr>
    </tfoot>
</table>
<script>
    onclick = function() {
        document.getElementById('section').style.display = 'table-footer-group';
        document.getElementById('elm').style.width = '100px';
    }
</script>

> Also your suggestion was to walk the render tree to find each cell but in that
> case wil have to find section, row and then cells. Wont that end up to the
same
> thing? Correct me if I am wrong.

No, if you just walk the render tree normally (i.e. use
RenderTable::slowFirstChild() / nextSibling() to find and walk all sections
(need to skip captions), firstRow() / nextRow() to walk all rows in each
section, and firstCell() / nextCell() to walk all cells in each row), you don't
need to perform the pending section recalc, if any.

https://codereview.chromium.org/208263013/diff/100001/LayoutTests/fast/table/...
File
LayoutTests/fast/table/resize-table-using-col-height-vertical-writing-mode.html
(right):

https://codereview.chromium.org/208263013/diff/100001/LayoutTests/fast/table/...
LayoutTests/fast/table/resize-table-using-col-height-vertical-writing-mode.html:1:
<style>
Still need <!DOCTYPE html> here and in all other tests.

We don't want to enter quirks mode unless it's necessary for the test.

[Gurpreet, 2015-01-12 13:01:04]

Sorry for the delay. Done. Please review.

[Gurpreet, 2015-01-12 13:02:29]

Also tested my earlier patch and recent patch. It was asserting with the test
code which you mentioned with the earlier patch.

[mstensho (USE GERRIT), 2015-01-12 14:00:09]

https://codereview.chromium.org/208263013/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderTableCol.cpp (right):

https://codereview.chromium.org/208263013/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderTableCol.cpp:67: if (child->isRenderTableCol())
Please change to "if (!child->isRenderTableSection())\n continue;").

Need to skip captions too.

https://codereview.chromium.org/208263013/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderTableCol.cpp:69: if (child->isTableSection()) {
So no need for this check if you do as I suggested above.

[Gurpreet, 2015-01-12 14:14:14]

https://codereview.chromium.org/208263013/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderTableCol.cpp (right):

https://codereview.chromium.org/208263013/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderTableCol.cpp:67: if (child->isRenderTableCol())
On 2015/01/12 14:00:09, mstensho wrote:
> Please change to "if (!child->isRenderTableSection())\n continue;").
> 
> Need to skip captions too.

But the check if(!child->isRenderTableSection()) will take care of caption too
right? I mean is its caption then it will continue.

[mstensho (USE GERRIT), 2015-01-12 16:07:52]

https://codereview.chromium.org/208263013/diff/120001/Source/core/rendering/R...
File Source/core/rendering/RenderTableCol.cpp (right):

https://codereview.chromium.org/208263013/diff/120001/Source/core/rendering/R...
Source/core/rendering/RenderTableCol.cpp:67: if (child->isRenderTableCol())
On 2015/01/12 14:14:14, Gurpreet wrote:
> On 2015/01/12 14:00:09, mstensho wrote:
> > Please change to "if (!child->isRenderTableSection())\n continue;").
> > 
> > Need to skip captions too.
> 
> But the check if(!child->isRenderTableSection()) will take care of caption too
> right? I mean is its caption then it will continue.

You're right, of course. The code works correctly as it is. But why waste CPU
time and code lines on checking if it's a column, since you require it to be a
section afterwards anyway?

[Gurpreet, 2015-01-13 05:09:56]

Done. Please review. Thanks.

[mstensho (USE GERRIT), 2015-01-13 08:34:48]

https://codereview.chromium.org/208263013/diff/140001/Source/core/rendering/R...
File Source/core/rendering/RenderTableCol.cpp (right):

https://codereview.chromium.org/208263013/diff/140001/Source/core/rendering/R...
Source/core/rendering/RenderTableCol.cpp:67: if (child->isTableCaption() ||
!child->isTableSection())
A caption isn't a section, so why bother checking? "if
(!child->isTableSection())" should be enough.

[Gurpreet, 2015-01-13 08:38:34]

Some confusion maybe from my side :(
Earlier in review comment https://codereview.chromium.org/208263013/#msg24 you
had mentioned "Need to skip captions too." 
Comment #msg25 I said "But the check if(!child->isRenderTableSection()) will
take care of caption too right? I mean is its caption then it will continue."
Comment #msg26 you said "You're right, of course. The code works correctly as it
is. But why waste CPU
time and code lines on checking if it's a column, since you require it to be a
section afterwards anyway?"
So I thought maybe you wanna check for caption separately.

[mstensho (USE GERRIT), 2015-01-13 09:02:30]

On 2015/01/13 08:38:34, Gurpreet wrote:
> Some confusion maybe from my side :(
> Earlier in review comment https://codereview.chromium.org/208263013/#msg24 you
> had mentioned "Need to skip captions too." 
> Comment #msg25 I said "But the check if(!child->isRenderTableSection()) will
> take care of caption too right? I mean is its caption then it will continue."
> Comment #msg26 you said "You're right, of course. The code works correctly as
it
> is. But why waste CPU
> time and code lines on checking if it's a column, since you require it to be a
> section afterwards anyway?"
> So I thought maybe you wanna check for caption separately.

Sorry for being unclear. As you know, tables typically have three types of
children: columns/column groups, captions and sections. I didn't understand why
you worried about table columns explicitly in patch set 5 (since you'd check if
the child was a section right afterwards anyway), and furthermore, if you wanted
to worry about columns, why forget captions? :)

But my (poorly conveyed) point was: all you need to know is whether it's a
section or not, and you don't need additional tests for columns (or captions,
for that matter).

"if (!child->isRenderTableSection())\n continue;" is what we want.

[Gurpreet, 2015-01-13 09:26:58]

Submitted new patch.
You are right. This check "if (!child->isRenderTableSection())" is enough :) and
will take care of all.
Thanks for reviewing so patiently :)

[mstensho (USE GERRIT), 2015-01-13 09:38:38]

lgtm

[Gurpreet, 2015-01-13 09:42:54]

The CQ bit was checked by k.gurpreet@samsung.com

[commit-bot: I haz the power, 2015-01-13 09:43:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/208263013/160001

[commit-bot: I haz the power, 2015-01-13 11:54:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-13 11:54:04]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/45891)

[Gurpreet, 2015-01-13 11:56:26]

The CQ bit was checked by k.gurpreet@samsung.com

[commit-bot: I haz the power, 2015-01-13 11:56:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/208263013/160001

[commit-bot: I haz the power, 2015-01-13 12:55:50]

Committed patchset #7 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188289