Boris Zbarsky [:bzbarsky] Reporter
	Description • 14 years ago

It might be better to just set a flag that says we need to renumber rows and renumber them if/when we really need to, at least if nothing in table frame manipulation depends on these numbers.  See bug 559396 for the issues the current behavior causes: every row frame removal calls nsTableRowGroupFrame::AdjustRowIndices which walks all row frames.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 1 • 14 years ago

roc, do we have anyone who might be able to work on this?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 2 • 14 years ago

Lots of people *could* work on this, it's a question of priorities...

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 3 • 14 years ago

OK.  Do we have a general layout perf tracker?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 4 • 14 years ago

A tracking bug? Not that I know of.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 5 • 14 years ago

OK, filed bug 561937.

	Bernd
	Comment 6 • 14 years ago

>every row frame removal calls
>nsTableRowGroupFrame::AdjustRowIndices which walks all row frames.
Boris are you saying that nsTableFrame::RemoveRows is called with single rows for every row that gets removed?

	Bernd
	Comment 7 • 14 years ago

we do as  nsTableRowGroupFrame::RemoveFrame shows

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 8 • 14 years ago

> Boris are you saying that nsTableFrame::RemoveRows is called with single rows
> for every row that gets removed?

In the testcase in bug 559396, yes.  That testcase sets some (non-contiguous) subset of the rows to display:none, and therefore we remove the rowframes corresponding to those rows one at a time.

	Bernd
	Comment 9 • 14 years ago

I think regardless of the ordering we are only removing one frame at a time.


Further the cellmap manipulation heavily relies on the correct rowindex, so before we call into cellmap manipulation we need to be sure that the rowindex is correct.

1. step we call remove rows the rowIndex of the row to be removed is correct we defer adjustRowIndices.
2. we call a second time remove rows, the row index is unknown to be correct we call the deferred AdjustRowIndex before we can remove as we will otherwise do bad things in the cellmap.

==> performance gain = 0

What would really help is to remove more than one frame at a time.

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 10 • 7 years ago

Did this get fixed in bug 1285874?

	Neerja Pancholi[:neerja]
	Comment 11 • 7 years ago

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #10)
> Did this get fixed in bug 1285874?

Yes, I believe so.
With the fix for Bug 1285874, instead of calculating all the new indices at the time of row deletion (as AdjustRowIndices was doing) we just add the index of the newly deleted row to a map. The final index is then recalculated on the basis of the indices in the map when GetRowIndex is called. 
The only place where AdjustRowIndices is now called is in InsertRows -> https://dxr.mozilla.org/mozilla-central/source/layout/tables/nsTableFrame.cpp#955.
Thanks!