Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18050007:
Height of fixed height cell is not proper when cell's row is under row spanning cell. (Closed)
|
Created:
7 years, 5 months ago by a.suchit Modified:
7 years, 5 months ago CC:
blink-reviews, dglazkov+blink, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionHeight of fixed height cell is not proper when cell's row is under row spanning cell.
If cell have fixed height then it's height should not changed till there is
other rows present to take extra rowspan cell height.
Extra rowspan cell height, First, distributes to percent height spanning rows
which is depend on table height. If extra rowspan cell height remains after
distributing to percent height rows then remaining extra rowspan cell height
distributes in auto spanning rows if auto spanning rows are present otherwise
it distributes to remaining spanning rows.
R=jchaffraix@chromium.org
BUG=254914
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=154604
Patch Set 1 #
Total comments: 4
Patch Set 2 : #Patch Set 3 : #
Total comments: 18
Patch Set 4 : Review comments addressed #
Total comments: 15
Patch Set 5 : #
Total comments: 16
Patch Set 6 : Review comments addressed #
Total comments: 12
Patch Set 7 : #
Total comments: 10
Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #
Total comments: 1
Messages
Total messages: 25 (0 generated)
I don't think this is going in the right direction at all. You should gather the information about which rows have which type of length upfront and then do a pass at spreading the logical height for percent, then auto, then all the rest. Currently your code is super complicated as you have to patch your sizes a posteriori. https://codereview.chromium.org/18050007/diff/1/Source/core/rendering/RenderT... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/18050007/diff/1/Source/core/rendering/RenderT... Source/core/rendering/RenderTableSection.cpp:282: void RenderTableSection::recalcRowsHeightInRowSpanningCell(RenderTableCell* cell, int& expectedTotalRowsHeight, int& currTotalRowsHeight, Vector<int>& rowsHeight) You pass |currTotalRowsHeight| as a reference, yet you never update it. The name of the function also gives no direction as to what this giant function is supposed to do. https://codereview.chromium.org/18050007/diff/1/Source/core/rendering/RenderT... Source/core/rendering/RenderTableSection.cpp:284: ASSERT(cell); If you need those type of ASSERT, it means that you should pass a reference and not a pointer to the function. https://codereview.chromium.org/18050007/diff/1/Source/core/rendering/RenderT... Source/core/rendering/RenderTableSection.cpp:295: int extraHeight = expectedTotalRowsHeight - currTotalRowsHeight; I am sorry but I can't say what |expectedTotalRowHeight| could mean, same for |currTotalRowsHeight|. https://codereview.chromium.org/18050007/diff/1/Source/core/rendering/RenderT... Source/core/rendering/RenderTableSection.cpp:301: totalRowsHeightExceptPercentRows = currTotalRowsHeight; Why do we need 2 initialization of this variable?
Split giant function 'recalcRowsHeightInRowSpanningCell' in to small-small functions based on their functionality.
https://codereview.chromium.org/18050007/diff/22001/Source/core/rendering/Ren... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/18050007/diff/22001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:256: int RenderTableSection::getRowsHeightInRowSpan(RenderTableCell* cell, int& expectedTotalRowsHeight, Vector<int>& rowsHeight) This function starts to smell badly as you now have 2 in-parameters and one out-parameter. That makes it hard to see what's going on. https://codereview.chromium.org/18050007/diff/22001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:278: // Else total percent share would go to percent rows and remaining would be distributed in other rows. I read this several times and failed to understand it. You should make your code self-explanatory and remove those comments that just distract people from the actual code. https://codereview.chromium.org/18050007/diff/22001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:287: if (100 <= totalPercent) { I had a hard time understanding this check and believe it's probably wrong in the general case: you are making an assumption that you can just handle the percentage locally at the current cell's level. If your table is above 100% but not your cell's rows, your percentage will be off as percentages refers to the whole table's dimension. https://codereview.chromium.org/18050007/diff/22001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:297: extraRowSpanningHeight = 0; Again, it is WRONG to clear extraRowSpanningHeight as you are neutering your ASSERT. If you have rounding issues that make the ASSERT trigger, they should be fixed. https://codereview.chromium.org/18050007/diff/22001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:330: extraRowSpanningHeight = 0; This is wrong to clear extraRowSpanningHeight unconditionally here. https://codereview.chromium.org/18050007/diff/22001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:334: void RenderTableSection::distributeExtraRowSpanHeightToRemainingRows(RenderTableCell* cell, int totalRemainingRowsHeight, int& extraRowSpanningHeight, Vector<int>& rowsHeight) Because of extraRowSpanningHeight = 0; above, this function is pretty much dead code. https://codereview.chromium.org/18050007/diff/22001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:346: m_rowPos[row + 1] += changedPosBy; The more I see these lines that basically mandates O(n) behavior *all the time*, the more I think we should have some better data structure for layout. The issue with rowPos is that it stores the absolute position, not the row's heights which we manipulate heavily during layout. https://codereview.chromium.org/18050007/diff/22001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:351: extraRowSpanningHeight = 0; This is just gross. Nothing guarantees that changedPosBy == extraRowSpanningHeight and thus you are making the ASSERT(!extraRowSpanningHeight) useless because of this change.
https://codereview.chromium.org/18050007/diff/22001/Source/core/rendering/Ren... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/18050007/diff/22001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:256: int RenderTableSection::getRowsHeightInRowSpan(RenderTableCell* cell, int& expectedTotalRowsHeight, Vector<int>& rowsHeight) On 2013/07/02 02:16:18, Julien Chaffraix wrote: > This function starts to smell badly as you now have 2 in-parameters and one > out-parameter. That makes it hard to see what's going on. Done. https://codereview.chromium.org/18050007/diff/22001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:287: if (100 <= totalPercent) { On 2013/07/02 02:16:18, Julien Chaffraix wrote: > I had a hard time understanding this check and believe it's probably wrong in > the general case: you are making an assumption that you can just handle the > percentage locally at the current cell's level. > > If your table is above 100% but not your cell's rows, your percentage will be > off as percentages refers to the whole table's dimension. At this point, total table height is not fixed so we are taking percent based on the current rowspan cell's level. At the end of table layout, row's height percent are handled saperatly based on table height if table have extra logical height. https://codereview.chromium.org/18050007/diff/22001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:297: extraRowSpanningHeight = 0; On 2013/07/02 02:16:18, Julien Chaffraix wrote: > Again, it is WRONG to clear extraRowSpanningHeight as you are neutering your > ASSERT. If you have rounding issues that make the ASSERT trigger, they should be > fixed. Done. https://codereview.chromium.org/18050007/diff/22001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:330: extraRowSpanningHeight = 0; On 2013/07/02 02:16:18, Julien Chaffraix wrote: > This is wrong to clear extraRowSpanningHeight unconditionally here. Done. https://codereview.chromium.org/18050007/diff/22001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:334: void RenderTableSection::distributeExtraRowSpanHeightToRemainingRows(RenderTableCell* cell, int totalRemainingRowsHeight, int& extraRowSpanningHeight, Vector<int>& rowsHeight) On 2013/07/02 02:16:18, Julien Chaffraix wrote: > Because of extraRowSpanningHeight = 0; above, this function is pretty much dead > code. If rowspan cell does not contain auto rows then extra height would be distributed in all rows. https://codereview.chromium.org/18050007/diff/22001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:334: void RenderTableSection::distributeExtraRowSpanHeightToRemainingRows(RenderTableCell* cell, int totalRemainingRowsHeight, int& extraRowSpanningHeight, Vector<int>& rowsHeight) On 2013/07/02 02:16:18, Julien Chaffraix wrote: > Because of extraRowSpanningHeight = 0; above, this function is pretty much dead > code. Done. https://codereview.chromium.org/18050007/diff/22001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:346: m_rowPos[row + 1] += changedPosBy; On 2013/07/02 02:16:18, Julien Chaffraix wrote: > The more I see these lines that basically mandates O(n) behavior *all the time*, > the more I think we should have some better data structure for layout. The issue > with rowPos is that it stores the absolute position, not the row's heights which > we manipulate heavily during layout. m_rowPos is already handled to store the absolute positions, so if we will go to change it then it will heavily affect all the existing places and we need to change all over the places. https://codereview.chromium.org/18050007/diff/22001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:351: extraRowSpanningHeight = 0; On 2013/07/02 02:16:18, Julien Chaffraix wrote: > This is just gross. Nothing guarantees that changedPosBy == > extraRowSpanningHeight and thus you are making the > ASSERT(!extraRowSpanningHeight) useless because of this change. Done.
I didn't have time to look at the tests nor the rebaselines. https://codereview.chromium.org/18050007/diff/22001/Source/core/rendering/Ren... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/18050007/diff/22001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:287: if (100 <= totalPercent) { On 2013/07/03 12:59:57, a.suchit wrote: > On 2013/07/02 02:16:18, Julien Chaffraix wrote: > > I had a hard time understanding this check and believe it's probably wrong in > > the general case: you are making an assumption that you can just handle the > > percentage locally at the current cell's level. > > > > If your table is above 100% but not your cell's rows, your percentage will be > > off as percentages refers to the whole table's dimension. > > At this point, total table height is not fixed so we are taking percent based on > the current rowspan cell's level. I am still not totally convinced this is correct to ignore other percentage for that but I would have to come up with a good counter-argument if I can find one. > At the end of table layout, row's height percent are handled saperatly based on > table height if table have extra logical height. That's a separate piece of code and not one that has much bearing on this change (apart from the similarity). https://codereview.chromium.org/18050007/diff/22001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:346: m_rowPos[row + 1] += changedPosBy; On 2013/07/03 12:59:57, a.suchit wrote: > On 2013/07/02 02:16:18, Julien Chaffraix wrote: > > The more I see these lines that basically mandates O(n) behavior *all the > time*, > > the more I think we should have some better data structure for layout. The > issue > > with rowPos is that it stores the absolute position, not the row's heights > which > > we manipulate heavily during layout. > > m_rowPos is already handled to store the absolute positions, so if we will go to > change it then it will heavily affect all the existing places and we need to > change all over the places. That's an understatement! However this argument shouldn't prevent us from doing the right thing. What you don't account for is that it would probably remove a lot of complexity in your change and the layout code. https://codereview.chromium.org/18050007/diff/42001/Source/core/rendering/Ren... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/18050007/diff/42001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:258: void RenderTableSection::getRowsHeightInRowSpan(RenderTableCell* cell, int& expectedTotalRowsHeight, int& totalRowsHeight, Vector<int>& rowsHeight) Really get a structure for the parameters you always pass around, that would make the code a lot more readable. That would also show you that naming this a getter is really a misnomer, it's populating your variable. https://codereview.chromium.org/18050007/diff/42001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:273: expectedTotalRowsHeight = rowSpanCellHeight; Why do we need |rowSpanCellHeight| then? https://codereview.chromium.org/18050007/diff/42001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:280: void RenderTableSection::distributeExtraRowSpanHeightToPrecentRows(RenderTableCell* cell, int totalPercent, int& extraRowSpanningHeight, Vector<int>& rowsHeight) |cell| is not modified (and is not expected to be) so it should be const RenderTableCell*. This comment applies to the other methods. https://codereview.chromium.org/18050007/diff/42001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:288: if (100 < totalPercent) { You never answered about this and I am pretty much convinced this is wrong and adds some unneeded complexity. https://codereview.chromium.org/18050007/diff/42001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:303: changedPosBy += extraRowSpanningHeight * m_grid[row].logicalHeight.percent() / 100; This code is backwards if you look at what we do in RenderTableSection::distributeExtraLogicalHeightToPercentRows: totalPercent = min(totalPercent, 100) (ie we allow totalPercent to be above 100 and still keep the total ratio correct which seems to match FF). https://codereview.chromium.org/18050007/diff/42001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:313: // Else extra height would be distributed in other rows. This line is useless. As a whole, your comments are *what* comments and for the second time, I am pointing out that *why* comments are much much more useful. https://codereview.chromium.org/18050007/diff/42001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:328: // Remaining height added in the last row under rowSpan cell This comment is also a *what* comment, I would rather see a *why* comment like: // If we have at least an auto row(s), we need to ensure that we have distributed all the extra space to the last one. Not doing so would yield to fixed rows being wrongly increased. Btw, that whole comment hints at a missing ASSERT here: ASSERT(!extraRowSpanningHeight); https://codereview.chromium.org/18050007/diff/42001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:333: // Whole extra height would be distribute in remaining rows based on their height ratios so that Again, that's a what comment. Here you could explain: 1) that it's the last hope of spreading the extra logical space so we just do it. 2) why it makes sense to use the current height ratio as our guide.
Also please fix the description "is not proper" is vague and indefinite.
https://codereview.chromium.org/18050007/diff/42001/Source/core/rendering/Ren... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/18050007/diff/42001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:258: void RenderTableSection::getRowsHeightInRowSpan(RenderTableCell* cell, int& expectedTotalRowsHeight, int& totalRowsHeight, Vector<int>& rowsHeight) On 2013/07/03 21:08:22, Julien Chaffraix wrote: > Really get a structure for the parameters you always pass around, that would > make the code a lot more readable. > > That would also show you that naming this a getter is really a misnomer, it's > populating your variable. Done. https://codereview.chromium.org/18050007/diff/42001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:273: expectedTotalRowsHeight = rowSpanCellHeight; On 2013/07/03 21:08:22, Julien Chaffraix wrote: > Why do we need |rowSpanCellHeight| then? Changed https://codereview.chromium.org/18050007/diff/42001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:288: if (100 < totalPercent) { On 2013/07/03 21:08:22, Julien Chaffraix wrote: > You never answered about this and I am pretty much convinced this is wrong and > adds some unneeded complexity. Removed https://codereview.chromium.org/18050007/diff/42001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:303: changedPosBy += extraRowSpanningHeight * m_grid[row].logicalHeight.percent() / 100; On 2013/07/03 21:08:22, Julien Chaffraix wrote: > This code is backwards if you look at what we do in > RenderTableSection::distributeExtraLogicalHeightToPercentRows: > > totalPercent = min(totalPercent, 100) > > (ie we allow totalPercent to be above 100 and still keep the total ratio correct > which seems to match FF). Done. https://codereview.chromium.org/18050007/diff/42001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:313: // Else extra height would be distributed in other rows. On 2013/07/03 21:08:22, Julien Chaffraix wrote: > This line is useless. As a whole, your comments are *what* comments and for the > second time, I am pointing out that *why* comments are much much more useful. Done. https://codereview.chromium.org/18050007/diff/42001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:328: // Remaining height added in the last row under rowSpan cell On 2013/07/03 21:08:22, Julien Chaffraix wrote: > This comment is also a *what* comment, I would rather see a *why* comment like: > > // If we have at least an auto row(s), we need to ensure that we have > distributed all the extra space to the last one. Not doing so would yield to > fixed rows being wrongly increased. > > Btw, that whole comment hints at a missing ASSERT here: > ASSERT(!extraRowSpanningHeight); Done. https://codereview.chromium.org/18050007/diff/42001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:333: // Whole extra height would be distribute in remaining rows based on their height ratios so that On 2013/07/03 21:08:22, Julien Chaffraix wrote: > Again, that's a what comment. Here you could explain: > 1) that it's the last hope of spreading the extra logical space so we just do > it. > 2) why it makes sense to use the current height ratio as our guide. Done.
https://codereview.chromium.org/18050007/diff/59001/Source/core/rendering/Ren... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/18050007/diff/59001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:263: spanningRowsHeight.expectedTotalRowsHeight = cell->logicalHeightForRowSizing(); I have a hard time with this variable |expectedTotalRowsHeight|. In a way, it is expected but the context feels weird. How about spanningCellHeightIgnoringBorderSpacing? https://codereview.chromium.org/18050007/diff/59001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:276: void RenderTableSection::distributeExtraRowSpanHeightToPrecentRows(RenderTableCell* cell, int tableHeight, int totalPercent, int& extraRowSpanningHeight) typo: PrecentRows https://codereview.chromium.org/18050007/diff/59001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:282: unsigned rowIndex = cell->rowIndex(); const? https://codereview.chromium.org/18050007/diff/59001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:300: extraRowSpanningHeight = remainingRowSpanningHeight > 0 ? remainingRowSpanningHeight : 0; If you need to do a max, please just use one instead of re-inventing the wheel. I wouldn't expect remainingRowSpanningHeight to be negative btw or that would mean that you over-extended (and/or have rounding errors). https://codereview.chromium.org/18050007/diff/59001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:310: int changedPosBy = 0; Not a fan of |changedPosBy|, how about |accumulatedPositionIncrease|. https://codereview.chromium.org/18050007/diff/59001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:321: // Full height is distributed in auto rows so make it 0. Remember my asking always about *why*, that's another case of a what comment that people can make from the code without help. Btw, you code will probably be off in most cases as you are doing an integer division above to split the extra space. That's the reason why RenderTableSection::distributeExtraLogicalHeightToAutoRows updates the |extraLogicalHeight| in the for loop. https://codereview.chromium.org/18050007/diff/59001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:344: extraRowSpanningHeight = 0; What guarantees that? https://codereview.chromium.org/18050007/diff/59001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:387: ASSERT(!extraRowSpanningHeight); This ASSERT is neutered by distributeExtraRowSpanHeightToRemainingRows :(
https://codereview.chromium.org/18050007/diff/59001/Source/core/rendering/Ren... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/18050007/diff/59001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:263: spanningRowsHeight.expectedTotalRowsHeight = cell->logicalHeightForRowSizing(); On 2013/07/13 00:56:36, Julien Chaffraix wrote: > I have a hard time with this variable |expectedTotalRowsHeight|. In a way, it is > expected but the context feels weird. > > How about spanningCellHeightIgnoringBorderSpacing? Done. https://codereview.chromium.org/18050007/diff/59001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:276: void RenderTableSection::distributeExtraRowSpanHeightToPrecentRows(RenderTableCell* cell, int tableHeight, int totalPercent, int& extraRowSpanningHeight) On 2013/07/13 00:56:36, Julien Chaffraix wrote: > typo: PrecentRows Done. https://codereview.chromium.org/18050007/diff/59001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:282: unsigned rowIndex = cell->rowIndex(); On 2013/07/13 00:56:36, Julien Chaffraix wrote: > const? Done. https://codereview.chromium.org/18050007/diff/59001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:300: extraRowSpanningHeight = remainingRowSpanningHeight > 0 ? remainingRowSpanningHeight : 0; On 2013/07/13 00:56:36, Julien Chaffraix wrote: > If you need to do a max, please just use one instead of re-inventing the wheel. > > I wouldn't expect remainingRowSpanningHeight to be negative btw or that would > mean that you over-extended (and/or have rounding errors). changedPosBy += min(toAdd, remainingRowSpanningHeight); remainingRowSpanningHeight -= toAdd; |ChangedPosBy| is increasing by value min(toAdd, remainingRowSpanningHeight) and |remainingRowspanningHeight| should be decremented by same value. But it was not done just because of saving min called twice. So in some cases |remainingRowSpanningHeight| was going in negative and handled it after the loop. Now it is changed. min(toAdd, remainingRowSpanningHeight) value took in |toAdd| and |toAdd| used to update |ChangedPosBy| and |remainingRowSpanningHeight|. https://codereview.chromium.org/18050007/diff/59001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:310: int changedPosBy = 0; On 2013/07/13 00:56:36, Julien Chaffraix wrote: > Not a fan of |changedPosBy|, how about |accumulatedPositionIncrease|. Done. https://codereview.chromium.org/18050007/diff/59001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:321: // Full height is distributed in auto rows so make it 0. On 2013/07/13 00:56:36, Julien Chaffraix wrote: > Remember my asking always about *why*, that's another case of a what comment > that people can make from the code without help. > > Btw, you code will probably be off in most cases as you are doing an integer > division above to split the extra space. That's the reason why > RenderTableSection::distributeExtraLogicalHeightToAutoRows updates the > |extraLogicalHeight| in the for loop. Done. https://codereview.chromium.org/18050007/diff/59001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:344: extraRowSpanningHeight = 0; On 2013/07/13 00:56:36, Julien Chaffraix wrote: > What guarantees that? We are distributing the whole extra row spanning height in spanning rows based on rows weightage. I some cases, very small amount of height will remain which is added in last spanning row. https://codereview.chromium.org/18050007/diff/59001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:387: ASSERT(!extraRowSpanningHeight); On 2013/07/13 00:56:36, Julien Chaffraix wrote: > This ASSERT is neutered by distributeExtraRowSpanHeightToRemainingRows :( Full height is distributed in the spanning rows by adding remaining very small height in the last row spanning cell. So Assert is removed.
https://codereview.chromium.org/18050007/diff/69001/Source/core/rendering/Ren... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/18050007/diff/69001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:258: void RenderTableSection::calcRowsHeightInRowSpan(RenderTableCell* cell, struct SpanningRowsHeight& spanningRowsHeight) Not a huge fan of the name (calc is an abbreviation that doesn't save us much), how about populateSpanningRowsHeightFromCell? https://codereview.chromium.org/18050007/diff/69001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:273: spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing += borderSpacingForRow(rowIndex + rowSpan - 1); This is typically the line of code that would require some explanations for the reader's sake. Based on what it's supposed to do here is a canvas: // We don't span the following row so its border-spacing (if any) should be included. https://codereview.chromium.org/18050007/diff/69001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:284: int remainingRowSpanningHeight = extraRowSpanningHeight; tableHeight + extraRowSpanningHeight is the only reason you need |remainingRowSpanningHeight|. It's also a constant that you can store upfront. https://codereview.chromium.org/18050007/diff/69001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:290: int toAdd = (tableHeight + extraRowSpanningHeight) * m_grid[row].logicalHeight.percent() / 100; The whole algorithm really need some explanations, like saying why is it correct for totalPercent > 100 would definitely help. You stripped all comments but the comments I was against were "what" comment describing what the code was doing in a bad way. Explaining why we do it this way is very valuable (e.g. why did we chose this way of spreading the logical height? ...) https://codereview.chromium.org/18050007/diff/69001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:349: // Value should be round properly in the algorithm. Can't we do the rounding properly instead of swiping the dust under the carpet here? https://codereview.chromium.org/18050007/diff/69001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:393: The ASSERT was really important, what I wasn't happy with is overriding extraRowSpanningHeight to 0 (which you are still doing :(), which is just weakening it. https://codereview.chromium.org/18050007/diff/69001/Source/core/rendering/Ren... File Source/core/rendering/RenderTableSection.h (right): https://codereview.chromium.org/18050007/diff/69001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.h:129: struct SpanningRowsHeight { It should probably be WTF_MAKE_NONCOPYABLE as we don't want it copied.
https://codereview.chromium.org/18050007/diff/69001/Source/core/rendering/Ren... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/18050007/diff/69001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:258: void RenderTableSection::calcRowsHeightInRowSpan(RenderTableCell* cell, struct SpanningRowsHeight& spanningRowsHeight) On 2013/07/16 17:22:13, Julien Chaffraix wrote: > Not a huge fan of the name (calc is an abbreviation that doesn't save us much), > how about populateSpanningRowsHeightFromCell? Done. https://codereview.chromium.org/18050007/diff/69001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:273: spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing += borderSpacingForRow(rowIndex + rowSpan - 1); On 2013/07/16 17:22:13, Julien Chaffraix wrote: > This is typically the line of code that would require some explanations for the > reader's sake. Based on what it's supposed to do here is a canvas: > > // We don't span the following row so its border-spacing (if any) should be > included. Done. https://codereview.chromium.org/18050007/diff/69001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:284: int remainingRowSpanningHeight = extraRowSpanningHeight; On 2013/07/16 17:22:13, Julien Chaffraix wrote: > tableHeight + extraRowSpanningHeight is the only reason you need > |remainingRowSpanningHeight|. It's also a constant that you can store upfront. Done. https://codereview.chromium.org/18050007/diff/69001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:349: // Value should be round properly in the algorithm. On 2013/07/16 17:22:13, Julien Chaffraix wrote: > Can't we do the rounding properly instead of swiping the dust under the carpet > here? Done. https://codereview.chromium.org/18050007/diff/69001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:393: On 2013/07/16 17:22:13, Julien Chaffraix wrote: > The ASSERT was really important, what I wasn't happy with is overriding > extraRowSpanningHeight to 0 (which you are still doing :(), which is just > weakening it. Done.
lgtm https://codereview.chromium.org/18050007/diff/87001/LayoutTests/TestExpectations File LayoutTests/TestExpectations (right): https://codereview.chromium.org/18050007/diff/87001/LayoutTests/TestExpectati... LayoutTests/TestExpectations:1731: crbug.com/254914 [ Win Mac ] tables/mozilla_expected_failures/bugs/bug58402-2.html [ Failure ] We don't mark tests that needs a rebaseline as failures anymore. Welcome the new NeedsRebaseline keyword (https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/J4i52waev9w) https://codereview.chromium.org/18050007/diff/87001/Source/core/rendering/Ren... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/18050007/diff/87001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:284: // we are doing here as per FF. Extra spanning height would be distributed Only in first percent height rows Nit: I would turn the first sentence as: // Our algorithm matches Firefox. (However please do capitalize the first letter, it's an English sentence!) https://codereview.chromium.org/18050007/diff/87001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:290: int toAdd = (tableHeight * m_grid[row].logicalHeight.percent() / 100) - rowsHeight[row - rowIndex]; Note that this is wrong if we have a percentage above 100% and may make us grow above the available space (you divide by 100, not the |totalPercent| which can make us allocate more than 100%). I don't expect you to fix this in this change but let's add a FIXME about it. https://codereview.chromium.org/18050007/diff/87001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:312: // Extra height distributed in auto spanning rows based on their weightage in spanning cell. weightage is an Indian English word, let's use weight instead (not repeated for every use of the word). Btw this doesn't really add much as this is (again) a 'what' comment. What is more interesting is how is the weight determined? Why did we pick this weighting algorithm instead of another one? If you can't come with a good and clear explanation, I would rather remove all those comments. https://codereview.chromium.org/18050007/diff/87001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:320: // is equvalent to divisor then 1 unit increased in row position. I would simplify this comment (which took me a while to understand): // While distributing the extra spanning height, we accumulate the rounding errors due to the integer divisions. We add one in increment of totalAutoRowsHeight to keep the aspect ratio of the rows. With maybe this additional note: // Note that this algorithm is biased towards adding more space towards the lower rows.
https://codereview.chromium.org/18050007/diff/87001/LayoutTests/TestExpectations File LayoutTests/TestExpectations (right): https://codereview.chromium.org/18050007/diff/87001/LayoutTests/TestExpectati... LayoutTests/TestExpectations:1731: crbug.com/254914 [ Win Mac ] tables/mozilla_expected_failures/bugs/bug58402-2.html [ Failure ] On 2013/07/18 18:19:30, Julien Chaffraix wrote: > We don't mark tests that needs a rebaseline as failures anymore. Welcome the new > NeedsRebaseline keyword > (https://groups.google.com/a/chromium.org/forum/#%21topic/blink-dev/J4i52waev9w) Done. https://codereview.chromium.org/18050007/diff/87001/Source/core/rendering/Ren... File Source/core/rendering/RenderTableSection.cpp (right): https://codereview.chromium.org/18050007/diff/87001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:284: // we are doing here as per FF. Extra spanning height would be distributed Only in first percent height rows On 2013/07/18 18:19:30, Julien Chaffraix wrote: > Nit: I would turn the first sentence as: > > // Our algorithm matches Firefox. > > (However please do capitalize the first letter, it's an English sentence!) Done. https://codereview.chromium.org/18050007/diff/87001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:290: int toAdd = (tableHeight * m_grid[row].logicalHeight.percent() / 100) - rowsHeight[row - rowIndex]; On 2013/07/18 18:19:30, Julien Chaffraix wrote: > Note that this is wrong if we have a percentage above 100% and may make us grow > above the available space (you divide by 100, not the |totalPercent| which can > make us allocate more than 100%). I don't expect you to fix this in this change > but let's add a FIXME about it. Done. https://codereview.chromium.org/18050007/diff/87001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:312: // Extra height distributed in auto spanning rows based on their weightage in spanning cell. On 2013/07/18 18:19:30, Julien Chaffraix wrote: > weightage is an Indian English word, let's use weight instead (not repeated for > every use of the word). > > Btw this doesn't really add much as this is (again) a 'what' comment. What is > more interesting is how is the weight determined? Why did we pick this weighting > algorithm instead of another one? > > If you can't come with a good and clear explanation, I would rather remove all > those comments. Done. https://codereview.chromium.org/18050007/diff/87001/Source/core/rendering/Ren... Source/core/rendering/RenderTableSection.cpp:320: // is equvalent to divisor then 1 unit increased in row position. On 2013/07/18 18:19:30, Julien Chaffraix wrote: > I would simplify this comment (which took me a while to understand): > > // While distributing the extra spanning height, we accumulate the rounding > errors due to the integer divisions. We add one in increment of > totalAutoRowsHeight to keep the aspect ratio of the rows. > > With maybe this additional note: > // Note that this algorithm is biased towards adding more space towards the > lower rows. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/a.suchit@samsung.com/18050007/103001
Can't process patch for file LayoutTests/platform/linux/tables/mozilla/core/bloomberg-expected.png. Binary file support is temporarilly disabled due to a bug. Please commit blindly the binary files first then commit the source change as a separate CL. Sorry for the annoyance.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/a.suchit@samsung.com/18050007/108003
Retried try job too often on mac_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/a.suchit@samsung.com/18050007/108003
Retried try job too often on win_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_blink_...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/a.suchit@samsung.com/18050007/119001
Retried try job too often on mac_layout_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/a.suchit@samsung.com/18050007/126003
Message was sent while issue was closed.
Change committed as 154604
Message was sent while issue was closed.
https://codereview.chromium.org/18050007/diff/126003/LayoutTests/TestExpectat... File LayoutTests/TestExpectations (right): https://codereview.chromium.org/18050007/diff/126003/LayoutTests/TestExpectat... LayoutTests/TestExpectations:1728: # Below test case need to rebaseline after crbug.com/254914 I tried new keyword NeedsRebaseline but I was facing some issues and was not able to identify problems. So I changed it back based on old rebaseline method. I also tried to get problem from someone in IRC. From next patches, I will use NeedsRebaseline keyword. |
