Not able to set width using ColGroup when table-layout is fixed.

Not able to set width using ColGroup when table-layout is fixed.

When Some columns width are specified by colgroup than remaining columns
width is set as fixed width if colspan cell present in first row for those
columns.

Now by default these columns are set as auto width.

R=jchaffraix@chromium.org
BUG=20865

[Julien - ping for review, 2013-10-22 21:11:40]

https://codereview.chromium.org/27690002/diff/16001/Source/core/rendering/Fix...
File Source/core/rendering/FixedTableLayout.cpp (right):

https://codereview.chromium.org/27690002/diff/16001/Source/core/rendering/Fix...
Source/core/rendering/FixedTableLayout.cpp:165:
m_width[currentColumn].setValue(m_width[currentColumn].type(),
logicalWidth.intValue());
I don't understand how this line is correct: m_width[currentColumn].type() is
Auto if you are in this branch, setting a value in this case is just wrong.

If I am reading correctly, this line just ignore the logical width value.

[suchit.agrawal, 2013-10-23 13:41:05]

https://codereview.chromium.org/27690002/diff/16001/Source/core/rendering/Fix...
File Source/core/rendering/FixedTableLayout.cpp (right):

https://codereview.chromium.org/27690002/diff/16001/Source/core/rendering/Fix...
Source/core/rendering/FixedTableLayout.cpp:165:
m_width[currentColumn].setValue(m_width[currentColumn].type(),
logicalWidth.intValue());
On 2013/10/22 21:11:41, Julien Chaffraix wrote:
> I don't understand how this line is correct: m_width[currentColumn].type() is
> Auto if you are in this branch, setting a value in this case is just wrong.
> 
> If I am reading correctly, this line just ignore the logical width value.

If column is auto then we need to keep it auto.
By assigning 'logicalWidth' to 'm_width[currentColumn]',
'm_width[currentColumn]' become fixed. So now we are assigning 'logicalWidth' to
'm_width[currentColumn]' but setting 'm_width[currentColumn]' as Auto.

[Julien - ping for review, 2013-10-23 17:04:51]

https://codereview.chromium.org/27690002/diff/16001/LayoutTests/fast/table/ta...
File LayoutTests/fast/table/table-colgroup-width-not-set-with-fixed-layout.html
(right):

https://codereview.chromium.org/27690002/diff/16001/LayoutTests/fast/table/ta...
LayoutTests/fast/table/table-colgroup-width-not-set-with-fixed-layout.html:20:
<col width="100px">
These should be indented to match the rest of the file.

https://codereview.chromium.org/27690002/diff/16001/Source/core/rendering/Fix...
File Source/core/rendering/FixedTableLayout.cpp (right):

https://codereview.chromium.org/27690002/diff/16001/Source/core/rendering/Fix...
Source/core/rendering/FixedTableLayout.cpp:165:
m_width[currentColumn].setValue(m_width[currentColumn].type(),
logicalWidth.intValue());
On 2013/10/23 13:41:06, suchit.agrawal wrote:
> On 2013/10/22 21:11:41, Julien Chaffraix wrote:
> > I don't understand how this line is correct: m_width[currentColumn].type()
is
> > Auto if you are in this branch, setting a value in this case is just wrong.
> > 
> > If I am reading correctly, this line just ignore the logical width value.
> 
> If column is auto then we need to keep it auto.
> By assigning 'logicalWidth' to 'm_width[currentColumn]',
> 'm_width[currentColumn]' become fixed. So now we are assigning 'logicalWidth'
to
> 'm_width[currentColumn]' but setting 'm_width[currentColumn]' as Auto.

Again, you're explaining 'what' which can be inferred from the code. What I am
interested in is *why*.

Also what you are telling me is basically that we should ignore logicalWidth for
span > 1, we don't need this confusing line.

[eseidel, 2014-05-30 00:18:54]

Julien is the right reviewer for this change (if its even still needed).  Please
track him down in #blink if you aren't getting regular feedback.

It's possible we should just close this 7-month-old change.

[a.suchit2, 2014-06-02 06:31:45]

On 2014/05/30 00:18:54, eseidel wrote:
> Julien is the right reviewer for this change (if its even still needed). 
Please
> track him down in #blink if you aren't getting regular feedback.
> 
> It's possible we should just close this 7-month-old change.

Sorry for delay. I am not able to give time here. If you want, we can close this
issue right now and when I will start actively working than I can raise a new
change issue.