Made varied number of cells in each row based on row's requirement.

Made varied number of cells in each row based on row's requirement.

Rowspan-only-row(s) was not getting height when there is at least one
empty cell present.

Number of the cells are created in the row based on the required
column in that row. So Empty cells would not be present at the end
of the row.

R=jchaffraix@chromium.org, mstensho@opera.com

BUG=396653
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/1060f916d9f7026fa22dd3bb6b563294ec7f8224
Cr-Commit-Position: refs/heads/master@{#441620}

[a.suchit2, 2016-12-01 06:30:59]

a.suchit@chromium.org changed reviewers:
+ a.suchit@chromium.org

[a.suchit2, 2016-12-01 06:31:00]

Please review.

[suchit.agrawal, 2016-12-01 06:38:25]

suchit.agrawal@gmail.com changed reviewers:
+ suchit.agrawal@gmail.com

[suchit.agrawal, 2016-12-01 06:38:25]

Some behavior of the table (in rowspan case) is changed but changed behavior is
matching with the Mozilla.

[a.suchit, 2016-12-01 06:55:05]

a.suchit@samsung.com changed reviewers:
+ eae@chromium.org
- a.suchit@chromium.org, suchit.agrawal@gmail.com

[mstensho (USE GERRIT), 2016-12-01 08:37:38]

Please add a minimal test for this.

[a.suchit, 2016-12-01 09:35:09]

On 2016/12/01 08:37:38, mstensho wrote:
> Please add a minimal test for this.

Ohh sorry. I will do it.

[suchit.agrawal, 2016-12-02 00:51:25]

suchit.agrawal@gmail.com changed reviewers:
+ suchit.agrawal@gmail.com

[suchit.agrawal, 2016-12-02 00:51:25]

Layout test added please review. Thanks

[mstensho (USE GERRIT), 2016-12-02 09:01:02]

On 2016/12/02 00:51:25, suchit.agrawal wrote:
> Layout test added please review. Thanks

Please write a reftest or a testharness test with a clear pass condition. No PNG
tests, unless that's strictly necessary.

Please also make it a little more "minimal".

[a.suchit, 2016-12-02 11:24:00]

On 2016/12/02 09:01:02, mstensho wrote:
> On 2016/12/02 00:51:25, suchit.agrawal wrote:
> > Layout test added please review. Thanks
> 
> Please write a reftest or a testharness test with a clear pass condition. No
PNG
> tests, unless that's strictly necessary.
> 
> Please also make it a little more "minimal".

I did not reduce the test case because removing the cells in the rows in
specific strategy gives me different results.
Yes, I can reduce one OR two test case but that may increase the size of script.
All tables in the test are same but give different results based on removed
columns in the rows.

[mstensho (USE GERRIT), 2016-12-02 11:47:34]

On 2016/12/02 11:24:00, a.suchit wrote:
> On 2016/12/02 09:01:02, mstensho wrote:
> > On 2016/12/02 00:51:25, suchit.agrawal wrote:
> > > Layout test added please review. Thanks
> > 
> > Please write a reftest or a testharness test with a clear pass condition. No
> PNG
> > tests, unless that's strictly necessary.
> > 
> > Please also make it a little more "minimal".
> 
> I did not reduce the test case because removing the cells in the rows in
> specific strategy gives me different results.
> Yes, I can reduce one OR two test case but that may increase the size of
script.
> All tables in the test are same but give different results based on removed
> columns in the rows.

I would like to see one simple test that demonstrates what you're fixing.

One table per test would be nice. One test should be enough. If you feel that
you need more coverage than that, please create multiple tests in multiple
files.

Writing a test in such a way that it's possible to provide a textual pass
condition for a human to look at the output and see if it's right or wrong would
also be great.
E.g. "There should be a green square below."

[a.suchit, 2016-12-02 12:29:44]

On 2016/12/02 11:47:34, mstensho wrote:
> On 2016/12/02 11:24:00, a.suchit wrote:
> > On 2016/12/02 09:01:02, mstensho wrote:
> > > On 2016/12/02 00:51:25, suchit.agrawal wrote:
> > > > Layout test added please review. Thanks
> > > 
> > > Please write a reftest or a testharness test with a clear pass condition.
No
> > PNG
> > > tests, unless that's strictly necessary.
> > > 
> > > Please also make it a little more "minimal".
> > 
> > I did not reduce the test case because removing the cells in the rows in
> > specific strategy gives me different results.
> > Yes, I can reduce one OR two test case but that may increase the size of
> script.
> > All tables in the test are same but give different results based on removed
> > columns in the rows.
> 
> I would like to see one simple test that demonstrates what you're fixing.
> 
> One table per test would be nice. One test should be enough. If you feel that
> you need more coverage than that, please create multiple tests in multiple
> files.
> 
> Writing a test in such a way that it's possible to provide a textual pass
> condition for a human to look at the output and see if it's right or wrong
would
> also be great.
> E.g. "There should be a green square below."

Please check  if any more improvement required. Thanks :)

