Fix Assert failure for test case bug7714.html

Fix Assert failure for test case bug7714.html

While distributing spanning cell's extra height in rows, some spanning cells
does not have enough extra height to distribute in that case, last row's
height of the previous spanning cell is updated more then once so position
of the row are not proper and below row gets negative height. To stop
updating row's height twice, row's height sets back to its original position.

R=jchaffraix@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155825

[suchit.agrawal, 2013-08-06 13:09:45]

This should be fixed in the patch which is landed today.
Please check that can be land this patch separately OR we need to revert
previous patch and do this change in that and upload.

[Julien - ping for review, 2013-08-06 18:08:08]

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

https://codereview.chromium.org/22415003/diff/2/Source/core/rendering/RenderT...
Source/core/rendering/RenderTableSection.cpp:429: m_rowPos[rowIndex + rowSpan]
-= extraHeightToPropagate;
*sigh*, this is called wac-a-molling. The minute you add an extra continue
somewhere in this loop, we have another bug.

You landed the change with the let-s-add-and-remove-the-same-quantity which is
super dumb and with this bug, it really prompts the question of whether the
approach couldn't be improved.

[a.suchit, 2013-08-07 07:28:48]

It have fixed as I discussed with you on IRC.
Now, This will not give any side effect if any changes will happen in future.

[Julien - ping for review, 2013-08-07 16:51:30]

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

https://codereview.chromium.org/22415003/diff/7001/Source/core/rendering/Rend...
Source/core/rendering/RenderTableSection.cpp:421: if ((rowIndex + rowSpan) ==
(lastRowIndex + lastRowSpan) || ((rowIndex + rowSpan) >= (m_grid.size() + 1) &&
(lastRowIndex + lastRowSpan) >= (m_grid.size() + 1)))
Do we really need to check if we have cells protuding out of the section?
RenderTableSection::addCell should make sure this never happens.

[a.suchit, 2013-08-07 19:49:50]

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

https://codereview.chromium.org/22415003/diff/7001/Source/core/rendering/Rend...
Source/core/rendering/RenderTableSection.cpp:421: if ((rowIndex + rowSpan) ==
(lastRowIndex + lastRowSpan) || ((rowIndex + rowSpan) >= (m_grid.size() + 1) &&
(lastRowIndex + lastRowSpan) >= (m_grid.size() + 1)))
On 2013/08/07 16:51:30, Julien Chaffraix wrote:
> Do we really need to check if we have cells protuding out of the section?
> RenderTableSection::addCell should make sure this never happens.

You are right that addCell will handle it.
I have done wrong here, This point did not come to me.
We need to check that next row is valid or not. Row is not valid means rows with
all invalid cells.
Here, I need to check that which is the last row is valid row in the table.

[a.suchit, 2013-08-08 11:17:40]

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

https://codereview.chromium.org/22415003/diff/7001/Source/core/rendering/Rend...
Source/core/rendering/RenderTableSection.cpp:421: if ((rowIndex + rowSpan) ==
(lastRowIndex + lastRowSpan) || ((rowIndex + rowSpan) >= (m_grid.size() + 1) &&
(lastRowIndex + lastRowSpan) >= (m_grid.size() + 1)))
On 2013/08/07 19:49:50, a.suchit wrote:
> On 2013/08/07 16:51:30, Julien Chaffraix wrote:
> > Do we really need to check if we have cells protuding out of the section?
> > RenderTableSection::addCell should make sure this never happens.
> 
> You are right that addCell will handle it.
> I have done wrong here, This point did not come to me.
> We need to check that next row is valid or not. Row is not valid means rows
with
> all invalid cells.
> Here, I need to check that which is the last row is valid row in the table.

I checked it. What problem was in my mind that will not occur. Thanks

[Julien - ping for review, 2013-08-08 16:42:02]

lgtm

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

https://codereview.chromium.org/22415003/diff/15001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:419: if ((rowIndex + rowSpan) ==
(lastRowIndex + lastRowSpan))
The parentheses are unneeded due to operator precedence.

