Table with fixed layout behaves like auto layout when its width is set by js instead of css.

Table with fixed layout behaves like auto layout when its width is set by js instead of css.

The new width of table was not being applied when changed dynamically
even though the table-layout is fixed.

On calculating whether we need to set the m_tableLayout as fixed or
auto the change in layout parameters such as width, height etc was
not considered hence the right layout type was not being set. Incase
there are any layout changes we need to see whether m_tableLayout
needs to be changed or not.

R=jchaffraix@chromium.org
BUG=239421
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=173713

[Gurpreet, 2014-05-06 12:58:06]

Hi Julien, Can you please review this. Thanks.

[mstensho (USE GERRIT), 2014-05-06 13:28:59]

https://codereview.chromium.org/264283002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderTable.cpp (right):

https://codereview.chromium.org/264283002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderTable.cpp:93: if (!m_tableLayout ||
style()->tableLayout() != oldTableLayout || diff.needsFullLayout()) {
It seems wrong to recreate the table layout object every time you want to lay
out the table. You only want to do this if it's missing or if you're really
going to switch layout algorithm.

The problem here is the "style()->tableLayout() != oldTableLayout" test. It
doesn't account for the fact that auto width means auto table layout, even if
the specified value of 'table-layout' is 'fixed'.

A helper method (e.g. "isTableLayoutFixed()") in RenderStyle, which checks both
for table-layout:fixed AND non-auto width, could be a better solution. It could
be called both on this line, and on a few lines further below (which already
does the right thing).

[Gurpreet, 2014-05-06 15:06:03]

Thanks Morten for the review. I agree with you. Have done the changes as per
your suggestion .

[mstensho (USE GERRIT), 2014-05-06 19:29:06]

https://codereview.chromium.org/264283002/diff/10001/Source/core/rendering/Re...
File Source/core/rendering/RenderTable.cpp (right):

https://codereview.chromium.org/264283002/diff/10001/Source/core/rendering/Re...
Source/core/rendering/RenderTable.cpp:91: if (!m_tableLayout ||
style()->isTableLayoutFixed()) {
Need to compare old fixedness against new, like it used to do.

Something like:

bool wasFixed = oldStyle && oldStyle->isTableLayoutFixed();

...

if (!m_tableLayout || wasFixed != style()->isTableLayoutFixed())

https://codereview.chromium.org/264283002/diff/10001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyle.h (right):

https://codereview.chromium.org/264283002/diff/10001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyle.h:566: bool isTableLayoutFixed() const {
return static_cast<ETableLayout>(noninherited_flags._table_layout) == TFIXED &&
!logicalWidth().isAuto(); }
Nit: Could call tableLayout() instead.

[Julien - ping for review, 2014-05-07 00:35:29]

If you implement Morten's suggestion (plus a couple from me), the change would
look great!

https://codereview.chromium.org/264283002/diff/10001/LayoutTests/fast/table/f...
File LayoutTests/fast/table/fixed-table-layout-width-change.html (right):

https://codereview.chromium.org/264283002/diff/10001/LayoutTests/fast/table/f...
LayoutTests/fast/table/fixed-table-layout-width-change.html:6: outer =
document.getElementById('outer');
"var outer" to avoid attaching outer to the global context (aka window)?

https://codereview.chromium.org/264283002/diff/10001/LayoutTests/fast/table/f...
LayoutTests/fast/table/fixed-table-layout-width-change.html:26: Tests that the
width of table changes on changing the table width to new width.New width should
be 100.
Awesome that you considered mentioning the new width but it's already included
in the failing output of check-layout.js so it's redundant.

https://codereview.chromium.org/264283002/diff/10001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyle.h (right):

https://codereview.chromium.org/264283002/diff/10001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyle.h:566: bool isTableLayoutFixed() const {
return static_cast<ETableLayout>(noninherited_flags._table_layout) == TFIXED &&
!logicalWidth().isAuto(); }
On 2014/05/06 19:29:07, Morten Stenshorne wrote:
> Nit: Could call tableLayout() instead.

Also in English, you would say "is it a fixed table layout?" so a better boolean
name is isFixedTableLayout.

[Gurpreet, 2014-05-07 05:00:13]

Thanks Morten and Julien for the review. Have done the changes. Please review.

[mstensho (USE GERRIT), 2014-05-07 19:12:27]

non-owner lgtm with nits. I also believe that one of Julien's issues hasn't been
addressed the way he had in mind.

https://codereview.chromium.org/264283002/diff/30001/Source/core/rendering/Re...
File Source/core/rendering/RenderTable.cpp (right):

https://codereview.chromium.org/264283002/diff/30001/Source/core/rendering/Re...
Source/core/rendering/RenderTable.cpp:86: bool fixedTableLayout = oldStyle ?
oldStyle->isFixedTableLayout() : false;
Nit: I'd just say "oldStyle && oldStyle->isFixedTableLayout()". Keeping "old" as
part of the variable name would be nice too.

https://codereview.chromium.org/264283002/diff/30001/Source/core/rendering/st...
File Source/core/rendering/style/RenderStyle.h (right):

https://codereview.chromium.org/264283002/diff/30001/Source/core/rendering/st...
Source/core/rendering/style/RenderStyle.h:566: bool isFixedTableLayout() const {
return static_cast<ETableLayout>(noninherited_flags._table_layout) == TFIXED &&
!logicalWidth().isAuto(); }
Nit: still duplicating the
"static_cast<ETableLayout>(noninherited_flags._table_layout)" thing here, rather
than just calling tableLayout().

[Julien - ping for review, 2014-05-09 01:53:41]

lgtm

[commit-bot: I haz the power, 2014-05-09 01:54:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/k.gurpreet@samsung.com/264283002/50001

[commit-bot: I haz the power, 2014-05-09 03:19:22]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/6767)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/6738)

[commit-bot: I haz the power, 2014-05-09 03:57:43]

Change committed as 173713