Optimize number of rowspan cells required to distribute their height in rows.

Optimize number of rowspan cells required to distribute their height in rows.

Rowspan cell duplicates were present in rowSpanCells list when when rowspan
cell have collspan value more than 1.

Now, rowspan cell added only once in the rowSpanCells list.

R=jchaffraix@chromium.org

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

[eseidel, 2013-09-16 14:42:59]

Is this a behavioral change?  Can we test this with a layout test?

[a.suchit, 2013-09-16 17:50:34]

On 2013/09/16 14:42:59, eseidel wrote:
> Is this a behavioral change?  Can we test this with a layout test?

There is no changes in behavior. It is just reducing the number of rowspan cells
need to process in distributing rowspan cell's height in rows.

[a.suchit, 2013-09-16 17:54:15]

On 2013/09/16 14:42:59, eseidel wrote:
> Is this a behavioral change?  Can we test this with a layout test?

I thing, no use of layout test for it because it will not show any different
output.

Can you help me that wat type of test case would be useful for it?

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

lgtm, your optimization is far from perfect though and could be improved by
using a HashSet + copyToVector to offer hard guarantees (but this would imply
losing the initial Vector's DOM ordering though).

In any case, no change in behavior is intended so you can't test it.

The description is OK but there are some verbs that are not conjugated properly
like "duplicates was" [were] and "row cell is adding" [added].

[commit-bot: I haz the power, 2013-09-16 21:16:51]

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

[suchit.agrawal, 2013-09-16 22:05:10]

is this change ok for uniqueness checking?

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

https://codereview.chromium.org/23526051/diff/1/Source/core/rendering/RenderT...
Source/core/rendering/RenderTableSection.cpp:530: SpanningRenderTableCells
rowSpanCells;
#ifndef NDEBUG
HashSet<const RenderTableCell*> uniqueCells;
#endif

https://codereview.chromium.org/23526051/diff/1/Source/core/rendering/RenderT...
Source/core/rendering/RenderTableSection.cpp:554: rowSpanCells.append(cell);
#ifndef NDEBUG
ASSERT(!uniqueCells.contains(cell));
uniqueCell.append(cell);
#endif

[Julien - ping for review, 2013-09-16 23:03:02]

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

https://codereview.chromium.org/23526051/diff/1/Source/core/rendering/RenderT...
Source/core/rendering/RenderTableSection.cpp:530: SpanningRenderTableCells
rowSpanCells;
On 2013/09/16 22:05:11, suchit.agrawal wrote:
> #ifndef NDEBUG
> HashSet<const RenderTableCell*> uniqueCells;
> #endif

Not here...

https://codereview.chromium.org/23526051/diff/1/Source/core/rendering/RenderT...
Source/core/rendering/RenderTableSection.cpp:554: rowSpanCells.append(cell);
On 2013/09/16 22:05:11, suchit.agrawal wrote:
> #ifndef NDEBUG
HashSet<const RenderTableCell*> uniqueCells;
... but here we define variables when we need them. ^^^
> ASSERT(!uniqueCells.contains(cell));
> uniqueCell.append(cell);
> #endif

[a.suchit, 2013-09-17 00:44:11]

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

https://codereview.chromium.org/23526051/diff/1/Source/core/rendering/RenderT...
Source/core/rendering/RenderTableSection.cpp:554: rowSpanCells.append(cell);
On 2013/09/16 23:03:02, Julien Chaffraix wrote:
> On 2013/09/16 22:05:11, suchit.agrawal wrote:
> > #ifndef NDEBUG
> HashSet<const RenderTableCell*> uniqueCells;
> ... but here we define variables when we need them. ^^^
> > ASSERT(!uniqueCells.contains(cell));
> > uniqueCell.append(cell);
> > #endif
> 
It would be local variable here. How will it work here?

[commit-bot: I haz the power, 2013-09-17 05:53:21]

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

[commit-bot: I haz the power, 2013-09-17 06:44:51]

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

[commit-bot: I haz the power, 2013-09-17 09:01:09]

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_...

[commit-bot: I haz the power, 2013-09-17 09:01:44]

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

[commit-bot: I haz the power, 2013-09-17 09:53:05]

Change committed as 157891