[a.suchit, 2016-12-06 12:35:44]

Please review.

[mstensho (USE GERRIT), 2016-12-06 12:41:06]

On 2016/12/06 12:35:44, a.suchit wrote:
> Please review.

Sorry about the delay. This patch is quite heavy, so I need to spend some time
on it. I'll get back to it as soon as I can. Right now I'm occupied with a beta
blocker.

I'll start a dry run, to see if the bots have anything to say about it in the
meantime.

[mstensho (USE GERRIT), 2016-12-06 12:41:14]

The CQ bit was checked by mstensho@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 12:41:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-06 14:16:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-06 14:16:10]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[mstensho (USE GERRIT), 2016-12-06 23:39:22]

Seems to me that the root cause of the problem is that
LayoutTable::numEffectiveColumns() stays at 3, even if you have removed one cell
from each row. This in turn gives a 3x3 m_grid in LayoutTableSection just like
before the change, even though each row now only contains 2 columns, after the
cell removals.

I think that's what needs to be fixed.

[a.suchit, 2016-12-08 05:45:24]

On 2016/12/06 23:39:22, mstensho wrote:
> Seems to me that the root cause of the problem is that
> LayoutTable::numEffectiveColumns() stays at 3, even if you have removed one
cell
> from each row. This in turn gives a 3x3 m_grid in LayoutTableSection just like
> before the change, even though each row now only contains 2 columns, after the
> cell removals.
> 
> I think that's what needs to be fixed.

I updated the patch, It just need some of the layout test to be re-baseline
mostly for mac and win. Linux one, I have done it.

[a.suchit, 2016-12-08 06:06:22]

The CQ bit was checked by a.suchit@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 06:06:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 06:09:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-08 06:09:20]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[a.suchit, 2016-12-08 06:27:04]

The CQ bit was checked by a.suchit@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 06:27:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mstensho (USE GERRIT), 2016-12-08 07:42:46]

On 2016/12/08 05:45:24, a.suchit wrote:
> On 2016/12/06 23:39:22, mstensho wrote:
> > Seems to me that the root cause of the problem is that
> > LayoutTable::numEffectiveColumns() stays at 3, even if you have removed one
> cell
> > from each row. This in turn gives a 3x3 m_grid in LayoutTableSection just
like
> > before the change, even though each row now only contains 2 columns, after
the
> > cell removals.
> > 
> > I think that's what needs to be fixed.
> 
> I updated the patch, It just need some of the layout test to be re-baseline
> mostly for mac and win. Linux one, I have done it.

Looks like you didn't address my issue about numEffectiveColumns() being the
root cause.

[commit-bot: I haz the power, 2016-12-08 08:50:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-08 08:50:25]

Dry run: This issue passed the CQ dry run.

[a.suchit, 2016-12-08 10:34:52]

On 2016/12/08 07:42:46, mstensho wrote:
> On 2016/12/08 05:45:24, a.suchit wrote:
> > On 2016/12/06 23:39:22, mstensho wrote:
> > > Seems to me that the root cause of the problem is that
> > > LayoutTable::numEffectiveColumns() stays at 3, even if you have removed
one
> > cell
> > > from each row. This in turn gives a 3x3 m_grid in LayoutTableSection just
> like
> > > before the change, even though each row now only contains 2 columns, after
> the
> > > cell removals.
> > > 
> > > I think that's what needs to be fixed.
> > 
> > I updated the patch, It just need some of the layout test to be re-baseline
> > mostly for mac and win. Linux one, I have done it.
> 
> Looks like you didn't address my issue about numEffectiveColumns() being the
> root cause.

Sorry, But rowspan height distribution does not have any relation with function
"numEffectiveColumns()". It is not used in the rowspan height distribution.

[mstensho (USE GERRIT), 2016-12-08 10:39:48]

