Spanning logical height is not added properly in all spanning rows.

Spanning logical height is not added properly in all spanning rows.

The remaining logical height is wrongly added to the last row spanning cell,
not spread on the rows when more then one rowspanning cell present in the table.

Remaining logical height was handled to only row spanning cell which comes
first in the table. Remaining row spanning cells were not handles.

Here, we handled all row spanning cells in the table those are not overlapping
each others.

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

[Julien - ping for review, 2013-07-23 00:13:43]

https://codereview.chromium.org/19390002/diff/6001/LayoutTests/fast/table/tab...
File LayoutTests/fast/table/table-rowspan-height-distribution-in-rows-3.html
(right):

https://codereview.chromium.org/19390002/diff/6001/LayoutTests/fast/table/tab...
LayoutTests/fast/table/table-rowspan-height-distribution-in-rows-3.html:15:
<h3>Test for chromium bug : <a
href="https://code.google.com/p/chromium/issues/detail?id=249600">249600</a>.
Extra logical height is not properly spread over the rows in a row-spanning
cell.</h3>
It would really be better to pick up the meta bug directly instead of changing
the bug number for every change.

https://codereview.chromium.org/19390002/diff/6001/LayoutTests/fast/table/tab...
LayoutTests/fast/table/table-rowspan-height-distribution-in-rows-3.html:78:
</tr>
Can we please add more complex cases as we implement more of the behavior? This
kind of changes make it impossible to know if there is good coverage and also
the more simple tests serve as smoke tests for the more complex ones (here if
this one fails, you have no clue what could be wrong).

https://codereview.chromium.org/19390002/diff/6001/Source/core/rendering/Rend...
File Source/core/rendering/RenderTableSection.cpp (right):

https://codereview.chromium.org/19390002/diff/6001/Source/core/rendering/Rend...
Source/core/rendering/RenderTableSection.cpp:378: unsigned changedHeight = 0;
Trying to find better naming for this one:
- extraHeightToPropagate
- heightToPropagate
- heightDifference
- heightDelta