On top of that we now check repeatedly for rowIndex + rowSpan and lastRowIndex +
lastRowSpan which should be given variables with meaningful names.

[commit-bot: I haz the power, 2013-08-08 17:02:46]

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

[Julien - ping for review, 2013-08-08 17:09:06]

On 2013/08/08 17:02:46, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/a.suchit%40samsung.com/22415003/15001

Please fix the patch before landing, the comments are not nits. :-/

[a.suchit, 2013-08-08 17:32:44]

I need to just remove the parenthesis before landing right?

I did not get about below comment :
On top of that we now check repeatedly for rowIndex + rowSpan and lastRowIndex +
lastRowSpan which should be given variables with meaningful names.

[Julien - ping for review, 2013-08-08 17:45:29]

> I need to just remove the parenthesis before landing right?

That would be a minimum.

> I did not get about below comment :
> On top of that we now check repeatedly for rowIndex + rowSpan and lastRowIndex
+
> lastRowSpan which should be given variables with meaningful names.

Look at the code, (rowIndex + rowSpan) is used several times in the loop so it
should be assigned to a variable. Same for (lastRowIndex + lastRowSpan).

[a.suchit, 2013-08-08 18:13:56]

On 2013/08/08 17:45:29, Julien Chaffraix wrote:
> > I need to just remove the parenthesis before landing right?
> 
> That would be a minimum.
> 
> > I did not get about below comment :
> > On top of that we now check repeatedly for rowIndex + rowSpan and
lastRowIndex
> +
> > lastRowSpan which should be given variables with meaningful names.
> 
> Look at the code, (rowIndex + rowSpan) is used several times in the loop so it
> should be assigned to a variable. Same for (lastRowIndex + lastRowSpan).

(rowIndex + rowSpan) is present 4 to 5 time but (lastRowIndex + lastRowSpan) is
present only 2 times in loop.
I thing right now these statements are more clear.
What is your opinion?
if yes then I will lastSpanningCellEndIndex for (lastRowIndex + lastRowSpan) and
spanningCellEndIndex for (rowIndex + rowSpan).
Are these ok?

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

On 2013/08/08 18:13:56, a.suchit wrote:
> On 2013/08/08 17:45:29, Julien Chaffraix wrote:
> > > I need to just remove the parenthesis before landing right?
> > 
> > That would be a minimum.
> > 
> > > I did not get about below comment :
> > > On top of that we now check repeatedly for rowIndex + rowSpan and
> lastRowIndex
> > +
> > > lastRowSpan which should be given variables with meaningful names.
> > 
> > Look at the code, (rowIndex + rowSpan) is used several times in the loop so
it
> > should be assigned to a variable. Same for (lastRowIndex + lastRowSpan).
> 
> (rowIndex + rowSpan) is present 4 to 5 time but (lastRowIndex + lastRowSpan)
is
> present only 2 times in loop.
> I thing right now these statements are more clear.
> What is your opinion?

I already stated my opinion (now 3 times): let's just add these variables. Code
duplication is almost *never* a good idea, be it a single variable, a code block
or an entire function.

> if yes then I will lastSpanningCellEndIndex for (lastRowIndex + lastRowSpan)
and
> spanningCellEndIndex for (rowIndex + rowSpan).
> Are these ok?

SGTM.

[commit-bot: I haz the power, 2013-08-08 19:07:45]

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

[Julien - ping for review, 2013-08-08 20:08:42]

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

https://codereview.chromium.org/22415003/diff/30001/Source/core/rendering/Ren...
Source/core/rendering/RenderTableSection.cpp:410: unsigned
lastSpanningCellEndBoundry = lastRowIndex + lastRowSpan;
typo: "boundry" is not an English word, "boundary" is. Can we stick with index
as the spelling is easier?

[commit-bot: I haz the power, 2013-08-08 22:42:49]

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

[commit-bot: I haz the power, 2013-08-09 02:27:05]

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

[commit-bot: I haz the power, 2013-08-09 05:24:30]

Change committed as 155825