On 2016/12/08 10:34:52, a.suchit wrote:
> On 2016/12/08 07:42:46, mstensho wrote:
> > On 2016/12/08 05:45:24, a.suchit wrote:
> > > On 2016/12/06 23:39:22, mstensho wrote:
> > > > Seems to me that the root cause of the problem is that
> > > > LayoutTable::numEffectiveColumns() stays at 3, even if you have removed
> one
> > > cell
> > > > from each row. This in turn gives a 3x3 m_grid in LayoutTableSection
just
> > like
> > > > before the change, even though each row now only contains 2 columns,
after
> > the
> > > > cell removals.
> > > > 
> > > > I think that's what needs to be fixed.
> > > 
> > > I updated the patch, It just need some of the layout test to be
re-baseline
> > > mostly for mac and win. Linux one, I have done it.
> > 
> > Looks like you didn't address my issue about numEffectiveColumns() being the
> > root cause.
> 
> Sorry, But rowspan height distribution does not have any relation with
function
> "numEffectiveColumns()". It is not used in the rowspan height distribution.

Look, I even tried this, after having debugged (which is when I realized that
m_grid was still 3x3 after the layout object removals, although we clearly only
had 2 columns). Making sure that LayoutTable::recalcSections() rebuilds
m_effectiveColumns makes the problem go away. You are not addressing the root
cause of the problem.

[a.suchit, 2016-12-08 10:53:43]

On 2016/12/08 10:39:48, mstensho wrote:
> On 2016/12/08 10:34:52, a.suchit wrote:
> > On 2016/12/08 07:42:46, mstensho wrote:
> > > On 2016/12/08 05:45:24, a.suchit wrote:
> > > > On 2016/12/06 23:39:22, mstensho wrote:
> > > > > Seems to me that the root cause of the problem is that
> > > > > LayoutTable::numEffectiveColumns() stays at 3, even if you have
removed
> > one
> > > > cell
> > > > > from each row. This in turn gives a 3x3 m_grid in LayoutTableSection
> just
> > > like
> > > > > before the change, even though each row now only contains 2 columns,
> after
> > > the
> > > > > cell removals.
> > > > > 
> > > > > I think that's what needs to be fixed.
> > > > 
> > > > I updated the patch, It just need some of the layout test to be
> re-baseline
> > > > mostly for mac and win. Linux one, I have done it.
> > > 
> > > Looks like you didn't address my issue about numEffectiveColumns() being
the
> > > root cause.
> > 
> > Sorry, But rowspan height distribution does not have any relation with
> function
> > "numEffectiveColumns()". It is not used in the rowspan height distribution.
> 
> Look, I even tried this, after having debugged (which is when I realized that
> m_grid was still 3x3 after the layout object removals, although we clearly
only
> had 2 columns). Making sure that LayoutTable::recalcSections() rebuilds
> m_effectiveColumns makes the problem go away. You are not addressing the root
> cause of the problem.

Fixing that problem is not easy task.
It will be fixed when call come from script for removing the child.
In current code, It does not remove the cells but in place of that it delete the
content of the cell and cell becomes empty.

[a.suchit, 2016-12-08 11:08:34]

On 2016/12/08 10:53:43, a.suchit wrote:
> On 2016/12/08 10:39:48, mstensho wrote:
> > On 2016/12/08 10:34:52, a.suchit wrote:
> > > On 2016/12/08 07:42:46, mstensho wrote:
> > > > On 2016/12/08 05:45:24, a.suchit wrote:
> > > > > On 2016/12/06 23:39:22, mstensho wrote:
> > > > > > Seems to me that the root cause of the problem is that
> > > > > > LayoutTable::numEffectiveColumns() stays at 3, even if you have
> removed
> > > one
> > > > > cell
> > > > > > from each row. This in turn gives a 3x3 m_grid in LayoutTableSection
> > just
> > > > like
> > > > > > before the change, even though each row now only contains 2 columns,
> > after
> > > > the
> > > > > > cell removals.
> > > > > > 
> > > > > > I think that's what needs to be fixed.
> > > > > 
> > > > > I updated the patch, It just need some of the layout test to be
> > re-baseline
> > > > > mostly for mac and win. Linux one, I have done it.
> > > > 
> > > > Looks like you didn't address my issue about numEffectiveColumns() being
> the
> > > > root cause.
> > > 
> > > Sorry, But rowspan height distribution does not have any relation with
> > function
> > > "numEffectiveColumns()". It is not used in the rowspan height
distribution.
> > 
> > Look, I even tried this, after having debugged (which is when I realized
that
> > m_grid was still 3x3 after the layout object removals, although we clearly
> only
> > had 2 columns). Making sure that LayoutTable::recalcSections() rebuilds
> > m_effectiveColumns makes the problem go away. You are not addressing the
root
> > cause of the problem.
> 
> Fixing that problem is not easy task.
> It will be fixed when call come from script for removing the child.
> In current code, It does not remove the cells but in place of that it delete
the
> content of the cell and cell becomes empty.