(changedHeight sounds like the new height where it's actually a difference)

Slight preference for the first name but feel free to find a better one.

https://codereview.chromium.org/19390002/diff/6001/Source/core/rendering/Rend...
Source/core/rendering/RenderTableSection.cpp:393: int initialPos =
m_rowPos[rowIndex + rowSpan];
Let's not abbreviate variables: |initialPosition|

Maybe better: originalBeforePosition

[a.suchit, 2013-07-23 12:19:56]

https://codereview.chromium.org/19390002/diff/6001/LayoutTests/fast/table/tab...
File LayoutTests/fast/table/table-rowspan-height-distribution-in-rows-3.html
(right):

https://codereview.chromium.org/19390002/diff/6001/LayoutTests/fast/table/tab...
LayoutTests/fast/table/table-rowspan-height-distribution-in-rows-3.html:15:
<h3>Test for chromium bug : <a
href="https://code.google.com/p/chromium/issues/detail?id=249600">249600</a>.
Extra logical height is not properly spread over the rows in a row-spanning
cell.</h3>
On 2013/07/23 00:13:43, Julien Chaffraix wrote:
> It would really be better to pick up the meta bug directly instead of changing
> the bug number for every change.

Every patch is created to fix a specific bug in the current changes and it will
clearly specifies that if any problem or regression come in future then
respective bug will reopen instead a meta bug which will point to all the
patches landed to fix meta bug.

https://codereview.chromium.org/19390002/diff/6001/LayoutTests/fast/table/tab...
LayoutTests/fast/table/table-rowspan-height-distribution-in-rows-3.html:78:
</tr>
On 2013/07/23 00:13:43, Julien Chaffraix wrote:
> Can we please add more complex cases as we implement more of the behavior?
This
> kind of changes make it impossible to know if there is good coverage and also
> the more simple tests serve as smoke tests for the more complex ones (here if
> this one fails, you have no clue what could be wrong).

These test cases are specifying that multiple spanning cells in the table are
handled by the patch.
As I am thinking to go for more complex cases like combination of different type
of row height in spanning cell.
So I will add/update test cases for different types of row height (like percent,
auto, ...) in spanning cell.

https://codereview.chromium.org/19390002/diff/6001/Source/core/rendering/Rend...
File Source/core/rendering/RenderTableSection.cpp (right):

https://codereview.chromium.org/19390002/diff/6001/Source/core/rendering/Rend...
Source/core/rendering/RenderTableSection.cpp:378: unsigned changedHeight = 0;
On 2013/07/23 00:13:43, Julien Chaffraix wrote:
> Trying to find better naming for this one:
> - extraHeightToPropagate
> - heightToPropagate
> - heightDifference
> - heightDelta
> 
> (changedHeight sounds like the new height where it's actually a difference)
> 
> Slight preference for the first name but feel free to find a better one.

Done.

https://codereview.chromium.org/19390002/diff/6001/Source/core/rendering/Rend...
Source/core/rendering/RenderTableSection.cpp:393: int initialPos =
m_rowPos[rowIndex + rowSpan];
On 2013/07/23 00:13:43, Julien Chaffraix wrote:
> Let's not abbreviate variables: |initialPosition|
> 
> Maybe better: originalBeforePosition

Done.

[Julien - ping for review, 2013-07-23 18:00:11]

As said, I have a hard time knowing if your new testing is adequate and don't
think what you are advocating is a good testing strategy. My point is that a
layered testing will give us greater coverage at the end and that's what
matters.

https://codereview.chromium.org/19390002/diff/6001/LayoutTests/fast/table/tab...
File LayoutTests/fast/table/table-rowspan-height-distribution-in-rows-3.html
(right):

https://codereview.chromium.org/19390002/diff/6001/LayoutTests/fast/table/tab...
LayoutTests/fast/table/table-rowspan-height-distribution-in-rows-3.html:15:
<h3>Test for chromium bug : <a
href="https://code.google.com/p/chromium/issues/detail?id=249600">249600</a>.
Extra logical height is not properly spread over the rows in a row-spanning
cell.</h3>
On 2013/07/23 12:19:56, a.suchit wrote:
> On 2013/07/23 00:13:43, Julien Chaffraix wrote:
> > It would really be better to pick up the meta bug directly instead of
changing
> > the bug number for every change.
> 
> Every patch is created to fix a specific bug in the current changes and it
will
> clearly specifies that if any problem or regression come in future then
> respective bug will reopen instead a meta bug which will point to all the
> patches landed to fix meta bug.

I have a hard time understanding your point here. All your changes should be
targeted towards fixing the meta bug and the tests should be cumulative (ie we
build on simple test cases towards more complex ones), not isolated.

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

https://codereview.chromium.org/19390002/diff/17001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:407: if
(spanningRowsHeight.totalRowsHeight &&
spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing >
spanningRowsHeight.totalRowsHeight) {
Let's avoid nested the nested if by reversing the condition:

if (!spanningRowsHeight.totalRowsHeight ||
spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing <=
spanningRowsHeight.totalRowsHeight)
    continue;

[a.suchit, 2013-07-24 06:32:38]

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

https://codereview.chromium.org/19390002/diff/17001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:407: if
(spanningRowsHeight.totalRowsHeight &&
spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing >
spanningRowsHeight.totalRowsHeight) {
On 2013/07/23 18:00:12, Julien Chaffraix wrote:
> Let's avoid nested the nested if by reversing the condition:
> 
> if (!spanningRowsHeight.totalRowsHeight ||
> spanningRowsHeight.spanningCellHeightIgnoringBorderSpacing <=
> spanningRowsHeight.totalRowsHeight)
>     continue;

Done.

[Julien - ping for review, 2013-07-24 23:14:41]

lgtm

https://codereview.chromium.org/19390002/diff/38001/LayoutTests/fast/table/ta...
File LayoutTests/fast/table/table-rowspan-height-distribution-in-rows-3.html
(right):

https://codereview.chromium.org/19390002/diff/38001/LayoutTests/fast/table/ta...
LayoutTests/fast/table/table-rowspan-height-distribution-in-rows-3.html:78:
</tr>
Shouldn't we test a mix of specified (percentage and different type of length
(em, px, vw, vh, ...)) and auto?

https://codereview.chromium.org/19390002/diff/38001/LayoutTests/fast/table/ta...
LayoutTests/fast/table/table-rowspan-height-distribution-in-rows-3.html:288:
<h5>Test 6 - RowSpan and ColSpan. </h5>
From the previous diff, this seemed like starting here, it is a copy and paste
of table-rowspan-height-distribution-in-rows-2.html so we may as well remove it
as it doesn't add more coverage.

BTW, I usually prefer test *without* those numbers as they make very poor names:
can you tell me the difference between
table-rowspan-height-distribution-in-rows-3.html and
table-rowspan-height-distribution-in-rows-1.html without looking at the file?

[a.suchit, 2013-07-25 10:08:01]

https://codereview.chromium.org/19390002/diff/38001/LayoutTests/fast/table/ta...
File LayoutTests/fast/table/table-rowspan-height-distribution-in-rows-3.html
(right):

https://codereview.chromium.org/19390002/diff/38001/LayoutTests/fast/table/ta...
LayoutTests/fast/table/table-rowspan-height-distribution-in-rows-3.html:78:
</tr>
On 2013/07/24 23:14:41, Julien Chaffraix wrote:
> Shouldn't we test a mix of specified (percentage and different type of length
> (em, px, vw, vh, ...)) and auto?

I am mostly writing the test cases based on the changes done in the code. These
changes are basically depend in percent height, fixed height and auto height and
I tried to cover all three in test cases.
For the fixed height, I am just using unit px because using all fixed type units
will not give any advantages in that case. All fixed type units will cover same
think in these test cases. em, vw, vh, ... are not depend on table height. So I
think these should come under fixed height for the table. 
Row or cell without height specified would be consider as auto so I did not
specify any where height=auto.

https://codereview.chromium.org/19390002/diff/38001/LayoutTests/fast/table/ta...
LayoutTests/fast/table/table-rowspan-height-distribution-in-rows-3.html:288:
<h5>Test 6 - RowSpan and ColSpan. </h5>
On 2013/07/24 23:14:41, Julien Chaffraix wrote:
> From the previous diff, this seemed like starting here, it is a copy and paste
> of table-rowspan-height-distribution-in-rows-2.html so we may as well remove
it
> as it doesn't add more coverage.
> 
> BTW, I usually prefer test *without* those numbers as they make very poor
names:
> can you tell me the difference between
> table-rowspan-height-distribution-in-rows-3.html and
> table-rowspan-height-distribution-in-rows-1.html without looking at the file?

changed named from table-rowspan-height-distribution-in-rows-3.html to
table-all-rowspans-height-distribution-in-rows-except-overlapped.html

https://codereview.chromium.org/19390002/diff/38001/LayoutTests/fast/table/ta...
LayoutTests/fast/table/table-rowspan-height-distribution-in-rows-3.html:288:
<h5>Test 6 - RowSpan and ColSpan. </h5>
On 2013/07/24 23:14:41, Julien Chaffraix wrote:
> From the previous diff, this seemed like starting here, it is a copy and paste
> of table-rowspan-height-distribution-in-rows-2.html so we may as well remove
it
> as it doesn't add more coverage.
> 
> BTW, I usually prefer test *without* those numbers as they make very poor
names:
> can you tell me the difference between
> table-rowspan-height-distribution-in-rows-3.html and
> table-rowspan-height-distribution-in-rows-1.html without looking at the file?

Done.

[commit-bot: I haz the power, 2013-07-25 18:43:14]

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

[commit-bot: I haz the power, 2013-07-25 20:01:16]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[commit-bot: I haz the power, 2013-07-26 08:34:04]

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

[commit-bot: I haz the power, 2013-07-26 08:34:05]

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 1726.
  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
669f4ef6e61e3e735dddf74bb0d57720d0a37c58..4d3fd923afd5cf95d25b3cf682ba2d3d1e106ee5
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -1726,6 +1726,13 @@ crbug.com/254914 [ Win Mac ]
tables/mozilla/core/bloomberg.html [ NeedsRebaselin
 # Image only
 crbug.com/254914 [ Linux ]
tables/mozilla_expected_failures/bugs/bug58402-2.html [ NeedsRebaseline ]
 crbug.com/254914 [ Linux ] tables/mozilla/core/bloomberg.html [ NeedsRebaseline
]
+
+# Below test case need to rebaseline after crbug.com/249600
+# Image only
+crbug.com/249600 [ Linux Win ] tables/mozilla/bugs/bug7714.html [
NeedsRebaseline ]
 # Text only
-crbug.com/254914 [ Mac ]
fast/table/table-rowspan-height-distribution-in-rows-1.html [ NeedsRebaseline ]
-crbug.com/254914 [ Mac ]
fast/table/table-rowspan-height-distribution-in-rows-2.html [ NeedsRebaseline ]
+crbug.com/249600 [ Mac ]
fast/table/table-rowspan-height-distribution-in-rows-1.html [ NeedsRebaseline ]
+crbug.com/249600 [ Mac ]
fast/table/table-rowspan-height-distribution-in-rows-2.html [ NeedsRebaseline ]
+crbug.com/249600 [ Mac ]
fast/table/table-all-rowspans-height-distribution-in-rows-except-overlapped.html
[ NeedsRebaseline ]
+# Text and image both
+crbug.com/249600 [ Mac ] tables/mozilla/bugs/bug7714.html [ NeedsRebaseline ]

[commit-bot: I haz the power, 2013-07-26 08:55:33]

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

[commit-bot: I haz the power, 2013-07-26 09:33:08]

Retried try job too often on mac_layout_rel for step(s) webkit_lint,
webkit_python_tests, webkit_tests, webkit_unit_tests, weborigin_unittests,
wtf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout...

[commit-bot: I haz the power, 2013-07-26 09:42:10]

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

[commit-bot: I haz the power, 2013-07-26 11:12:45]

Change committed as 154990