Row with only spanning cells have a height of 0.

Row with only spanning cells have a height of 0.

While distributing extra height of the row spanning cell into spanning rows,
Rows are not getting any height those contains only spanning cells because these
rows already having zero height.

Now, We are assigning one part of spanning cell's height based on their span
value to the rows those contains only spanning cells.

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

[Julien - ping for review, 2013-09-09 21:32:55]

Let's try to have a better description:

Row with only spanning cells have a height of 0

(contrary to your first line, this tells the reader what is wrong)

https://codereview.chromium.org/23530044/diff/1/Source/core/rendering/RenderT...
File Source/core/rendering/RenderTableSection.cpp (right):

https://codereview.chromium.org/23530044/diff/1/Source/core/rendering/RenderT...
Source/core/rendering/RenderTableSection.cpp:256: // Check weather row has only
spanning cells
s/weather/whether/ :(

This is yet another what comment that duplicates the name.

https://codereview.chromium.org/23530044/diff/1/Source/core/rendering/RenderT...
Source/core/rendering/RenderTableSection.cpp:257: unsigned
RenderTableSection::hasRowOnlySpanningCells(unsigned row)
rowHasOnlySpanningCells is more English.

https://codereview.chromium.org/23530044/diff/1/Source/core/rendering/RenderT...
Source/core/rendering/RenderTableSection.cpp:262: bool spanningCellStart =
false, emptyCell = false;
Style violation: we define 1 variable per line.

Why do we need those 2 variables that are never used in practice?

https://codereview.chromium.org/23530044/diff/1/Source/core/rendering/RenderT...
Source/core/rendering/RenderTableSection.cpp:275: return false;
An empty cell emplacement makes the result which is weird. Could you explain the
need for this?

https://codereview.chromium.org/23530044/diff/1/Source/core/rendering/RenderT...
Source/core/rendering/RenderTableSection.cpp:429: unsigned
RenderTableSection::calcRowHeightHavingOnlySpanningCells(unsigned row)
Note that the name makes people believe this only works on rows with only
spanning cells which is not the case at all.

https://codereview.chromium.org/23530044/diff/1/Source/core/rendering/RenderT...
Source/core/rendering/RenderTableSection.cpp:449: void
RenderTableSection::updateRowsHeightHavingOnlySpanningCells(RenderTableCell*
cell, struct SpanningRowsHeight& spanningRowsHeight)
Ditto.

https://codereview.chromium.org/23530044/diff/1/Source/core/rendering/RenderT...
Source/core/rendering/RenderTableSection.cpp:461: if
(!spanningRowsHeight.rowHeight[row] && hasRowOnlySpanningCells(actualRow)) {
This is now O(columns) as hasRowOnlySpanningCells needs to check all columns. We
should probably put a comment about it, mentioning that we would want to have
this cached somehow.

https://codereview.chromium.org/23530044/diff/1/Source/core/rendering/RenderT...
Source/core/rendering/RenderTableSection.cpp:521:
updateRowsHeightHavingOnlySpanningCells(cell, spanningRowsHeight);
Probably dumb question but why do we need all this *new* logic instead of
reusing the existing logic?

https://codereview.chromium.org/23530044/diff/1/Source/core/rendering/RenderT...
File Source/core/rendering/RenderTableSection.h (right):

https://codereview.chromium.org/23530044/diff/1/Source/core/rendering/RenderT...
Source/core/rendering/RenderTableSection.h:139: int
rowsCountWithOnlySpanningCells;
Do we need to keep track of the count or just whether we have one?

[a.suchit, 2013-09-10 13:47:59]

https://chromiumcodereview.appspot.com/23530044/diff/1/Source/core/rendering/...
File Source/core/rendering/RenderTableSection.cpp (right):

https://chromiumcodereview.appspot.com/23530044/diff/1/Source/core/rendering/...
Source/core/rendering/RenderTableSection.cpp:256: // Check weather row has only
spanning cells
On 2013/09/09 21:32:55, Julien Chaffraix wrote:
> s/weather/whether/ :(
> 
> This is yet another what comment that duplicates the name.

This comment is not needed so removed it.

https://chromiumcodereview.appspot.com/23530044/diff/1/Source/core/rendering/...
Source/core/rendering/RenderTableSection.cpp:257: unsigned
RenderTableSection::hasRowOnlySpanningCells(unsigned row)
On 2013/09/09 21:32:55, Julien Chaffraix wrote:
> rowHasOnlySpanningCells is more English.

Done.

https://chromiumcodereview.appspot.com/23530044/diff/1/Source/core/rendering/...
Source/core/rendering/RenderTableSection.cpp:262: bool spanningCellStart =
false, emptyCell = false;
On 2013/09/09 21:32:55, Julien Chaffraix wrote:
> Style violation: we define 1 variable per line.
> 
> Why do we need those 2 variables that are never used in practice?

Not needed. Removed

https://chromiumcodereview.appspot.com/23530044/diff/1/Source/core/rendering/...
Source/core/rendering/RenderTableSection.cpp:275: return false;
On 2013/09/09 21:32:55, Julien Chaffraix wrote:
> An empty cell emplacement makes the result which is weird. Could you explain
the
> need for this?

Empty cell does not say any thing and we are checking here for all row spanning
cells in the row.
In actuality, Empty cell do not have any roll in layout and render and even if
empty cell considers in layout and render then it would be 1 span cell.

https://chromiumcodereview.appspot.com/23530044/diff/1/Source/core/rendering/...
Source/core/rendering/RenderTableSection.cpp:429: unsigned
RenderTableSection::calcRowHeightHavingOnlySpanningCells(unsigned row)
On 2013/09/09 21:32:55, Julien Chaffraix wrote:
> Note that the name makes people believe this only works on rows with only
> spanning cells which is not the case at all.

Here is considered that row passed here have only spanning cells.

Added assert to check for row with only spanning cells.

https://chromiumcodereview.appspot.com/23530044/diff/1/Source/core/rendering/...
Source/core/rendering/RenderTableSection.cpp:449: void
RenderTableSection::updateRowsHeightHavingOnlySpanningCells(RenderTableCell*
cell, struct SpanningRowsHeight& spanningRowsHeight)
On 2013/09/09 21:32:55, Julien Chaffraix wrote:
> Ditto.

Down we are checking for row with only spanning cells than we are updating the
height of the rows.

https://chromiumcodereview.appspot.com/23530044/diff/1/Source/core/rendering/...
Source/core/rendering/RenderTableSection.cpp:461: if
(!spanningRowsHeight.rowHeight[row] && hasRowOnlySpanningCells(actualRow)) {
On 2013/09/09 21:32:55, Julien Chaffraix wrote:
> This is now O(columns) as hasRowOnlySpanningCells needs to check all columns.
We
> should probably put a comment about it, mentioning that we would want to have
> this cached somehow.

We are checking only rows those have 0 height. In most of the cases, row height
would be 0 only when row with only spanning cells.

https://chromiumcodereview.appspot.com/23530044/diff/1/Source/core/rendering/...
Source/core/rendering/RenderTableSection.cpp:521:
updateRowsHeightHavingOnlySpanningCells(cell, spanningRowsHeight);
On 2013/09/09 21:32:55, Julien Chaffraix wrote:
> Probably dumb question but why do we need all this *new* logic instead of
> reusing the existing logic?

Existing logic works for only when row have height.
Row with only spanning cells have 0 height so with existing logic, height will
remain 0.
First, we are giving a some valid height to these row and than applying existing
logic so these rows will get proper height.

https://chromiumcodereview.appspot.com/23530044/diff/1/Source/core/rendering/...
File Source/core/rendering/RenderTableSection.h (right):

https://chromiumcodereview.appspot.com/23530044/diff/1/Source/core/rendering/...
Source/core/rendering/RenderTableSection.h:139: int
rowsCountWithOnlySpanningCells;
On 2013/09/09 21:32:55, Julien Chaffraix wrote:
> Do we need to keep track of the count or just whether we have one?

just whether we have one. Changed it to bool type.

[Julien - ping for review, 2013-09-17 16:51:25]

Much better, still some comments.

https://codereview.chromium.org/23530044/diff/47001/LayoutTests/fast/table/00...
File LayoutTests/fast/table/007-expected.txt (right):

https://codereview.chromium.org/23530044/diff/47001/LayoutTests/fast/table/00...
LayoutTests/fast/table/007-expected.txt:21: RenderText {#text} at (1,1) size
397x19
This is a regression but compared to the massive progressions from this patch,
it's a good trade-off.

https://codereview.chromium.org/23530044/diff/47001/Source/core/rendering/Ren...
File Source/core/rendering/RenderTableSection.cpp (right):

https://codereview.chromium.org/23530044/diff/47001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:258: ASSERT(row < m_grid.size());
This will be caught by Vector::at() (even in release, out of bounds access are
really nasty) at line 260 as you do m_grid[row].

https://codereview.chromium.org/23530044/diff/47001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:266: CellStruct rowSpanCell =
cellAt(row, col);
Let's avoid a copy by using a reference here. Also it should probably be const?

https://codereview.chromium.org/23530044/diff/47001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:272: }
Let's write this (more readable with the early return):

if (rowSpan.cells.isEmpty()) {
    // Some explanations here for why we consider one empty cell to make the
return false as it's still unclear to me. Please let's make it a why comment.
    return false;
}

if (rowSpanCell.cells[0]->rowSpan() == 1)
    return false;

https://codereview.chromium.org/23530044/diff/47001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:292:
spanningRowsHeight.rowWithOnlySpanningCells = true;
We can remove the inner branch (yay!):

spanningRowsHeight.rowWithOnlySpanningCells |=
rowHasOnlySpanningCells(actualRow);

https://codereview.chromium.org/23530044/diff/47001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:425: unsigned
RenderTableSection::calcRowHeightHavingOnlySpanningCells(unsigned row)
This function is never called.

https://codereview.chromium.org/23530044/diff/47001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:427: ASSERT(row < m_grid.size());
Again, duplicated by Vector::at().

https://codereview.chromium.org/23530044/diff/47001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:454: ASSERT(rowSpan ==
spanningRowsHeight.rowHeight.size());
Nice!

[a.suchit, 2013-09-18 11:40:37]

https://codereview.chromium.org/23530044/diff/47001/Source/core/rendering/Ren...
File Source/core/rendering/RenderTableSection.cpp (right):

https://codereview.chromium.org/23530044/diff/47001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:258: ASSERT(row < m_grid.size());
On 2013/09/17 16:51:26, Julien Chaffraix wrote:
> This will be caught by Vector::at() (even in release, out of bounds access are
> really nasty) at line 260 as you do m_grid[row].

returned if row have more than vector size.

https://codereview.chromium.org/23530044/diff/47001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:266: CellStruct rowSpanCell =
cellAt(row, col);
On 2013/09/17 16:51:26, Julien Chaffraix wrote:
> Let's avoid a copy by using a reference here. Also it should probably be
const?

Done.

https://codereview.chromium.org/23530044/diff/47001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:272: }
On 2013/09/17 16:51:26, Julien Chaffraix wrote:
> Let's write this (more readable with the early return):
> 
> if (rowSpan.cells.isEmpty()) {
>     // Some explanations here for why we consider one empty cell to make the
> return false as it's still unclear to me. Please let's make it a why comment.
>     return false;
> }
> 
> if (rowSpanCell.cells[0]->rowSpan() == 1)
>     return false;

Done.

https://codereview.chromium.org/23530044/diff/47001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:292:
spanningRowsHeight.rowWithOnlySpanningCells = true;
On 2013/09/17 16:51:26, Julien Chaffraix wrote:
> We can remove the inner branch (yay!):
> 
> spanningRowsHeight.rowWithOnlySpanningCells |=
> rowHasOnlySpanningCells(actualRow);

Done.

https://codereview.chromium.org/23530044/diff/47001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:425: unsigned
RenderTableSection::calcRowHeightHavingOnlySpanningCells(unsigned row)
On 2013/09/17 16:51:26, Julien Chaffraix wrote:
> This function is never called.

It is called by function "updateRowsHeightHavingOnlySpanningCells" in line : 459

https://codereview.chromium.org/23530044/diff/47001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:427: ASSERT(row < m_grid.size());
On 2013/09/17 16:51:26, Julien Chaffraix wrote:
> Again, duplicated by Vector::at().

returned if row have more than vector size.

[Julien - ping for review, 2013-09-18 18:39:37]

This looks really good, it still need an extra round though.

https://codereview.chromium.org/23530044/diff/61001/Source/core/rendering/Ren...
File Source/core/rendering/RenderTableSection.cpp (right):

https://codereview.chromium.org/23530044/diff/61001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:258: ASSERT(row < m_grid.size());
This ...

https://codereview.chromium.org/23530044/diff/61001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:260: if (row >= m_grid.size())
.. and this are unneeded ...

https://codereview.chromium.org/23530044/diff/61001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:263: unsigned totalCols =
m_grid[row].row.size();
... because m_grid[row] calls Vector::at(size_t i) which is implemented as
follows:

RELEASE_ASSERT(i < size());
return Base::buffer()[i];

(The RELEASE_ASSERT is what duplicates both of your statements)

https://codereview.chromium.org/23530044/diff/61001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:271: // Empty cell would be 1
rowspan cell only if we consider that empty cell is valid cell.
The more I read this comment the less I understand what you mean. Is an empty
cell a rowspan cell? Is an empty cell a valid cell? What does that even mean to
be a "valid cell"?

https://codereview.chromium.org/23530044/diff/61001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:272: if
(rowSpanCell.cells.isEmpty() || rowSpanCell.cells[0]->rowSpan() == 1)
Nit: I would have separated these 2 conditions so that they are not confused
with each other.

https://codereview.chromium.org/23530044/diff/61001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:427: ASSERT(row < m_grid.size());
Same comment as above, unneeded and should be removed.

https://codereview.chromium.org/23530044/diff/61001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:430: if (row >= m_grid.size())
Same comment as above, unneeded and should be removed.

https://codereview.chromium.org/23530044/diff/61001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:441: CellStruct rowSpanCell =
cellAt(row, col);
Again, const reference to avoid an implicit copy! (unfortunately CellStruct
cannot be MAKE_NON_COPYABLE due to Vector :()

[a.suchit, 2013-09-19 08:18:36]

fast/table/007.html

I have checked this test case. Based on the test case it is fine.
But we can improve it in the next patch.

https://codereview.chromium.org/23530044/diff/61001/Source/core/rendering/Ren...
File Source/core/rendering/RenderTableSection.cpp (right):

https://codereview.chromium.org/23530044/diff/61001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:258: ASSERT(row < m_grid.size());
On 2013/09/18 18:39:37, Julien Chaffraix wrote:
> This ...

Done.

https://codereview.chromium.org/23530044/diff/61001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:260: if (row >= m_grid.size())
On 2013/09/18 18:39:37, Julien Chaffraix wrote:
> .. and this are unneeded ...

Done.

https://codereview.chromium.org/23530044/diff/61001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:271: // Empty cell would be 1
rowspan cell only if we consider that empty cell is valid cell.
On 2013/09/18 18:39:37, Julien Chaffraix wrote:
> The more I read this comment the less I understand what you mean. Is an empty
> cell a rowspan cell? Is an empty cell a valid cell? What does that even mean
to
> be a "valid cell"?

Done.

https://codereview.chromium.org/23530044/diff/61001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:272: if
(rowSpanCell.cells.isEmpty() || rowSpanCell.cells[0]->rowSpan() == 1)
On 2013/09/18 18:39:37, Julien Chaffraix wrote:
> Nit: I would have separated these 2 conditions so that they are not confused
> with each other.

Done.

https://codereview.chromium.org/23530044/diff/61001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:427: ASSERT(row < m_grid.size());
On 2013/09/18 18:39:37, Julien Chaffraix wrote:
> Same comment as above, unneeded and should be removed.

Done.

https://codereview.chromium.org/23530044/diff/61001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:430: if (row >= m_grid.size())
On 2013/09/18 18:39:37, Julien Chaffraix wrote:
> Same comment as above, unneeded and should be removed.

Done.

https://codereview.chromium.org/23530044/diff/61001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:441: CellStruct rowSpanCell =
cellAt(row, col);
On 2013/09/18 18:39:37, Julien Chaffraix wrote:
> Again, const reference to avoid an implicit copy! (unfortunately CellStruct
> cannot be MAKE_NON_COPYABLE due to Vector :()

Done.

[Julien - ping for review, 2013-09-19 16:33:52]

lgtm

[commit-bot: I haz the power, 2013-09-19 16:34:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/a.suchit@samsung.com/23530044/51001

[commit-bot: I haz the power, 2013-09-19 17:43:42]

Change committed as 158038