I can look for that but right now, I do not know, What would be sde effect will
code in current implementation after that change.
IMO, Some of the code may become redundant after that change. and some required
to be change in layoutTableSection.cpp and may be some other files also.

[mstensho (USE GERRIT), 2016-12-08 11:36:46]

On 2016/12/08 11:08:34, a.suchit wrote:
> On 2016/12/08 10:53:43, a.suchit wrote:
> > On 2016/12/08 10:39:48, mstensho wrote:
> > > On 2016/12/08 10:34:52, a.suchit wrote:
> > > > On 2016/12/08 07:42:46, mstensho wrote:
> > > > > On 2016/12/08 05:45:24, a.suchit wrote:
> > > > > > On 2016/12/06 23:39:22, mstensho wrote:
> > > > > > > Seems to me that the root cause of the problem is that
> > > > > > > LayoutTable::numEffectiveColumns() stays at 3, even if you have
> > removed
> > > > one
> > > > > > cell
> > > > > > > from each row. This in turn gives a 3x3 m_grid in
LayoutTableSection
> > > just
> > > > > like
> > > > > > > before the change, even though each row now only contains 2
columns,
> > > after
> > > > > the
> > > > > > > cell removals.
> > > > > > > 
> > > > > > > I think that's what needs to be fixed.
> > > > > > 
> > > > > > I updated the patch, It just need some of the layout test to be
> > > re-baseline
> > > > > > mostly for mac and win. Linux one, I have done it.
> > > > > 
> > > > > Looks like you didn't address my issue about numEffectiveColumns()
being
> > the
> > > > > root cause.
> > > > 
> > > > Sorry, But rowspan height distribution does not have any relation with
> > > function
> > > > "numEffectiveColumns()". It is not used in the rowspan height
> distribution.
> > > 
> > > Look, I even tried this, after having debugged (which is when I realized
> that
> > > m_grid was still 3x3 after the layout object removals, although we clearly
> > only
> > > had 2 columns). Making sure that LayoutTable::recalcSections() rebuilds
> > > m_effectiveColumns makes the problem go away. You are not addressing the
> root
> > > cause of the problem.
> > 
> > Fixing that problem is not easy task.
> > It will be fixed when call come from script for removing the child.
> > In current code, It does not remove the cells but in place of that it delete
> the
> > content of the cell and cell becomes empty.
> 
> I can look for that but right now, I do not know, What would be sde effect
will
> code in current implementation after that change.

Having the correct column count at any given time would be a good thing, right?

> IMO, Some of the code may become redundant after that change. and some
required
> to be change in layoutTableSection.cpp and may be some other files also.

All the code in this CL would be redundant, I guess.

[a.suchit, 2016-12-19 07:15:30]

On 2016/12/08 11:36:46, mstensho wrote:
> On 2016/12/08 11:08:34, a.suchit wrote:
> > On 2016/12/08 10:53:43, a.suchit wrote:
> > > On 2016/12/08 10:39:48, mstensho wrote:
> > > > On 2016/12/08 10:34:52, a.suchit wrote:
> > > > > On 2016/12/08 07:42:46, mstensho wrote:
> > > > > > On 2016/12/08 05:45:24, a.suchit wrote:
> > > > > > > On 2016/12/06 23:39:22, mstensho wrote:
> > > > > > > > Seems to me that the root cause of the problem is that
> > > > > > > > LayoutTable::numEffectiveColumns() stays at 3, even if you have
> > > removed
> > > > > one
> > > > > > > cell
> > > > > > > > from each row. This in turn gives a 3x3 m_grid in
> LayoutTableSection
> > > > just
> > > > > > like
> > > > > > > > before the change, even though each row now only contains 2
> columns,
> > > > after
> > > > > > the
> > > > > > > > cell removals.
> > > > > > > > 
> > > > > > > > I think that's what needs to be fixed.
> > > > > > > 
> > > > > > > I updated the patch, It just need some of the layout test to be
> > > > re-baseline
> > > > > > > mostly for mac and win. Linux one, I have done it.
> > > > > > 
> > > > > > Looks like you didn't address my issue about numEffectiveColumns()
> being
> > > the
> > > > > > root cause.
> > > > > 
> > > > > Sorry, But rowspan height distribution does not have any relation with
> > > > function
> > > > > "numEffectiveColumns()". It is not used in the rowspan height
> > distribution.
> > > > 
> > > > Look, I even tried this, after having debugged (which is when I realized
> > that
> > > > m_grid was still 3x3 after the layout object removals, although we
clearly
> > > only
> > > > had 2 columns). Making sure that LayoutTable::recalcSections() rebuilds
> > > > m_effectiveColumns makes the problem go away. You are not addressing the
> > root
> > > > cause of the problem.
> > > 
> > > Fixing that problem is not easy task.
> > > It will be fixed when call come from script for removing the child.
> > > In current code, It does not remove the cells but in place of that it
delete
> > the
> > > content of the cell and cell becomes empty.
> > 
> > I can look for that but right now, I do not know, What would be sde effect
> will
> > code in current implementation after that change.
> 
> Having the correct column count at any given time would be a good thing,
right?
> 
> > IMO, Some of the code may become redundant after that change. and some
> required
> > to be change in layoutTableSection.cpp and may be some other files also.
> 
> All the code in this CL would be redundant, I guess.

