Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
596823002:
Tables with specific merge cell configuration render with extra height to tr elements. (Closed)
|
Created:
6 years, 3 months ago by a.suchit Modified:
6 years ago Reviewers:
a.suchit2, leviw_travelin_and_unemployed, dsinclair, esprehn, suchit.agrawal, Julien - ping for review CC:
blink-reviews, blink-reviews-rendering, eae+blinkwatch, jchaffraix+rendering, leviw+renderwatch, pdr., rune+blink, zoltan1 Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Project:
blink Visibility:
Public. |
DescriptionTables with specific merge cell configuration render with extra height to tr elements.
Rows with only rowspan cells was not getting proper height. This row just
getting the one part of the existing height of rowspan cell.
Now, Calculating extra height required for the spanning cells in the row and
one portion of this height is allocated to it based on number of rows with
only rowspan cells having zero height.
R=jchaffraix@chromium.org, eseidel@chromium.org, leviw@chromium.org, rschoen@chromium.org
BUG=396655, 377518
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=187448
Patch Set 1 : Patch gives bad perfromance #Patch Set 2 : Improved #Patch Set 3 : Fixed #Patch Set 4 : Rebase and Added test in TestExpectation for rebaseline on Win and Mac #Patch Set 5 : Rebase #Patch Set 6 : Rebase #Patch Set 7 : Rebase #Patch Set 8 : Rebase #
Total comments: 12
Patch Set 9 : Review comments addressed #
Total comments: 42
Patch Set 10 : Review comments addresses #
Total comments: 22
Patch Set 11 : Temp #Patch Set 12 : Review comments Addressed #Patch Set 13 : Rebase #
Total comments: 10
Patch Set 14 : Rebase #
Total comments: 6
Patch Set 15 : Review comments addressed #Patch Set 16 : Rebase #Messages
Total messages: 47 (16 generated)
a.suchit@chromium.org changed reviewers: + eseidel@chromium.org, jchaffraix@chromium.org
a.suchit@samsung.com changed reviewers: + leviw@chromium.org, rschoen@google.com
On 2014/09/25 10:17:10, a.suchit wrote: > mailto:a.suchit@samsung.com changed reviewers: > + mailto:leviw@chromium.org, mailto:rschoen@google.com Please review it, I need to just update the testExpectation file for mac and win. Thanks
On 2014/09/26 02:49:20, suchit.agrawal wrote: > On 2014/09/25 10:17:10, a.suchit wrote: > > mailto:a.suchit@samsung.com changed reviewers: > > + mailto:leviw@chromium.org, mailto:rschoen@google.com > > Please review it, I need to just update the testExpectation file for mac and > win. Thanks crbug.com/377518 also fixed with this patch.
a.suchit@samsung.com changed reviewers: + esprehn@chromium.org, pdr@chromium.org
On 2014/09/27 06:34:54, a.suchit wrote: > On 2014/09/26 02:49:20, suchit.agrawal wrote: > > On 2014/09/25 10:17:10, a.suchit wrote: > > > mailto:a.suchit@samsung.com changed reviewers: > > > + mailto:leviw@chromium.org, mailto:rschoen@google.com > > > > Please review it, I need to just update the testExpectation file for mac and > > win. Thanks > > crbug.com/377518 also fixed with this patch. Please review this issue. Thanks
On 2014/09/29 17:35:34, a.suchit wrote: > On 2014/09/27 06:34:54, a.suchit wrote: > > On 2014/09/26 02:49:20, suchit.agrawal wrote: > > > On 2014/09/25 10:17:10, a.suchit wrote: > > > > mailto:a.suchit@samsung.com changed reviewers: > > > > + mailto:leviw@chromium.org, mailto:rschoen@google.com > > > > > > Please review it, I need to just update the testExpectation file for mac and > > > win. Thanks > > > > crbug.com/377518 also fixed with this patch. > > Please review this issue. Thanks Please review it. Thanks
This patch needs a lot more explanations for me to be comfortable with it. https://codereview.chromium.org/596823002/diff/140001/Source/core/rendering/R... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/596823002/diff/140001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:488: unsigned RenderTableSection::calcRowHeightHavingOnlySpanningCells(unsigned row, unsigned& extraHeightToPropagate, unsigned r1, int& accumulatedPositionIncrease, unsigned r2, Vector<int>& rowsCountWithOnlySpanningCells) r1 and r2 are not good variable names :( Also this function has 6 arguments, which is a bad smell (and the limit of craziness). To top it off, the only caller calls it with r2 == row. https://codereview.chromium.org/596823002/diff/140001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:510: unsigned spanningCellsRowsCountHavingZeroHeight = rowsCountWithOnlySpanningCells[endRow -1] - rowsCountWithOnlySpanningCells[max(rowIndex, row)]; max(rowIndex, row) is repeated 4 times, it seems like we should have a variable for it. It would be nice to give an explanation as to *why* this is the index we're looking for. https://codereview.chromium.org/596823002/diff/140001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:513: spanningCellsRowsCountHavingZeroHeight += rowsCountWithOnlySpanningCells[max(rowIndex, row)] - rowsCountWithOnlySpanningCells[max(rowIndex, row) - 1]; I am probably confused by this line but it seems deeply wrong to use a *table index* to access a spot into the sparse array rowsCountWithOnlySpanningCells. https://codereview.chromium.org/596823002/diff/140001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:515: spanningCellsRowsCountHavingZeroHeight += rowsCountWithOnlySpanningCells[0]; This else is suspicious, I don't see why some cells are different than others. https://codereview.chromium.org/596823002/diff/140001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:580: // At this stage, Height of the rows are zero those contains only spanning cells. Let's do correct English sentence: // At this stage, the height of the rows is zero for the one containing only spanning cells. https://codereview.chromium.org/596823002/diff/140001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:586: } You've added an unrelated O(N) loop here inside a function that can be called repeatedly during layout :(
https://codereview.chromium.org/596823002/diff/140001/Source/core/rendering/R... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/596823002/diff/140001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:488: unsigned RenderTableSection::calcRowHeightHavingOnlySpanningCells(unsigned row, unsigned& extraHeightToPropagate, unsigned r1, int& accumulatedPositionIncrease, unsigned r2, Vector<int>& rowsCountWithOnlySpanningCells) On 2014/10/08 00:16:08, Julien Chaffraix - PST wrote: > r1 and r2 are not good variable names :( > > Also this function has 6 arguments, which is a bad smell (and the limit of > craziness). > > To top it off, the only caller calls it with r2 == row. changed and name of r1 variable I did not observed about r2 == row. Thanks Now I removed it. Now, function has 5 argument, it is also more but I don't have any other option because I do not want to use structure just for it. https://codereview.chromium.org/596823002/diff/140001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:510: unsigned spanningCellsRowsCountHavingZeroHeight = rowsCountWithOnlySpanningCells[endRow -1] - rowsCountWithOnlySpanningCells[max(rowIndex, row)]; On 2014/10/08 00:16:08, Julien Chaffraix - PST wrote: > max(rowIndex, row) is repeated 4 times, it seems like we should have a variable > for it. > > It would be nice to give an explanation as to *why* this is the index we're > looking for. Done. https://codereview.chromium.org/596823002/diff/140001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:513: spanningCellsRowsCountHavingZeroHeight += rowsCountWithOnlySpanningCells[max(rowIndex, row)] - rowsCountWithOnlySpanningCells[max(rowIndex, row) - 1]; On 2014/10/08 00:16:08, Julien Chaffraix - PST wrote: > I am probably confused by this line but it seems deeply wrong to use a *table > index* to access a spot into the sparse array rowsCountWithOnlySpanningCells. Acknowledged. https://codereview.chromium.org/596823002/diff/140001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:515: spanningCellsRowsCountHavingZeroHeight += rowsCountWithOnlySpanningCells[0]; On 2014/10/08 00:16:08, Julien Chaffraix - PST wrote: > This else is suspicious, I don't see why some cells are different than others. Acknowledged. https://codereview.chromium.org/596823002/diff/140001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:580: // At this stage, Height of the rows are zero those contains only spanning cells. On 2014/10/08 00:16:08, Julien Chaffraix - PST wrote: > Let's do correct English sentence: > > // At this stage, the height of the rows is zero for the one containing only > spanning cells. Done. https://codereview.chromium.org/596823002/diff/140001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:586: } On 2014/10/08 00:16:08, Julien Chaffraix - PST wrote: > You've added an unrelated O(N) loop here inside a function that can be called > repeatedly during layout :( If it is not done here then repeatedly, need to check it in calcRowHeightHavingOnlySpanningCells(...) which is too expensive.
On 2014/10/08 11:30:08, a.suchit wrote: > https://codereview.chromium.org/596823002/diff/140001/Source/core/rendering/R... > File Source/core/rendering/RenderTableSection.cpp (right): > > https://codereview.chromium.org/596823002/diff/140001/Source/core/rendering/R... > Source/core/rendering/RenderTableSection.cpp:488: unsigned > RenderTableSection::calcRowHeightHavingOnlySpanningCells(unsigned row, unsigned& > extraHeightToPropagate, unsigned r1, int& accumulatedPositionIncrease, unsigned > r2, Vector<int>& rowsCountWithOnlySpanningCells) > On 2014/10/08 00:16:08, Julien Chaffraix - PST wrote: > > r1 and r2 are not good variable names :( > > > > Also this function has 6 arguments, which is a bad smell (and the limit of > > craziness). > > > > To top it off, the only caller calls it with r2 == row. > > changed and name of r1 variable > > I did not observed about r2 == row. Thanks > Now I removed it. > > Now, function has 5 argument, it is also more but I don't have any other option > because I do not want to use structure just for it. > > https://codereview.chromium.org/596823002/diff/140001/Source/core/rendering/R... > Source/core/rendering/RenderTableSection.cpp:510: unsigned > spanningCellsRowsCountHavingZeroHeight = rowsCountWithOnlySpanningCells[endRow > -1] - rowsCountWithOnlySpanningCells[max(rowIndex, row)]; > On 2014/10/08 00:16:08, Julien Chaffraix - PST wrote: > > max(rowIndex, row) is repeated 4 times, it seems like we should have a > variable > > for it. > > > > It would be nice to give an explanation as to *why* this is the index we're > > looking for. > > Done. > > https://codereview.chromium.org/596823002/diff/140001/Source/core/rendering/R... > Source/core/rendering/RenderTableSection.cpp:513: > spanningCellsRowsCountHavingZeroHeight += > rowsCountWithOnlySpanningCells[max(rowIndex, row)] - > rowsCountWithOnlySpanningCells[max(rowIndex, row) - 1]; > On 2014/10/08 00:16:08, Julien Chaffraix - PST wrote: > > I am probably confused by this line but it seems deeply wrong to use a *table > > index* to access a spot into the sparse array rowsCountWithOnlySpanningCells. > > Acknowledged. > > https://codereview.chromium.org/596823002/diff/140001/Source/core/rendering/R... > Source/core/rendering/RenderTableSection.cpp:515: > spanningCellsRowsCountHavingZeroHeight += rowsCountWithOnlySpanningCells[0]; > On 2014/10/08 00:16:08, Julien Chaffraix - PST wrote: > > This else is suspicious, I don't see why some cells are different than others. > > Acknowledged. > > https://codereview.chromium.org/596823002/diff/140001/Source/core/rendering/R... > Source/core/rendering/RenderTableSection.cpp:580: // At this stage, Height of > the rows are zero those contains only spanning cells. > On 2014/10/08 00:16:08, Julien Chaffraix - PST wrote: > > Let's do correct English sentence: > > > > // At this stage, the height of the rows is zero for the one containing only > > spanning cells. > > Done. > > https://codereview.chromium.org/596823002/diff/140001/Source/core/rendering/R... > Source/core/rendering/RenderTableSection.cpp:586: } > On 2014/10/08 00:16:08, Julien Chaffraix - PST wrote: > > You've added an unrelated O(N) loop here inside a function that can be called > > repeatedly during layout :( > > If it is not done here then repeatedly, need to check it in > calcRowHeightHavingOnlySpanningCells(...) which is too expensive. Please review it thanks
dsinclair@chromium.org changed reviewers: + dsinclair@chromium.org
https://codereview.chromium.org/596823002/diff/160001/LayoutTests/fast/table/... File LayoutTests/fast/table/table-rowspan-wrong-height-with-only-spanning-cells.html (right): https://codereview.chromium.org/596823002/diff/160001/LayoutTests/fast/table/... LayoutTests/fast/table/table-rowspan-wrong-height-with-only-spanning-cells.html:1: <html> Missing <!DOCTYPE html> https://codereview.chromium.org/596823002/diff/160001/LayoutTests/fast/table/... LayoutTests/fast/table/table-rowspan-wrong-height-with-only-spanning-cells.html:2: <head> Don't need html, head or body nodes. https://codereview.chromium.org/596823002/diff/160001/LayoutTests/fast/table/... LayoutTests/fast/table/table-rowspan-wrong-height-with-only-spanning-cells.html:11: <p>The middle tr, which has been merged over by two cells, should have a height of 1px, the difference in heights between cells that span one row and two rows.</p> This description is difficult to understand. How about something like: We have the below table. Because cell A and cell D overlap in the second row, the height of the second row is the 1px difference between A and B and the 1px difference between C and D. |-----|-----| | A | B | | |-----| | | | |-----| | | C | D | |-----|-----| https://codereview.chromium.org/596823002/diff/160001/LayoutTests/platform/li... File LayoutTests/platform/linux/fast/table/giantRowspan2-expected.txt (left): https://codereview.chromium.org/596823002/diff/160001/LayoutTests/platform/li... LayoutTests/platform/linux/fast/table/giantRowspan2-expected.txt:8: RenderTableRow {TR} at (0,2) size 784x0 Where does the 1px come from? https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:488: unsigned RenderTableSection::calcRowHeightHavingOnlySpanningCells(unsigned row, int& accumulatedPositionIncrease, unsigned effectedRowByExtraHeight, unsigned& extraHeightToPropagate, Vector<int>& rowsCountWithOnlySpanningCells) How does accumulatedPositionIncrease relate to extraHeightToPropagate? https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:488: unsigned RenderTableSection::calcRowHeightHavingOnlySpanningCells(unsigned row, int& accumulatedPositionIncrease, unsigned effectedRowByExtraHeight, unsigned& extraHeightToPropagate, Vector<int>& rowsCountWithOnlySpanningCells) What does effectedRowByExtraHeight mean? Does it mean the row was effected by extra height? Or, is it the height that effected the row? https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:488: unsigned RenderTableSection::calcRowHeightHavingOnlySpanningCells(unsigned row, int& accumulatedPositionIncrease, unsigned effectedRowByExtraHeight, unsigned& extraHeightToPropagate, Vector<int>& rowsCountWithOnlySpanningCells) s/rowsCountWithOnlySpanningCells/rowCountWithOnlySpanningCells https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:502: if (rowSpanCell.cells.size()) { if (!rowSpanCell.cells.size()) continue; https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:505: if (cell->rowSpan() > 1) { if (cell.rowSpan() <= 1) continue; https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:507: const unsigned rowSpan = cell->rowSpan(); Can we rename these to cellRowIndex and cellRowSpan to make it obvious why they are different from row above? https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:510: // and all previous rows are already processed so we need to find the number of rows with only This comment is difficult to process, does the below make sense? // As we are going from the top of the table to the bottom to calculate the row // heights for rows that only contain spanning cells and all previous rows are // processed we only need to find the number of rows with spanning cells from the // current cell to the end of the current cells spanning height. https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:513: unsigned endRow = rowIndex + rowSpan; Does this mean the endRow of our current cell, or the row that comes after the ending row of our cell? https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:517: spanningCellsRowsCountHavingZeroHeight = rowsCountWithOnlySpanningCells[endRow - 1] - rowsCountWithOnlySpanningCells[startRow - 1]; Why is this startRow - 1 and not startRow? https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:519: spanningCellsRowsCountHavingZeroHeight = (rowsCountWithOnlySpanningCells[endRow - 1] - rowsCountWithOnlySpanningCells[startRow]) + rowsCountWithOnlySpanningCells[0]; If I'm reading this correctly we're doing: spanning = count[endRow - 1] - count[0] + count[0]; Is that correct? We'd only get in here if startRow is 0. So we subtract and then add the value for 0? https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:522: int totalRowspanCellHeight = m_rowPos[endRow] - m_rowPos[rowIndex]; It's very confusing that we jump back and forth between startRow and rowIndex for what we're starting with. Can we fixup these names so it's clearer what's going on? https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:537: rowHeight = std::max(rowHeight, extraHeightRequired / spanningCellsRowsCountHavingZeroHeight); Can we ever have a case where spanningCellsRowsCountHavingZeroHeight is 0? Should we have an ASSERT to that effect? https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:537: rowHeight = std::max(rowHeight, extraHeightRequired / spanningCellsRowsCountHavingZeroHeight); Wont' this fail to set rowHeight in the case where it's less then logicalHeightForRowSizing? So, isn't it now possible for us to return a 0 rowHeight? https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:559: spanningRowsHeight.rowHeight[row] = calcRowHeightHavingOnlySpanningCells(actualRow, accumulatedPositionIncrease, rowIndex + rowSpan, extraHeightToPropagate, rowsCountWithOnlySpanningCells); We set effectedRowByExtraHeight to rowIndex + rowSpan. So, it's the next row, after the spanning row that gets the extra height? If I'm reading the above correctly, this row won't necessarily get the extra height. It only changes if it's within the index + span of a given cell? https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:584: // At this stage, Height of the rows are zero for the one containing only spanning cells. This comment doesn't make sense to me, can you re-write it? https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:589: rowsCountWithOnlySpanningCells.append(count); I'm not sure I understand what this is storing. We have a vector of ints. The count always increases, so if each row only contains spanning cells the vector will contain: 1,2,3,4,5,6? Or, if only a couple of them do we'll have something like: 1,1,3,3,5,5? https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... File Source/core/rendering/RenderTableSection.h (right): https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.h:263: unsigned calcRowHeightHavingOnlySpanningCells(unsigned, int&, unsigned, unsigned&, Vector<int>&); I believe, in this case where it's ambiguous, it's OK to put the parameter names in the header.
pdr@chromium.org changed reviewers: - pdr@chromium.org
suchit.agrawal@gmail.com changed reviewers: + suchit.agrawal@gmail.com
I updated my comments on your review comments. Please check. https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:488: unsigned RenderTableSection::calcRowHeightHavingOnlySpanningCells(unsigned row, int& accumulatedPositionIncrease, unsigned effectedRowByExtraHeight, unsigned& extraHeightToPropagate, Vector<int>& rowsCountWithOnlySpanningCells) On 2014/10/15 21:37:42, dsinclair wrote: > How does accumulatedPositionIncrease relate to extraHeightToPropagate? There is no relation between these 2. extraHeightToPropagate is the extra height applied till end of current spanning cell in distributeRowSpanHeightToRows(). This extra height is the total additional height calculated in all rows (all rows before current spanning cell) of the table. accumulatedPositionIncrease is the total additional height calculated in previous rows of current spanning cell in updateRowsHeightHavingOnlySpanningCells(). https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:513: unsigned endRow = rowIndex + rowSpan; On 2014/10/15 21:37:42, dsinclair wrote: > Does this mean the endRow of our current cell, or the row that comes after the > ending row of our cell? we can see it in 2 ways, 1. It is the row that comes after the ending row of our cell. 2. It is the end border of the ending row of our cell. https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:517: spanningCellsRowsCountHavingZeroHeight = rowsCountWithOnlySpanningCells[endRow - 1] - rowsCountWithOnlySpanningCells[startRow - 1]; On 2014/10/15 21:37:42, dsinclair wrote: > Why is this startRow - 1 and not startRow? 000112223334 ^ ^ 4-3=1. Means 1 row with only spanning cell. But actually it have 2 (fist row and last row). So 4-2=2 https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:519: spanningCellsRowsCountHavingZeroHeight = (rowsCountWithOnlySpanningCells[endRow - 1] - rowsCountWithOnlySpanningCells[startRow]) + rowsCountWithOnlySpanningCells[0]; On 2014/10/15 21:37:41, dsinclair wrote: > If I'm reading this correctly we're doing: > > spanning = count[endRow - 1] - count[0] + count[0]; > > Is that correct? We'd only get in here if startRow is 0. So we subtract and then > add the value for 0? yes, we can remove subtraction and addition. https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:522: int totalRowspanCellHeight = m_rowPos[endRow] - m_rowPos[rowIndex]; On 2014/10/15 21:37:41, dsinclair wrote: > It's very confusing that we jump back and forth between startRow and rowIndex > for what we're starting with. Can we fixup these names so it's clearer what's > going on? I will change |startRow| to |startRowforSpanningCellCount|. https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:537: rowHeight = std::max(rowHeight, extraHeightRequired / spanningCellsRowsCountHavingZeroHeight); On 2014/10/15 21:37:41, dsinclair wrote: > Wont' this fail to set rowHeight in the case where it's less then > logicalHeightForRowSizing? So, isn't it now possible for us to return a 0 > rowHeight? > Wont' this fail to set rowHeight in the case where it's less then > logicalHeightForRowSizing? rowHeight would be less than logicalHeightForRowSizing because logicalHeightForRowSizing is the total height of spanning rows. > isn't it now possible for us to return a 0? It may return 0 or non-zero. It is depend on extraHeightRequired. https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:537: rowHeight = std::max(rowHeight, extraHeightRequired / spanningCellsRowsCountHavingZeroHeight); On 2014/10/15 21:37:41, dsinclair wrote: > Can we ever have a case where spanningCellsRowsCountHavingZeroHeight is 0? > Should we have an ASSERT to that effect? spanningCellsRowsCountHavingZeroHeight would not be 0 because this fuction calls only when current row have only spanning cells. https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:559: spanningRowsHeight.rowHeight[row] = calcRowHeightHavingOnlySpanningCells(actualRow, accumulatedPositionIncrease, rowIndex + rowSpan, extraHeightToPropagate, rowsCountWithOnlySpanningCells); On 2014/10/15 21:37:41, dsinclair wrote: > We set effectedRowByExtraHeight to rowIndex + rowSpan. So, it's the next row, > after the spanning row that gets the extra height? > > If I'm reading the above correctly, this row won't necessarily get the extra > height. It only changes if it's within the index + span of a given cell? In calcRowHeightHavingOnlySpanningCells(), We may go beyond (index + span) because current row have many other spanning cells and those spanning cells may start before current cell index and/or end after current cell (index + span). https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:584: // At this stage, Height of the rows are zero for the one containing only spanning cells. On 2014/10/15 21:37:42, dsinclair wrote: > This comment doesn't make sense to me, can you re-write it? We are collecting the information of the rows with only spanning cells because these row heights are 0 and we may required to give some height to these rows. So there is comment indicating that we are collecting information for all the rows with only spanning cells because all these rows have 0 height. https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:589: rowsCountWithOnlySpanningCells.append(count); On 2014/10/15 21:37:41, dsinclair wrote: > I'm not sure I understand what this is storing. We have a vector of ints. The > count always increases, so if each row only contains spanning cells the vector > will contain: 1,2,3,4,5,6? > > Or, if only a couple of them do we'll have something like: 1,1,3,3,5,5? yes, we are doing same here.
I updated my comments on your review comments. Please check.
On 2014/10/31 23:14:24, suchit.agrawal wrote: > I updated my comments on your review comments. Please check. Sorry, I was waiting for a new patch to be uploaded. I'll take a look at your comments on Monday.
https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:488: unsigned RenderTableSection::calcRowHeightHavingOnlySpanningCells(unsigned row, int& accumulatedPositionIncrease, unsigned effectedRowByExtraHeight, unsigned& extraHeightToPropagate, Vector<int>& rowsCountWithOnlySpanningCells) On 2014/10/25 04:28:59, suchit.agrawal wrote: > On 2014/10/15 21:37:42, dsinclair wrote: > > How does accumulatedPositionIncrease relate to extraHeightToPropagate? > > There is no relation between these 2. > > extraHeightToPropagate is the extra height applied till end of current spanning > cell in distributeRowSpanHeightToRows(). This extra height is the total > additional height calculated in all rows (all rows before current spanning cell) > of the table. > > accumulatedPositionIncrease is the total additional height calculated in > previous rows of current spanning cell in > updateRowsHeightHavingOnlySpanningCells(). So, if I'm understanding correctly, accumulatedPositionIncrease is only related to the current spanning cells and extraHeightToPropagate is related to the entire table. Yes?
https://codereview.chromium.org/596823002/diff/160001/LayoutTests/fast/table/... File LayoutTests/fast/table/table-rowspan-wrong-height-with-only-spanning-cells.html (right): https://codereview.chromium.org/596823002/diff/160001/LayoutTests/fast/table/... LayoutTests/fast/table/table-rowspan-wrong-height-with-only-spanning-cells.html:1: <html> On 2014/10/15 21:37:41, dsinclair wrote: > Missing <!DOCTYPE html> With using DOCTYPE, Test case is behaving different. So DOCTYPE is not used here. https://codereview.chromium.org/596823002/diff/160001/LayoutTests/fast/table/... LayoutTests/fast/table/table-rowspan-wrong-height-with-only-spanning-cells.html:2: <head> On 2014/10/15 21:37:41, dsinclair wrote: > Don't need html, head or body nodes. Removed HTML and HEAD node. BODY node is needed for checkLayout() on onload. https://codereview.chromium.org/596823002/diff/160001/LayoutTests/fast/table/... LayoutTests/fast/table/table-rowspan-wrong-height-with-only-spanning-cells.html:11: <p>The middle tr, which has been merged over by two cells, should have a height of 1px, the difference in heights between cells that span one row and two rows.</p> On 2014/10/15 21:37:41, dsinclair wrote: > This description is difficult to understand. How about something like: > > We have the below table. Because cell A and cell D overlap in the second row, > the height of the > second row is the 1px difference between A and B and the 1px difference between > C and D. > |-----|-----| > | A | B | > | |-----| > | | | > |-----| | > | C | D | > |-----|-----| Done. https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:488: unsigned RenderTableSection::calcRowHeightHavingOnlySpanningCells(unsigned row, int& accumulatedPositionIncrease, unsigned effectedRowByExtraHeight, unsigned& extraHeightToPropagate, Vector<int>& rowsCountWithOnlySpanningCells) On 2014/11/05 17:03:20, dsinclair wrote: > On 2014/10/25 04:28:59, suchit.agrawal wrote: > > On 2014/10/15 21:37:42, dsinclair wrote: > > > How does accumulatedPositionIncrease relate to extraHeightToPropagate? > > > > There is no relation between these 2. > > > > extraHeightToPropagate is the extra height applied till end of current > spanning > > cell in distributeRowSpanHeightToRows(). This extra height is the total > > additional height calculated in all rows (all rows before current spanning > cell) > > of the table. > > > > accumulatedPositionIncrease is the total additional height calculated in > > previous rows of current spanning cell in > > updateRowsHeightHavingOnlySpanningCells(). > > > So, if I'm understanding correctly, accumulatedPositionIncrease is only related > to the current spanning cells and extraHeightToPropagate is related to the > entire table. Yes? right. https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:502: if (rowSpanCell.cells.size()) { On 2014/10/15 21:37:41, dsinclair wrote: > if (!rowSpanCell.cells.size()) > continue; Done. https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:505: if (cell->rowSpan() > 1) { On 2014/10/15 21:37:42, dsinclair wrote: > if (cell.rowSpan() <= 1) > continue; Done. https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:507: const unsigned rowSpan = cell->rowSpan(); On 2014/10/15 21:37:42, dsinclair wrote: > Can we rename these to cellRowIndex and cellRowSpan to make it obvious why they > are different from row above? Done. https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:510: // and all previous rows are already processed so we need to find the number of rows with only On 2014/10/15 21:37:41, dsinclair wrote: > This comment is difficult to process, does the below make sense? > > // As we are going from the top of the table to the bottom to calculate the row > // heights for rows that only contain spanning cells and all previous rows are > // processed we only need to find the number of rows with spanning cells from > the > // current cell to the end of the current cells spanning height. Done. https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:519: spanningCellsRowsCountHavingZeroHeight = (rowsCountWithOnlySpanningCells[endRow - 1] - rowsCountWithOnlySpanningCells[startRow]) + rowsCountWithOnlySpanningCells[0]; On 2014/10/15 21:37:41, dsinclair wrote: > If I'm reading this correctly we're doing: > > spanning = count[endRow - 1] - count[0] + count[0]; > > Is that correct? We'd only get in here if startRow is 0. So we subtract and then > add the value for 0? Done. https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:522: int totalRowspanCellHeight = m_rowPos[endRow] - m_rowPos[rowIndex]; On 2014/10/15 21:37:41, dsinclair wrote: > It's very confusing that we jump back and forth between startRow and rowIndex > for what we're starting with. Can we fixup these names so it's clearer what's > going on? Done.
On 2014/12/03 13:09:14, a.suchit wrote: > https://codereview.chromium.org/596823002/diff/160001/LayoutTests/fast/table/... > File > LayoutTests/fast/table/table-rowspan-wrong-height-with-only-spanning-cells.html > (right): > > https://codereview.chromium.org/596823002/diff/160001/LayoutTests/fast/table/... > LayoutTests/fast/table/table-rowspan-wrong-height-with-only-spanning-cells.html:1: > <html> > On 2014/10/15 21:37:41, dsinclair wrote: > > Missing <!DOCTYPE html> > > With using DOCTYPE, Test case is behaving different. So DOCTYPE is not used > here. > > https://codereview.chromium.org/596823002/diff/160001/LayoutTests/fast/table/... > LayoutTests/fast/table/table-rowspan-wrong-height-with-only-spanning-cells.html:2: > <head> > On 2014/10/15 21:37:41, dsinclair wrote: > > Don't need html, head or body nodes. > > Removed HTML and HEAD node. > BODY node is needed for checkLayout() on onload. > > https://codereview.chromium.org/596823002/diff/160001/LayoutTests/fast/table/... > LayoutTests/fast/table/table-rowspan-wrong-height-with-only-spanning-cells.html:11: > <p>The middle tr, which has been merged over by two cells, should have a height > of 1px, the difference in heights between cells that span one row and two > rows.</p> > On 2014/10/15 21:37:41, dsinclair wrote: > > This description is difficult to understand. How about something like: > > > > We have the below table. Because cell A and cell D overlap in the second row, > > the height of the > > second row is the 1px difference between A and B and the 1px difference > between > > C and D. > > |-----|-----| > > | A | B | > > | |-----| > > | | | > > |-----| | > > | C | D | > > |-----|-----| > > Done. > > https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... > File Source/core/rendering/RenderTableSection.cpp (right): > > https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... > Source/core/rendering/RenderTableSection.cpp:488: unsigned > RenderTableSection::calcRowHeightHavingOnlySpanningCells(unsigned row, int& > accumulatedPositionIncrease, unsigned effectedRowByExtraHeight, unsigned& > extraHeightToPropagate, Vector<int>& rowsCountWithOnlySpanningCells) > On 2014/11/05 17:03:20, dsinclair wrote: > > On 2014/10/25 04:28:59, suchit.agrawal wrote: > > > On 2014/10/15 21:37:42, dsinclair wrote: > > > > How does accumulatedPositionIncrease relate to extraHeightToPropagate? > > > > > > There is no relation between these 2. > > > > > > extraHeightToPropagate is the extra height applied till end of current > > spanning > > > cell in distributeRowSpanHeightToRows(). This extra height is the total > > > additional height calculated in all rows (all rows before current spanning > > cell) > > > of the table. > > > > > > accumulatedPositionIncrease is the total additional height calculated in > > > previous rows of current spanning cell in > > > updateRowsHeightHavingOnlySpanningCells(). > > > > > > So, if I'm understanding correctly, accumulatedPositionIncrease is only > related > > to the current spanning cells and extraHeightToPropagate is related to the > > entire table. Yes? > > right. > > https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... > Source/core/rendering/RenderTableSection.cpp:502: if (rowSpanCell.cells.size()) > { > On 2014/10/15 21:37:41, dsinclair wrote: > > if (!rowSpanCell.cells.size()) > > continue; > > Done. > > https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... > Source/core/rendering/RenderTableSection.cpp:505: if (cell->rowSpan() > 1) { > On 2014/10/15 21:37:42, dsinclair wrote: > > if (cell.rowSpan() <= 1) > > continue; > > Done. > > https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... > Source/core/rendering/RenderTableSection.cpp:507: const unsigned rowSpan = > cell->rowSpan(); > On 2014/10/15 21:37:42, dsinclair wrote: > > Can we rename these to cellRowIndex and cellRowSpan to make it obvious why > they > > are different from row above? > > Done. > > https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... > Source/core/rendering/RenderTableSection.cpp:510: // and all previous rows are > already processed so we need to find the number of rows with only > On 2014/10/15 21:37:41, dsinclair wrote: > > This comment is difficult to process, does the below make sense? > > > > // As we are going from the top of the table to the bottom to calculate the > row > > // heights for rows that only contain spanning cells and all previous rows are > > // processed we only need to find the number of rows with spanning cells from > > the > > // current cell to the end of the current cells spanning height. > > Done. > > https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... > Source/core/rendering/RenderTableSection.cpp:519: > spanningCellsRowsCountHavingZeroHeight = (rowsCountWithOnlySpanningCells[endRow > - 1] - rowsCountWithOnlySpanningCells[startRow]) + > rowsCountWithOnlySpanningCells[0]; > On 2014/10/15 21:37:41, dsinclair wrote: > > If I'm reading this correctly we're doing: > > > > spanning = count[endRow - 1] - count[0] + count[0]; > > > > Is that correct? We'd only get in here if startRow is 0. So we subtract and > then > > add the value for 0? > > Done. > > https://codereview.chromium.org/596823002/diff/160001/Source/core/rendering/R... > Source/core/rendering/RenderTableSection.cpp:522: int totalRowspanCellHeight = > m_rowPos[endRow] - m_rowPos[rowIndex]; > On 2014/10/15 21:37:41, dsinclair wrote: > > It's very confusing that we jump back and forth between startRow and rowIndex > > for what we're starting with. Can we fixup these names so it's clearer what's > > going on? > > Done. Sorry for big delay.
Can you remove the uploaded .png's and run the layout tests so we can visually see the differences? It's hard to tell what changed just from the side by side comparisons. https://codereview.chromium.org/596823002/diff/180001/LayoutTests/TestExpecta... File LayoutTests/TestExpectations (right): https://codereview.chromium.org/596823002/diff/180001/LayoutTests/TestExpecta... LayoutTests/TestExpectations:1409: crbug.com/434090 inspector/tracing/worker-js-profile.html [ Pass Failure ] What are these entries for? https://codereview.chromium.org/596823002/diff/180001/LayoutTests/TestExpecta... LayoutTests/TestExpectations:1413: #Below test case need to rebaseline after crbug.com/396655 Comment isn't needed, it's covered by the crbug.com/396655 entry on each line. https://codereview.chromium.org/596823002/diff/180001/LayoutTests/platform/li... File LayoutTests/platform/linux/fast/table/giantRowspan2-expected.txt (right): https://codereview.chromium.org/596823002/diff/180001/LayoutTests/platform/li... LayoutTests/platform/linux/fast/table/giantRowspan2-expected.txt:8: RenderTableRow {TR} at (0,2) size 784x1 Do we know why this increased in size? https://codereview.chromium.org/596823002/diff/180001/LayoutTests/platform/li... File LayoutTests/platform/linux/tables/mozilla/bugs/bug7714-expected.txt (right): https://codereview.chromium.org/596823002/diff/180001/LayoutTests/platform/li... LayoutTests/platform/linux/tables/mozilla/bugs/bug7714-expected.txt:44: RenderTable {TABLE} at (0,274) size 37x152 [border: (1px outset #808080)] Do we know why the table got smaller? https://codereview.chromium.org/596823002/diff/180001/Source/core/rendering/R... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/596823002/diff/180001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:487: unsigned RenderTableSection::calcRowHeightHavingOnlySpanningCells(unsigned row, int& accumulatedPositionIncrease, unsigned effectedRowByExtraHeight, unsigned& extraHeightToPropagate, Vector<int>& rowsCountWithOnlySpanningCells) To clarify this, can we rename extraHeightToPropgate to extraTableHeightToPropgate and accumulatedPositionIncrease to accumulatedCellPositionIncrease? https://codereview.chromium.org/596823002/diff/180001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:487: unsigned RenderTableSection::calcRowHeightHavingOnlySpanningCells(unsigned row, int& accumulatedPositionIncrease, unsigned effectedRowByExtraHeight, unsigned& extraHeightToPropagate, Vector<int>& rowsCountWithOnlySpanningCells) If I'm reading this correctly effectedRowByExtraHeight is the index of the row which will receive the height? I think a better name would be rowToApplyExtraHeight https://codereview.chromium.org/596823002/diff/180001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:524: } I think this can be simplified too: unsigned spanningCellsRowsCountHavingZeroHeight = rowsCountWithOnlySpanningCells[endRow - 1]; if (startRowForSpanningCellCount) spanningCellsRowsCountHavingZeroHeight -= rowsCountWithOnlySpanningCells[startRowForSpanningCellCount - 1]; https://codereview.chromium.org/596823002/diff/180001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:526: int totalRowspanCellHeight = m_rowPos[endRow] - m_rowPos[cellRowIndex]; Is totalRowspanCellHeight the total height that is available for the cell? If that's the case, I think neededCellHeight might be a better name. https://codereview.chromium.org/596823002/diff/180001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:530: totalRowspanCellHeight += accumulatedPositionIncrease; Why not do these three calculations at the same time? https://codereview.chromium.org/596823002/diff/180001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:535: unsigned extraHeightRequired = rowSpanCell.cells[0]->logicalHeightForRowSizing() - totalRowspanCellHeight; We use cell above and rowSpanCell.cells[0] here, can you make this one cell so it's consistent? https://codereview.chromium.org/596823002/diff/180001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:539: extraHeightRequired += spanningCellsRowsCountHavingZeroHeight - remainder; Why do rows having zero height effect the height required for our current row?
Patchset #13 (id:240001) has been deleted
https://codereview.chromium.org/596823002/diff/180001/LayoutTests/TestExpecta... File LayoutTests/TestExpectations (right): https://codereview.chromium.org/596823002/diff/180001/LayoutTests/TestExpecta... LayoutTests/TestExpectations:1409: crbug.com/434090 inspector/tracing/worker-js-profile.html [ Pass Failure ] On 2014/12/03 19:25:22, dsinclair wrote: > What are these entries for? This happened because of rebase. Removed now. https://codereview.chromium.org/596823002/diff/180001/LayoutTests/TestExpecta... LayoutTests/TestExpectations:1413: #Below test case need to rebaseline after crbug.com/396655 On 2014/12/03 19:25:22, dsinclair wrote: > Comment isn't needed, it's covered by the crbug.com/396655 entry on each line. Done. https://codereview.chromium.org/596823002/diff/180001/LayoutTests/platform/li... File LayoutTests/platform/linux/fast/table/giantRowspan2-expected.txt (right): https://codereview.chromium.org/596823002/diff/180001/LayoutTests/platform/li... LayoutTests/platform/linux/fast/table/giantRowspan2-expected.txt:8: RenderTableRow {TR} at (0,2) size 784x1 On 2014/12/03 19:25:22, dsinclair wrote: > Do we know why this increased in size? Fixed https://codereview.chromium.org/596823002/diff/180001/Source/core/rendering/R... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/596823002/diff/180001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:487: unsigned RenderTableSection::calcRowHeightHavingOnlySpanningCells(unsigned row, int& accumulatedPositionIncrease, unsigned effectedRowByExtraHeight, unsigned& extraHeightToPropagate, Vector<int>& rowsCountWithOnlySpanningCells) On 2014/12/03 19:25:23, dsinclair wrote: > If I'm reading this correctly effectedRowByExtraHeight is the index of the row > which will receive the height? > > I think a better name would be rowToApplyExtraHeight Done. https://codereview.chromium.org/596823002/diff/180001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:487: unsigned RenderTableSection::calcRowHeightHavingOnlySpanningCells(unsigned row, int& accumulatedPositionIncrease, unsigned effectedRowByExtraHeight, unsigned& extraHeightToPropagate, Vector<int>& rowsCountWithOnlySpanningCells) On 2014/12/03 19:25:23, dsinclair wrote: > To clarify this, can we rename extraHeightToPropgate to > extraTableHeightToPropgate and accumulatedPositionIncrease to > accumulatedCellPositionIncrease? Done. https://codereview.chromium.org/596823002/diff/180001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:524: } On 2014/12/03 19:25:22, dsinclair wrote: > I think this can be simplified too: > > unsigned spanningCellsRowsCountHavingZeroHeight = > rowsCountWithOnlySpanningCells[endRow - 1]; > if (startRowForSpanningCellCount) > spanningCellsRowsCountHavingZeroHeight -= > rowsCountWithOnlySpanningCells[startRowForSpanningCellCount - 1]; Done. https://codereview.chromium.org/596823002/diff/180001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:526: int totalRowspanCellHeight = m_rowPos[endRow] - m_rowPos[cellRowIndex]; On 2014/12/03 19:25:23, dsinclair wrote: > Is totalRowspanCellHeight the total height that is available for the cell? > > If that's the case, I think neededCellHeight might be a better name. This is current rowspan cells height. https://codereview.chromium.org/596823002/diff/180001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:530: totalRowspanCellHeight += accumulatedPositionIncrease; On 2014/12/03 19:25:23, dsinclair wrote: > Why not do these three calculations at the same time? Readability and clarification. https://codereview.chromium.org/596823002/diff/180001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:530: totalRowspanCellHeight += accumulatedPositionIncrease; On 2014/12/03 19:25:23, dsinclair wrote: > Why not do these three calculations at the same time? Done. https://codereview.chromium.org/596823002/diff/180001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:535: unsigned extraHeightRequired = rowSpanCell.cells[0]->logicalHeightForRowSizing() - totalRowspanCellHeight; On 2014/12/03 19:25:23, dsinclair wrote: > We use cell above and rowSpanCell.cells[0] here, can you make this one cell so > it's consistent? Done. https://codereview.chromium.org/596823002/diff/180001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:539: extraHeightRequired += spanningCellsRowsCountHavingZeroHeight - remainder; On 2014/12/03 19:25:22, dsinclair wrote: > Why do rows having zero height effect the height required for our current row? Just assume a example here : spanning cell have 7 rows with only spanning cells and extra height required here is 11. If 11 is distributed here without considering the remainder then it would like below : 1st row height = 11 / 7 = 1 2nd row height = 10 / 6 = 1 3rd row height = 9 / 5 = 1 4th row height8 / 4 = 2 5th row height6 / 3 = 2 6th row height4 / 2 = 2 7th row height2 / 1 = 2 current schenario : 1st row height = 11 / 7 => 14 / 7 = 2 2nd row height = 9 / 6 => 12 / 6 = 2 3rd row height = 7 / 5 => 10 / 5 = 2 4th row height = 5 / 4 => 8 / 4 = 2 5th row height = 3 / 3 = 1 6th row height = 2 / 2 = 1 7th row height = 1 / 1 = 1 Remainder part, we can remove it here.
dsinclair@chromium.org changed reviewers: - eseidel@chromium.org, rschoen@google.com
Can you please remove the modified expectation files and put the entries in TestExpectations. Can you also do a try bot run without the test expectations so we can see the layout test results of what the expectation changes will be? https://codereview.chromium.org/596823002/diff/260001/LayoutTests/platform/li... File LayoutTests/platform/linux/tables/mozilla/bugs/bug133756-2-expected.txt (right): https://codereview.chromium.org/596823002/diff/260001/LayoutTests/platform/li... LayoutTests/platform/linux/tables/mozilla/bugs/bug133756-2-expected.txt:3: layer at (0,0) size 800x464 Are these pixel differences expected?
On 2014/12/10 14:07:47, dsinclair wrote: > Can you also do a try bot run without the test expectations so we can see the > layout test results of what the expectation changes will be? > You can check the patch set 11 (Temp)
On 2014/12/10 15:54:15, a.suchit2 wrote: > On 2014/12/10 14:07:47, dsinclair wrote: > > Can you also do a try bot run without the test expectations so we can see the > > layout test results of what the expectation changes will be? > > > You can check the patch set 11 (Temp) And, is it expected that all of those tables get shorter? What causes the height difference?
On 2014/12/10 15:56:38, dsinclair wrote: > On 2014/12/10 15:54:15, a.suchit2 wrote: > > On 2014/12/10 14:07:47, dsinclair wrote: > > > Can you also do a try bot run without the test expectations so we can see > the > > > layout test results of what the expectation changes will be? > > > > > You can check the patch set 11 (Temp) > > > And, is it expected that all of those tables get shorter? What causes the height > difference? Yes, it is expected in change of the table height because algorithm is different to apply the heights to rows those have only spanning cells.
a.suchit@chromium.org changed reviewers: + a.suchit@chromium.org
https://codereview.chromium.org/596823002/diff/260001/LayoutTests/platform/li... File LayoutTests/platform/linux/tables/mozilla/bugs/bug133756-2-expected.txt (left): https://codereview.chromium.org/596823002/diff/260001/LayoutTests/platform/li... LayoutTests/platform/linux/tables/mozilla/bugs/bug133756-2-expected.txt:9: RenderTable {TABLE} at (0,20) size 64x24 [border: (1px outset #808080)] Here is table height is 24. https://codereview.chromium.org/596823002/diff/260001/LayoutTests/platform/li... LayoutTests/platform/linux/tables/mozilla/bugs/bug133756-2-expected.txt:19: RenderTable {TABLE} at (0,64) size 66x27 [border: (1px outset #808080)] Here is cellspacing 1 so table height should be increased by 2 but it is increased by 3. https://codereview.chromium.org/596823002/diff/260001/LayoutTests/platform/li... LayoutTests/platform/linux/tables/mozilla/bugs/bug133756-2-expected.txt:29: RenderTable {TABLE} at (0,111) size 66x26 [border: (1px outset #808080)] Here is cellspacing 1 so table height should be increased by 2 and here it is right. https://codereview.chromium.org/596823002/diff/260001/LayoutTests/platform/li... LayoutTests/platform/linux/tables/mozilla/bugs/bug133756-2-expected.txt:39: RenderTable {TABLE} at (0,157) size 68x29 [border: (1px outset #808080)] Here is cellspacing 1 so table height should be increased by 4 but it is increased by 5. https://codereview.chromium.org/596823002/diff/260001/LayoutTests/platform/li... File LayoutTests/platform/linux/tables/mozilla/bugs/bug133756-2-expected.txt (right): https://codereview.chromium.org/596823002/diff/260001/LayoutTests/platform/li... LayoutTests/platform/linux/tables/mozilla/bugs/bug133756-2-expected.txt:3: layer at (0,0) size 800x464 On 2014/12/10 14:07:47, dsinclair wrote: > Are these pixel differences expected? Please check comments on table heights and based on that this layer height is reasonable which is decreased by 4 pixels. https://codereview.chromium.org/596823002/diff/260001/LayoutTests/platform/li... LayoutTests/platform/linux/tables/mozilla/bugs/bug133756-2-expected.txt:9: RenderTable {TABLE} at (0,20) size 64x24 [border: (1px outset #808080)] Here is table height is 24. https://codereview.chromium.org/596823002/diff/260001/LayoutTests/platform/li... LayoutTests/platform/linux/tables/mozilla/bugs/bug133756-2-expected.txt:19: RenderTable {TABLE} at (0,64) size 66x26 [border: (1px outset #808080)] Here is cellspacing 1 so table height is increased by 2. https://codereview.chromium.org/596823002/diff/260001/LayoutTests/platform/li... LayoutTests/platform/linux/tables/mozilla/bugs/bug133756-2-expected.txt:29: RenderTable {TABLE} at (0,110) size 66x26 [border: (1px outset #808080)] Here is cellpading 1 so table height is increased by 2. https://codereview.chromium.org/596823002/diff/260001/LayoutTests/platform/li... LayoutTests/platform/linux/tables/mozilla/bugs/bug133756-2-expected.txt:39: RenderTable {TABLE} at (0,156) size 68x28 [border: (1px outset #808080)] Here is cellspacing 1 and cellpadding 1 so table height is increased by 4.
On 2014/12/11 02:14:43, a.suchit2 wrote: > https://codereview.chromium.org/596823002/diff/260001/LayoutTests/platform/li... > File LayoutTests/platform/linux/tables/mozilla/bugs/bug133756-2-expected.txt > (left): > > https://codereview.chromium.org/596823002/diff/260001/LayoutTests/platform/li... > LayoutTests/platform/linux/tables/mozilla/bugs/bug133756-2-expected.txt:9: > RenderTable {TABLE} at (0,20) size 64x24 [border: (1px outset #808080)] > Here is table height is 24. > > https://codereview.chromium.org/596823002/diff/260001/LayoutTests/platform/li... > LayoutTests/platform/linux/tables/mozilla/bugs/bug133756-2-expected.txt:19: > RenderTable {TABLE} at (0,64) size 66x27 [border: (1px outset #808080)] > Here is cellspacing 1 so table height should be increased by 2 but it is > increased by 3. > > https://codereview.chromium.org/596823002/diff/260001/LayoutTests/platform/li... > LayoutTests/platform/linux/tables/mozilla/bugs/bug133756-2-expected.txt:29: > RenderTable {TABLE} at (0,111) size 66x26 [border: (1px outset #808080)] > Here is cellspacing 1 so table height should be increased by 2 and here it is > right. > > https://codereview.chromium.org/596823002/diff/260001/LayoutTests/platform/li... > LayoutTests/platform/linux/tables/mozilla/bugs/bug133756-2-expected.txt:39: > RenderTable {TABLE} at (0,157) size 68x29 [border: (1px outset #808080)] > Here is cellspacing 1 so table height should be increased by 4 but it is > increased by 5. > > https://codereview.chromium.org/596823002/diff/260001/LayoutTests/platform/li... > File LayoutTests/platform/linux/tables/mozilla/bugs/bug133756-2-expected.txt > (right): > > https://codereview.chromium.org/596823002/diff/260001/LayoutTests/platform/li... > LayoutTests/platform/linux/tables/mozilla/bugs/bug133756-2-expected.txt:3: layer > at (0,0) size 800x464 > On 2014/12/10 14:07:47, dsinclair wrote: > > Are these pixel differences expected? > > Please check comments on table heights and based on that this layer height is > reasonable which is decreased by 4 pixels. > > https://codereview.chromium.org/596823002/diff/260001/LayoutTests/platform/li... > LayoutTests/platform/linux/tables/mozilla/bugs/bug133756-2-expected.txt:9: > RenderTable {TABLE} at (0,20) size 64x24 [border: (1px outset #808080)] > Here is table height is 24. > > https://codereview.chromium.org/596823002/diff/260001/LayoutTests/platform/li... > LayoutTests/platform/linux/tables/mozilla/bugs/bug133756-2-expected.txt:19: > RenderTable {TABLE} at (0,64) size 66x26 [border: (1px outset #808080)] > Here is cellspacing 1 so table height is increased by 2. > > https://codereview.chromium.org/596823002/diff/260001/LayoutTests/platform/li... > LayoutTests/platform/linux/tables/mozilla/bugs/bug133756-2-expected.txt:29: > RenderTable {TABLE} at (0,110) size 66x26 [border: (1px outset #808080)] > Here is cellpading 1 so table height is increased by 2. > > https://codereview.chromium.org/596823002/diff/260001/LayoutTests/platform/li... > LayoutTests/platform/linux/tables/mozilla/bugs/bug133756-2-expected.txt:39: > RenderTable {TABLE} at (0,156) size 68x28 [border: (1px outset #808080)] > Here is cellspacing 1 and cellpadding 1 so table height is increased by 4. Please review it. All the review comments fixed and justified. Thank you
lgtm https://codereview.chromium.org/596823002/diff/280001/Source/core/rendering/R... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/596823002/diff/280001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:506: if (cell->rowSpan() <= 1) rowSpan is at least 1 so we shouldn't put use <= as it obfuscate the intent. https://codereview.chromium.org/596823002/diff/280001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:510: const unsigned cellRowSpan = cell->rowSpan(); Note that the Chromium coding style bans the use of any integer type except for 'int' and special types like size_t (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types). This means that we should produce new code using size_t. https://codereview.chromium.org/596823002/diff/280001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:516: unsigned startRowForSpanningCellCount = max(cellRowIndex, row); Coding style: std::max should be used not max.
https://codereview.chromium.org/596823002/diff/280001/Source/core/rendering/R... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/596823002/diff/280001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:506: if (cell->rowSpan() <= 1) On 2014/12/16 23:00:27, Julien Chaffraix - OOO til 1-1 wrote: > rowSpan is at least 1 so we shouldn't put use <= as it obfuscate the intent. Done. https://codereview.chromium.org/596823002/diff/280001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:510: const unsigned cellRowSpan = cell->rowSpan(); On 2014/12/16 23:00:27, Julien Chaffraix - OOO til 1-1 wrote: > Note that the Chromium coding style bans the use of any integer type except for > 'int' and special types like size_t > (http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Integer_Types). > > This means that we should produce new code using size_t. In that case, we need to change it in whole table module. I can work as another patch for this changes. Because it is required many changes. https://codereview.chromium.org/596823002/diff/280001/Source/core/rendering/R... Source/core/rendering/RenderTableSection.cpp:516: unsigned startRowForSpanningCellCount = max(cellRowIndex, row); On 2014/12/16 23:00:27, Julien Chaffraix - OOO til 1-1 wrote: > Coding style: std::max should be used not max. Done.
The CQ bit was checked by a.suchit@samsung.com
The CQ bit was unchecked by a.suchit@samsung.com
The CQ bit was checked by a.suchit@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/596823002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for LayoutTests/TestExpectations: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file LayoutTests/TestExpectations Hunk #1 FAILED at 1478. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/TestExpectations.rej Patch: LayoutTests/TestExpectations Index: LayoutTests/TestExpectations diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations index 18bc15b98b5bd9673e2655cce48c0313453178b2..c2811add44bf49d8ef5f43cacc034ff25d1e7d31 100644 --- a/LayoutTests/TestExpectations +++ b/LayoutTests/TestExpectations @@ -1478,3 +1478,9 @@ crbug.com/390488 [ Mac Debug ] svg/custom/foreign-object-skew.svg [ ImageOnlyFai crbug.com/381919 virtual/regionbasedmulticol/fast/multicol/transform-inside-opacity.html [ NeedsRebaseline ] crbug.com/438618 fast/css3-text/css3-text-justify/text-justify-distribute.html [ NeedsRebaseline ] +crbug.com/396655 [ Mac ] tables/mozilla/bugs/bug13169.html [ NeedsRebaseline ] +crbug.com/396655 [ Win ] tables/mozilla/bugs/bug133756-1.html [ NeedsRebaseline ] +crbug.com/396655 [ Win Mac ] tables/mozilla/bugs/bug133756-2.html [ NeedsRebaseline ] +crbug.com/396655 [ Win Mac ] tables/mozilla/bugs/bug7714.html [ NeedsRebaseline ] +crbug.com/396655 [ Win Mac ] tables/mozilla_expected_failures/bugs/bug65372.html [ NeedsRebaseline ] +crbug.com/396655 [ Win Mac ] tables/mozilla/bugs/bug8858.html [ NeedsRebaseline ]
The CQ bit was checked by a.suchit@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/596823002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/36045)
The CQ bit was checked by a.suchit@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/596823002/320001
Message was sent while issue was closed.
Committed patchset #16 (id:320001) as https://src.chromium.org/viewvc/blink?view=rev&revision=187448 |