I will take some time to updates about our discussion. Busy in some other work.

[mstensho (USE GERRIT), 2016-12-20 08:57:50]

On 2016/12/19 07:15:30, a.suchit wrote:
> I will take some time to updates about our discussion. Busy in some other
work.

OK.

[a.suchit, 2016-12-27 11:03:08]

Description was changed from

==========
Changed the logic behind height given to rowspan-only-row(s).

Rowspan-only-row(s) was not getting height when there is at least one
empty cell present.

Height is given to the rowspan-only-row(s) also even when
rowspan-only-row(s) contains empty cell(s) But this rowspan-only-row(s)
should not be part of top most and bottom most rowspan-only-row(s).

R=jchaffraix@chromium.org, mstensho@opera.com

BUG=396653
==========

to

==========
Changed the logic behind height given to rowspan-only-row(s).

Rowspan-only-row(s) was not getting height when there is at least one
empty cell present.

Height is given to the rowspan-only-row(s) also even when
rowspan-only-row(s) contains empty cell(s) But this rowspan-only-row(s)
should not be part of top most and bottom most rowspan-only-row(s).

R=jchaffraix@chromium.org, mstensho@opera.com

BUG=396653
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[a.suchit, 2016-12-27 11:03:50]

The CQ bit was checked by a.suchit@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-27 11:03:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[a.suchit, 2016-12-27 11:22:04]

Description was changed from

==========
Changed the logic behind height given to rowspan-only-row(s).

Rowspan-only-row(s) was not getting height when there is at least one
empty cell present.

Height is given to the rowspan-only-row(s) also even when
rowspan-only-row(s) contains empty cell(s) But this rowspan-only-row(s)
should not be part of top most and bottom most rowspan-only-row(s).

R=jchaffraix@chromium.org, mstensho@opera.com

BUG=396653
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Made varied number of cells in each row based on row's requirement.

Rowspan-only-row(s) was not getting height when there is at least one
empty cell present.

Number of the cells are created in the row based on the required
column in that row. So Emtpy cells would not be present at the end
of the row.

R=jchaffraix@chromium.org, mstensho@opera.com

BUG=396653
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[a.suchit, 2016-12-27 11:51:27]

The CQ bit was checked by a.suchit@samsung.com

[a.suchit, 2016-12-27 11:51:29]

The CQ bit was unchecked by a.suchit@samsung.com

[a.suchit, 2016-12-27 11:51:43]

The CQ bit was checked by a.suchit@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-27 11:51:53]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-27 13:44:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-27 13:44:31]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[a.suchit, 2016-12-29 04:10:59]

The CQ bit was checked by a.suchit@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-29 04:11:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-29 05:30:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-29 05:30:27]

Dry run: This issue passed the CQ dry run.

[a.suchit, 2016-12-29 05:30:45]

On 2016/12/20 08:57:50, mstensho wrote:
> On 2016/12/19 07:15:30, a.suchit wrote:
> > I will take some time to updates about our discussion. Busy in some other
> work.
> 
> OK.

Please review. Thanks

[mstensho (USE GERRIT), 2017-01-03 15:18:41]

Typo in description: "Emtpy"

https://codereview.chromium.org/2534413004/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2534413004/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:2414: # Affter Landing patch
https://codereview.chromium.org/2534413004/ below tests should rebaseline
Better rebaseline them right away, as part of this CL.

See
https://groups.google.com/a/chromium.org/forum/#!msg/blink-infra/rRjDVJab9zQ/...
for instructions, if you have never tried this before.

I'm talking about the "webkit-patch rebaseline-cl" approach mentioned there.

https://codereview.chromium.org/2534413004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right):

https://codereview.chromium.org/2534413004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:213: void
LayoutTableSection::ensureRows(unsigned numRows) {
Could make this inline now, just like ensureCol() is.

https://codereview.chromium.org/2534413004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1376: if (c >=
numCols(r))
Might just as well clamp totalCols to numCols(r), so that we don't need this
check.

https://codereview.chromium.org/2534413004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1819: continue;
Should break instead.

https://codereview.chromium.org/2534413004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.h (right):

https://codereview.chromium.org/2534413004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.h:354: void
ensureCol(unsigned r, unsigned c) {
Maybe it should be named ensureCols(), since we already have ensureRows()? Also,
I wonder if the "c" parameter should be column count, rather than column index,
since ensureRows() takes row count, not row index. And how about more
descriptive parameter names, such as rowIndex and numCols?

https://codereview.chromium.org/2534413004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/TableSectionPainter.cpp (right):

https://codereview.chromium.org/2534413004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:307:
m_layoutTableSection.cellAt(r, c);
Aren't we forgetting to check that 'c' is within bounds here?

[a.suchit, 2017-01-04 10:32:33]

Description was changed from

==========
Made varied number of cells in each row based on row's requirement.

Rowspan-only-row(s) was not getting height when there is at least one
empty cell present.

Number of the cells are created in the row based on the required
column in that row. So Emtpy cells would not be present at the end
of the row.

R=jchaffraix@chromium.org, mstensho@opera.com

BUG=396653
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Made varied number of cells in each row based on row's requirement.

Rowspan-only-row(s) was not getting height when there is at least one
empty cell present.

Number of the cells are created in the row based on the required
column in that row. So Empty cells would not be present at the end
of the row.

R=jchaffraix@chromium.org, mstensho@opera.com

BUG=396653
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[a.suchit, 2017-01-04 10:35:19]

https://codereview.chromium.org/2534413004/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2534413004/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:2414: # Affter Landing patch
https://codereview.chromium.org/2534413004/ below tests should rebaseline
On 2017/01/03 15:18:41, mstensho wrote:
> Better rebaseline them right away, as part of this CL.
> 
> See
>
https://groups.google.com/a/chromium.org/forum/#!msg/blink-infra/rRjDVJab9zQ/...
> for instructions, if you have never tried this before.
> 
> I'm talking about the "webkit-patch rebaseline-cl" approach mentioned there.

Working on it.

https://codereview.chromium.org/2534413004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right):

https://codereview.chromium.org/2534413004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:213: void
LayoutTableSection::ensureRows(unsigned numRows) {
On 2017/01/03 15:18:41, mstensho wrote:
> Could make this inline now, just like ensureCol() is.

Done.

https://codereview.chromium.org/2534413004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1376: if (c >=
numCols(r))
On 2017/01/03 15:18:41, mstensho wrote:
> Might just as well clamp totalCols to numCols(r), so that we don't need this
> check.

Done.

https://codereview.chromium.org/2534413004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1819: continue;
On 2017/01/03 15:18:41, mstensho wrote:
> Should break instead.

Done.

https://codereview.chromium.org/2534413004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.h (right):

https://codereview.chromium.org/2534413004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.h:354: void
ensureCol(unsigned r, unsigned c) {
On 2017/01/03 15:18:41, mstensho wrote:
> Maybe it should be named ensureCols(), since we already have ensureRows()?
Also,
> I wonder if the "c" parameter should be column count, rather than column
index,
> since ensureRows() takes row count, not row index. And how about more
> descriptive parameter names, such as rowIndex and numCols?

Done.

https://codereview.chromium.org/2534413004/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/paint/TableSectionPainter.cpp (right):

https://codereview.chromium.org/2534413004/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/paint/TableSectionPainter.cpp:307:
m_layoutTableSection.cellAt(r, c);
On 2017/01/03 15:18:41, mstensho wrote:
> Aren't we forgetting to check that 'c' is within bounds here?

Done.

[mstensho (USE GERRIT), 2017-01-04 10:56:06]

https://codereview.chromium.org/2534413004/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right):

https://codereview.chromium.org/2534413004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1742:
ensureCols(row, pos + 1 + 1);
pos + 2, perhaps? :)

https://codereview.chromium.org/2534413004/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.h (right):

https://codereview.chromium.org/2534413004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.h:358: if (numCols >
LayoutTableSection::numCols(rowIndex))
Could use this->numCols() instead.

[a.suchit, 2017-01-05 03:25:53]

The CQ bit was checked by a.suchit@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 03:26:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 03:28:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-05 03:28:29]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[a.suchit, 2017-01-05 03:30:56]

The CQ bit was checked by a.suchit@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-05 03:31:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[a.suchit, 2017-01-05 03:34:31]

https://codereview.chromium.org/2534413004/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right):

https://codereview.chromium.org/2534413004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1742:
ensureCols(row, pos + 1 + 1);
On 2017/01/04 10:56:06, mstensho wrote:
> pos + 2, perhaps? :)

Done.

https://codereview.chromium.org/2534413004/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.h (right):

https://codereview.chromium.org/2534413004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.h:358: if (numCols >
LayoutTableSection::numCols(rowIndex))
On 2017/01/04 10:56:06, mstensho wrote:
> Could use this->numCols() instead.

Done.

[a.suchit, 2017-01-05 03:35:47]

https://codereview.chromium.org/2534413004/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2534413004/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:2414: # Affter Landing patch
https://codereview.chromium.org/2534413004/ below tests should rebaseline
On 2017/01/03 15:18:41, mstensho wrote:
> Better rebaseline them right away, as part of this CL.
> 
> See
>
https://groups.google.com/a/chromium.org/forum/#!msg/blink-infra/rRjDVJab9zQ/...
> for instructions, if you have never tried this before.
> 
> I'm talking about the "webkit-patch rebaseline-cl" approach mentioned there.

Done.

[a.suchit, 2017-01-05 03:47:07]

third_party/WebKit/LayoutTests/fast/table/table-all-rowspans-height-distribution-in-rows.html
third_party/WebKit/LayoutTests/fast/table/table-all-rowspans-height-distribution-in-rows-except-overlapped.html
third_party/WebKit/LayoutTests/fast/table/table-rowspan-height-distribution-in-rows-1.html
third_party/WebKit/LayoutTests/fast/table/table-rowspan-height-distribution-in-rows-2.html

These test files are set based on the linux platform and same works for the
windows also.
But some test in these files fails for mac platform.
We should have separate test files for mac platform a well. and above should not
run for mac.

[commit-bot: I haz the power, 2017-01-05 04:56:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-05 04:56:40]

Dry run: This issue passed the CQ dry run.

[mstensho (USE GERRIT), 2017-01-05 10:41:18]

lgtm!

On 2017/01/05 03:47:07, a.suchit wrote:
>
third_party/WebKit/LayoutTests/fast/table/table-all-rowspans-height-distribution-in-rows.html
>
third_party/WebKit/LayoutTests/fast/table/table-all-rowspans-height-distribution-in-rows-except-overlapped.html
>
third_party/WebKit/LayoutTests/fast/table/table-rowspan-height-distribution-in-rows-1.html
>
third_party/WebKit/LayoutTests/fast/table/table-rowspan-height-distribution-in-rows-2.html
> 
> These test files are set based on the linux platform and same works for the
> windows also.
> But some test in these files fails for mac platform.
> We should have separate test files for mac platform a well. and above should
not
> run for mac.

Or maybe fix the tests, and write expectations that pass on all platforms, e.g.
by setting an explicit line-height or something (I didn't actually check if
that's the problem)? But anyway, that's beyond the scope of this CL.

[a.suchit, 2017-01-05 10:43:34]

The CQ bit was checked by a.suchit@samsung.com

[commit-bot: I haz the power, 2017-01-05 10:43:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-05 10:47:54]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1483613014385110,
"parent_rev": "2b438241d2a70ff52623936b8e7f32dede4be477", "commit_rev":
"f54e6be8111c890f1db0f252b310eccd6e03ea76"}

[commit-bot: I haz the power, 2017-01-05 10:48:27]

Description was changed from

==========
Made varied number of cells in each row based on row's requirement.

Rowspan-only-row(s) was not getting height when there is at least one
empty cell present.

Number of the cells are created in the row based on the required
column in that row. So Empty cells would not be present at the end
of the row.

R=jchaffraix@chromium.org, mstensho@opera.com

BUG=396653
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Made varied number of cells in each row based on row's requirement.

Rowspan-only-row(s) was not getting height when there is at least one
empty cell present.

Number of the cells are created in the row based on the required
column in that row. So Empty cells would not be present at the end
of the row.

R=jchaffraix@chromium.org, mstensho@opera.com

BUG=396653
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2534413004
==========

[commit-bot: I haz the power, 2017-01-05 10:48:29]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2017-01-05 10:50:47]

Description was changed from

==========
Made varied number of cells in each row based on row's requirement.

Rowspan-only-row(s) was not getting height when there is at least one
empty cell present.

Number of the cells are created in the row based on the required
column in that row. So Empty cells would not be present at the end
of the row.

R=jchaffraix@chromium.org, mstensho@opera.com

BUG=396653
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2534413004
==========

to

==========
Made varied number of cells in each row based on row's requirement.

Rowspan-only-row(s) was not getting height when there is at least one
empty cell present.

Number of the cells are created in the row based on the required
column in that row. So Empty cells would not be present at the end
of the row.

R=jchaffraix@chromium.org, mstensho@opera.com

BUG=396653
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Committed: https://crrev.com/1060f916d9f7026fa22dd3bb6b563294ec7f8224
Cr-Commit-Position: refs/heads/master@{#441620}
==========

[commit-bot: I haz the power, 2017-01-05 10:50:49]

Patchset 14 (id:??) landed as
https://crrev.com/1060f916d9f7026fa22dd3bb6b563294ec7f8224
Cr-Commit-Position: refs/heads/master@{#441620}

[dgrogan, 2017-09-19 00:47:42]

dgrogan@chromium.org changed reviewers:
+ dgrogan@chromium.org

[dgrogan, 2017-09-19 00:47:43]

https://codereview.chromium.org/2534413004/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right):

https://codereview.chromium.org/2534413004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1742:
ensureCols(row, pos + 1 + 1);
On 2017/01/04 10:56:06, mstensho (USE GERRIT) wrote:
> pos + 2, perhaps? :)

Why is this extra +1 needed? Or, actually, why the ensureCols call at all? The
.insert call on the following line increases the vector length by 1, which
should be sufficient?

I ask because I'm looking at https://crbug.com/761192 , which deals with this
case:

<table>
  <tr>
    <td colspan=5></td>
  </tr>
  <tr>
    <td></td>
  </tr>
</table>

EnsureCols increases the width of the grid cells vector to 2 then the insert
increases it to 3, but there should only be 2 effective columns here.

Morten, how well do you remember this? Suchit is probably going to take a while
to see this email :)

[mstensho (USE GERRIT), 2017-09-19 19:10:27]

https://codereview.chromium.org/2534413004/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right):

https://codereview.chromium.org/2534413004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1742:
ensureCols(row, pos + 1 + 1);
On 2017/09/19 00:47:43, dgrogan wrote:
> On 2017/01/04 10:56:06, mstensho (USE GERRIT) wrote:
> > pos + 2, perhaps? :)
> 
> Why is this extra +1 needed? Or, actually, why the ensureCols call at all? The
> .insert call on the following line increases the vector length by 1, which
> should be sufficient?
> 
> I ask because I'm looking at https://crbug.com/761192 , which deals with this
> case:
> 
> <table>
>   <tr>
>     <td colspan=5></td>
>   </tr>
>   <tr>
>     <td></td>
>   </tr>
> </table>
> 
> EnsureCols increases the width of the grid cells vector to 2 then the insert
> increases it to 3, but there should only be 2 effective columns here.
> 
> Morten, how well do you remember this? Suchit is probably going to take a
while
> to see this email :)

Not much at all. :)

According to the WTF::Vector::Insert() documentation, |position| (the index)
must be within bounds, so it does indeed look like this could be necessary,
unless we can tell for sure that pos+1 is within bounds.

[a.suchit2, 2017-09-20 04:52:07]

a.suchit@chromium.org changed reviewers:
+ a.suchit@chromium.org

[a.suchit2, 2017-09-20 04:52:09]

https://codereview.chromium.org/2534413004/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutTableSection.cpp (right):

https://codereview.chromium.org/2534413004/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutTableSection.cpp:1742:
ensureCols(row, pos + 1 + 1);
On 2017/09/19 00:47:43, dgrogan wrote:
> On 2017/01/04 10:56:06, mstensho (USE GERRIT) wrote:
> > pos + 2, perhaps? :)
> 
> Why is this extra +1 needed? Or, actually, why the ensureCols call at all? The
> .insert call on the following line increases the vector length by 1, which
> should be sufficient?
> 
> I ask because I'm looking at https://crbug.com/761192 , which deals with this
> case:
> 
> <table>
>   <tr>
>     <td colspan=5></td>
>   </tr>
>   <tr>
>     <td></td>
>   </tr>
> </table>
> 
> EnsureCols increases the width of the grid cells vector to 2 then the insert
> increases it to 3, but there should only be 2 effective columns here.
> 
> Morten, how well do you remember this? Suchit is probably going to take a
while
> to see this email :)

EnsureCols is used to ensure that insert would be successful for position (pos +
1).

'pos' represents to position(index).
we are trying to insert new column at (current position + 1) so (pos + 1).
As EnsureCols accept the number of columns so number of columns at (pos + 1)
position would be (pos + 1 + 1).

Note : Before this patch EnsureCols was EnsureCol and EnsureCol was accepting
position but EnsureCols accepts number of Columns.