Closed Bug 351942 Opened 18 years ago Closed 18 years ago

change the cellmap zerospan repair mechanism.

Categories

(Core :: Layout: Tables, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: bernd_mozilla, Assigned: bernd_mozilla)

References

(Blocks 1 open bug, )

Details

Attachments

(2 files, 5 obsolete files)

Currently nsCellMap::GetDataAt has parameter aUpdateZeroSpan if it is true then the function will if it hits a cellmap hole look up the vicinity of the position whether it contains a originating cell with zero span. If it is so then the cells that shall be covered by the zerospan are marked as zerospans.

This means a innocent looking data lookup call migh severly change the cellmap underneath. This is a service nightmare. 

Second a vanilla table does not have a zerospan, but very frequently cellmap holes (incr. reflow). So all tables pay the performance price of the zerospan handling. 
This price tag is also known as hang. I fixed a couple of those before by disbling the repair mechanism.
What I propose is to change the mechnanism. 

There is only one way how a cell can enter the cellmap:
nsCellMap::AppendCell. At this place if a cell has specified the zerospan set a flag that the cellmap/table contains zero spans.

Put inside nsTableFrame::MatchCellMapToColCache a funtion that expands the zerospans if possible.
http://lxr.mozilla.org/seamonkey/source/layout/tables/nsCellMap.cpp#47 change definition of #define MIN_NUM_COLS_FOR_ZERO_COLSPAN to 1.

Make sure that we rebuild the cellmap if we append after a zero spanning cell.
OS: Windows XP → All
Hardware: PC → All
Blocks: 352461
Blocks: 348805
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → bernd_mozilla
Status: NEW → ASSIGNED
Attached patch rev2 (obsolete) — Splinter Review
Attached patch patch rev2a (obsolete) — Splinter Review
Attachment #239793 - Flags: superreview?(bzbarsky)
Attachment #239793 - Flags: review?(bzbarsky)
More than comment 1 the following happend:
 GetNumCellsOriginatingInRow rewrite of the function it was dangerous function performance wise, its new structure is now like the other functions.

removal of redundant aMap args

constify the lookup methods

replace NS_ASSERTION(PR_FALSE with NS_ERROR(

Special care for review: 

void nsCellMap::ExpandZeroColSpans(nsTableCellMap& aMap) this function is somehow and the core routine of the whole patch.

Comment on attachment 239793 [details] [diff] [review]
patch rev2a

So for future reference it took a _long_ time to review this because I had to sort out the cleanup from the substantive changes.  Splitting those up into separate patches would be great.

>Index: nsCellMap.cpp

> {
>   PRInt32 rowIndex = aRowIndex;
>   nsCellMap* map = mFirstMap;
>   while (map) {
>     if (map->GetRowCount() > rowIndex) {
...
>     }
>     rowIndex -= map->GetRowCount();
>     map = map->GetNextSibling();
>   }

This code pattern is repeated a _lot_.  It's worth filing a followup bug to
make it into a macro.  Perhaps something like FIND_MAP_AND_CALL_METHOD_ON_IT or something....

>+void nsTableCellMap::ExpandZeroColSpans()
>+{
>+  mTableFrame.SetHasZeroColSpans(PR_FALSE); // reset the bit, if there is
>+                                         // zerospan it will be set again.

Line up the comments better?

>@@ -1347,32 +1350,38 @@ nsCellMap::AppendCell(nsTableCellMap&   
>+    if (data->IsZeroColSpan() ) {
>+      origData = data; //appending a cell collapses zerospans.

Space after the "//"?  And should this say something more like: "If the previous cell in this row has a colspan of zero, we now don't want it spanning past the new cell we're adding" or something?  Does the zero colspan collapse down into a single column in this case, by the way?  Or what?

Basically, I'd like to understand this part of the code, and I'm not sure I do.  Could we add more comments here?

Also following this code we have:

1389   // Setup CellData for this cell
1390   if (origData) {
1391     NS_ASSERTION(origData->IsDead(), "replacing a non dead cell is a memory leak");

Won't this assert (and leak?) if |origData->IsZeroColSpan()|?

>@@ -1415,25 +1424,26 @@ nsCellMap::AppendCell(nsTableCellMap&   
>+              // do nothing, this can be caused by rowspan which is overlapped
>+              // by a cell with row and colspan

s/row and colspan/rowspan and colspan/ to make it clearer please?

> PRInt32 nsCellMap::GetColSpanForNewCell(nsTableCellFrame& aCellFrameToAdd, 

This no longer needs the aColIndex or aNumColsInTable args, right?  Followup bug or roll in, either way.

>+nsCellMap::GetNumCellsOriginatingInRow(PRInt32 aRowIndex) const
>+{
>+  nsVoidArray* row = (nsVoidArray *)(mRows.SafeElementAt(aRowIndex));
>+  PRInt32 count = 0;
>+  if (row) {

So we know aRowIndex is in the right bounds.

>+    PRInt32 maxColIndex = row->Count();
>+    PRInt32 colIndex;
>+    for (colIndex = 0; colIndex < maxColIndex; colIndex++) {
>+      CellData* cellData = GetDataAt(aRowIndex, colIndex);

But this will bounds-check it again.  As a followup, might be worth considering making |*row| an nsTArray<CellData*> instead of nsVoidArray.  Then you could just get the cell from the array directly...  In fact, a lot of this code could be simplified with some nsTArray love.  I filed bug 355754 on having a way to do SafeElementAt on nsTArray.

>+void nsCellMap::ExpandZeroColSpans(nsTableCellMap& aMap)

>+      if (colZeroSpan) {
>+         aMap.mTableFrame.SetHasZeroColSpans(PR_TRUE);
>+         // do the expansion

Weird indent.  Fix that?

So this code will make us redo the expansion every time we resync the cellmap while there's at least one cell in the table with a zero colspan, right?  So adding cells to such a table will be at least O(N) in the number of cells already in the table?  And if I want, from JS, to build such a table, it'll be O(N^2) in table size?

Is there any way at all that we can avoid that?  Or am I missing something?  Could we possibly only perform zero-span expansion before actually reflowing or something like that?

>+          // look at the next colindex down if there is no obstacle mark the
>+          // column as zerospan

Maybe more like "look at columns from here to our colspan.  For each one, check the rows from here to our rowspan to make sure there is no obstacle to marking that column as a zerospanned column; if there isn't, mark it so".

>+            CellData* newData = (aMap.mBCInfo) ? new BCCellData(nsnull) :
>+                                                 new CellData(nsnull);

  AllocCellData(nsnull);

>+            if (colX > colIndex) {

Won't this always be true at this point?

>+            // colspan=0 is only counted as spanning the 1st col to the right
>+            // of its origin

Why?  This could use some more documentation.

>-nsCellMap::AdjustForZeroSpan(nsTableCellMap& aMap,

>-  // if there is both a rowspan=0 and colspan=0 then only expand the cols to a minimum

Why do we no longer need to do this?

In fact, where do we handle zero rowspans with the new code?  Or is that already handled by nsCellMap::AppendCell?  But what if after that the number of rows in the rowgroup increases?

>Index: nsTableFrame.cpp
>@@ -1012,16 +1012,18 @@ nsTableFrame::MatchCellMapToColCache(nsT

>+  if (HasZeroColSpans())
>+    aCellMap->ExpandZeroColSpans();

Curly braces, please.

I'd like to see answers to the questions above before marking reviews.  Esp. the performance issue and the substantive changes from the old code...
IRC Log:
        Bernd>	bz: do you have some time for my pet peeve
	<bz>	Bernd: which one of them? ;)
	<bz>	bernd: GetDataAt?
	<Bernd>	bingo
	<bz>	yes, I do
	<bz>	Let's get this baby dead
	<Bernd>	would it be good enough if we repair only if the number of columns changes
	<Bernd>	( I see the performance comment as the most serious issue)
	<bz>	Bernd: yeah, me too. So I did understand the situation correctly, right?
	<bz>	Bernd: So just to make sure we're on the same page... how _do_ we handle zero rowspans?
	<Bernd>	yes you did, my proposal is to add 2 bit needs expansion that is reset once we corrected it
	<bz>	Bernd: with the patch?
	<Bernd>	bz: no we do it differently
	<Bernd>	or to be more precise we don't do it correctly
	<bz>	ok
	<bz>	do we do it correctly before the patch? ;)
	<Bernd>	currently which causes bug 337124
	<bz>	oh
	<Bernd>	thats the wrong bug
	<Bernd>	bug 339129
	<Bernd>	bz: I will ask for you review for it soon, but the idea is we know many rows a table has when we build the cellmap, but we dont know before how many cols we will have at the end
	<Bernd>	so the colspan expansion is different
	<bz>	ok
	<bz>	So this patch is really just changing the colspan expansion?
	<Bernd>	yes
	<bz>	ok
	<Bernd>	if we add a row to a rowspan zero we will rebuild the cellmap
	<bz>	good
	<bz>	In that case I agree -- just rebuilding if we have zero colspans _and_ the number of columns has changed is the way to go
	<bz>	Another question
	<Bernd>	I tried to squeeze out every performance bit that I could get
	<bz>	Say I have:
	<Bernd>	thats why the cleanup/constify
	<bz>	A table with 3 columns
	<bz>	<td colspan="0"> in column 1
	<bz>	nothing else in that row
	<bz>	We would expand that to have entries in column 2 and column 3 in the cellmap, right?
	<Bernd>	yes
	<Bernd>	wait
	<bz>	OK.
	* bz	wats
	<bz>	er, waits
	<Bernd>	how do you get the tree columns?
	<bz>	there's a previous row
	<bz>	with three cells in it
	<bz>	or whatever.
	<Bernd>	then we will certainly expand
	<bz>	ok
	<bz>	Now I insert a cell in the same row right after the colspanning cell
	<Bernd>	you violate the html 4 spec
	<bz>	sure
	|<--	nosebleed has left moznet (Client exited)
	<Bernd>	so we will ignore the colspan 0
	<bz>	ok
	<Bernd>	and put the cell just behind
	<Bernd>	thats new
	<Bernd>	and I am proud of it
	<bz>	Right
	<bz>	so now in column 1 we have the same entry we used to have
	<bz>	in column 2 we have the entry for the new cell
	<bz>	and what happens to column 3?
	<Bernd>	dead cell
	<bz>	Do we mark it so?
	<bz>	With this patch?
	<Bernd>	I think so, at least we should
	<bz>	can you point me to where?
	<Bernd>	...
	|<--	gaoming has left moznet (Ping timeout)
	<Bernd>	+ if (data->IsZeroColSpan() ) {
+ origData = data; //appending a cell collapses zerospans.
+ break;
+ }
	<Bernd>	after the second plussed line it has to happen
	<bz>	ok
	<bz>	That's what I was thinking....
	<bz>	one last question
	<bz>	can it happen that we both add and remove a column in between potential cellmap rebuilds?
	<bz>	In particular, remove a column from one colgroup but add it to another one?
	<bz>	That requires zero colspan reexpansion, right?
	<Bernd>	no
	<Bernd>	our cellmap is colgroup agnostic
	<bz>	huh
	<Bernd>	the html spec requires something different but who cares about it.
	<bz>	See, HTML4 claims that colspan=0 means out to the end of the colgroup
	<bz>	ah
	<bz>	ok, then. ;)
	<Bernd>	I filed a bug about it
	<bz>	So wait
	<Bernd>	when I was young
	<bz>	Can it happen that we remove the last column and then append a column
	<bz>	without the rebuild between the two operations?
	<bz>	I'm guessing "no"....
	<Bernd>	https://bugzilla.mozilla.org/show_bug.cgi?id=185672
	<Bernd>	what happens all the time is that we move one column from one colgroup to another
	<bz>	ok
	<bz>	but since that doesn't affect zerospans we don't care
	<Bernd>	for instance if we add one content based col we remove it from the anonymous ones
	<Bernd>	thats as you say
	<bz>	So "Can it happen that we remove the last column and then append a column without the rebuild between the two operations?"
	<bz>	That's the only remaining case that worries me
	<bz>	Since in that case the column count doesn't change but zerospans need reexpanding
	<Bernd>	bz: if that could happen then it would mean that the first step could happen withotu the second one
	<Bernd>	and then we will crash (sooner or later)
	<Bernd>	the assumption is that I marked all possible paths already with the sync call
	<bz>	Bernd: ok. In that case, checking whether the col count changed sounds perfect.
	<Bernd>	what I think is a two bit approach: 1 bit has zerospan and another needsexpansion
	<Bernd>	if we add a cell with zerospan we set both flags
	<bz>	yep
	<Bernd>	once we expand we clear the later
	<bz>	yep
	<bz>	Sounds good!
	<Bernd>	if the col numbe changes we expand
	<Bernd>	or to be more precise we set the need bit and expand
	<bz>	bernd: right
	<bz>	bernd: sounds good
	
Blocks: 356335
Attachment #239793 - Flags: superreview?(bzbarsky)
Attachment #239793 - Flags: superreview-
Attachment #239793 - Flags: review?(bzbarsky)
Attachment #239793 - Flags: review-
see comming attachment:

> So for future reference it took a _long_ time to review this because I had to
> sort out the cleanup from the substantive changes.  Splitting those up into
> separate patches would be great.
> 
>>Index: nsCellMap.cpp
> 
>> {
>>   PRInt32 rowIndex = aRowIndex;
>>   nsCellMap* map = mFirstMap;
>>   while (map) {
>>     if (map->GetRowCount() > rowIndex) {
> ...
>>     }
>>     rowIndex -= map->GetRowCount();
>>     map = map->GetNextSibling();
>>   }
> 
> This code pattern is repeated a _lot_.  It's worth filing a followup bug to
> make it into a macro.  Perhaps something like FIND_MAP_AND_CALL_METHOD_ON_IT or
> something....

I filed bug  357110.
 
>>+void nsTableCellMap::ExpandZeroColSpans()
>>+{
>>+  mTableFrame.SetHasZeroColSpans(PR_FALSE); // reset the bit, if there is
>>+                                         // zerospan it will be set again.
> 
> Line up the comments better?
 
fixed

>>@@ -1347,32 +1350,38 @@ nsCellMap::AppendCell(nsTableCellMap&   
>>+    if (data->IsZeroColSpan() ) {
>>+      origData = data; //appending a cell collapses zerospans.
> 
> Space after the "//"?  And should this say something more like: "If the
> previous cell in this row has a colspan of zero, we now don't want it spanning
> past the new cell we're adding" or something?  Does the zero colspan collapse
> down into a single column in this case, by the way?  Or what?
> 
> Basically, I'd like to understand this part of the code, and I'm not sure I do.
>  Could we add more comments here?

see void nsCellMap::CollapseZeroColSpan(

> Also following this code we have:
> 
> 1389   // Setup CellData for this cell
> 1390   if (origData) {
> 1391     NS_ASSERTION(origData->IsDead(), "replacing a non dead cell is a
> memory leak");
> 
> Won't this assert (and leak?) if |origData->IsZeroColSpan()|?

This will/should not happen due to nsCellMap::CollapseZeroColSpan

>>@@ -1415,25 +1424,26 @@ nsCellMap::AppendCell(nsTableCellMap&   
>>+              // do nothing, this can be caused by rowspan which is overlapped
>>+              // by a cell with row and colspan
> 
> s/row and colspan/rowspan and colspan/ to make it clearer please?
> 
fixed

>> PRInt32 nsCellMap::GetColSpanForNewCell(nsTableCellFrame& aCellFrameToAdd, 
> 
> This no longer needs the aColIndex or aNumColsInTable args, right?  Followup
> bug or roll in, either way.
>
Code delete, no chance to defer this :-), fixed
 
>>+nsCellMap::GetNumCellsOriginatingInRow(PRInt32 aRowIndex) const
>>+{
>>+  nsVoidArray* row = (nsVoidArray *)(mRows.SafeElementAt(aRowIndex));
>>+  PRInt32 count = 0;
>>+  if (row) {
> 
> So we know aRowIndex is in the right bounds.
> 
>>+    PRInt32 maxColIndex = row->Count();
>>+    PRInt32 colIndex;
>>+    for (colIndex = 0; colIndex < maxColIndex; colIndex++) {
>>+      CellData* cellData = GetDataAt(aRowIndex, colIndex);
> 
> But this will bounds-check it again.  As a followup, might be worth considering
> making |*row| an nsTArray<CellData*> instead of nsVoidArray.  Then you could
> just get the cell from the array directly...  In fact, a lot of this code could
> be simplified with some nsTArray love.  I filed bug 355754 on having a way to
> do SafeElementAt on nsTArray.

-      CellData* cellData = GetDataAt(aRowIndex, colIndex);
+      CellData* cellData = (CellData *)(row->ElementAt(aColIndex));

fixed
 
>>+void nsCellMap::ExpandZeroColSpans(nsTableCellMap& aMap)
> 
>>+      if (colZeroSpan) {
>>+         aMap.mTableFrame.SetHasZeroColSpans(PR_TRUE);
>>+         // do the expansion
> 
> Weird indent.  Fix that?

fixed

> 
> So this code will make us redo the expansion every time we resync the cellmap
> while there's at least one cell in the table with a zero colspan, right?  So
> adding cells to such a table will be at least O(N) in the number of cells
> already in the table?  And if I want, from JS, to build such a table, it'll be
> O(N^2) in table size?
> 
> Is there any way at all that we can avoid that?  Or am I missing something? 
> Could we possibly only perform zero-span expansion before actually reflowing or
> something like that?
> 
I used another status bit, if the column number changes or a cell with a colspan 0 is added we will expand the cellmap later.
 
>>+          // look at the next colindex down if there is no obstacle mark the
>>+          // column as zerospan
> 
> Maybe more like "look at columns from here to our colspan.  For each one, check
> the rows from here to our rowspan to make sure there is no obstacle to marking
> that column as a zerospanned column; if there isn't, mark it so".
> 

copied the comment

>>+            CellData* newData = (aMap.mBCInfo) ? new BCCellData(nsnull) :
>>+                                                 new CellData(nsnull);
> 
>   AllocCellData(nsnull);
> 
>>+            if (colX > colIndex) {
> 
> Won't this always be true at this point?
> 
>>+            // colspan=0 is only counted as spanning the 1st col to the right
>>+            // of its origin
> 
> Why?  This could use some more documentation.
>
why document if one can delete the code ;-), 
 
>>-nsCellMap::AdjustForZeroSpan(nsTableCellMap& aMap,
> 
>>-  // if there is both a rowspan=0 and colspan=0 then only expand the cols to a minimum
> 
> Why do we no longer need to do this?

nsCellMap::ExpandZeroColSpans does exactly do this, infact the code from this function served as a starting point.

> In fact, where do we handle zero rowspans with the new code?  Or is that
> already handled by nsCellMap::AppendCell?  But what if after that the number of
> rows in the rowgroup increases?

Yes it is already handled by nsCellMap::AppendCell. See the call to GetRowSpanForNewCell. This requires a review for bug 339129.

>>Index: nsTableFrame.cpp
>>@@ -1012,16 +1012,18 @@ nsTableFrame::MatchCellMapToColCache(nsT
> 
>>+  if (HasZeroColSpans())
>>+    aCellMap->ExpandZeroColSpans();

fixed
> Curly braces, please.
> 
> I'd like to see answers to the questions above before marking reviews.  Esp.
> the performance issue and the substantive changes from the old code...

I hope I did answer the questions.
Attached patch rev3 (obsolete) — Splinter Review
Attachment #238920 - Attachment is obsolete: true
Attachment #239791 - Attachment is obsolete: true
Attachment #239793 - Attachment is obsolete: true
> Code delete, no chance to defer this :-), fixed

Well.  The main reason for deferring is to make the diff between patches smaller so I can review faster.  :(

Comment on attachment 242683 [details] [diff] [review]
rev3

>Index: nsCellMap.cpp
>@@ -1347,32 +1351,46 @@ nsCellMap::AppendCell(nsTableCellMap&   
>+    if (data->IsZeroColSpan() ) {
>+      // appending a cell collapses zerospans.
>+      CollapseZeroColSpan(aMap, aRowIndex,startColIndex);

Space after comma, please.

>@@ -1441,21 +1460,19 @@ nsCellMap::AppendCell(nsTableCellMap&   
>-              // only count the 1st spanned col of a zero col span
>-              if (!zeroColSpan || (colX == startColIndex + 1)) {
>-                nsColInfo* colInfo = aMap.GetColInfoAt(colX);
>-                colInfo->mNumCellsSpan++;
>-              }
>+              
>+              nsColInfo* colInfo = aMap.GetColInfoAt(colX);
>+              colInfo->mNumCellsSpan++;

So what's with this change?  Are we guaranteed now that

  !zeroColSpan || colX == startColIndex + 1

holds (because we called CollapseZeroColSpan())?  If so, can we assert it?  If not, why is this change correct?  Similar questions in other places where you made this sort of change.

>+void nsCellMap::CollapseZeroColSpan(nsTableCellMap&   aMap,
>+                                      PRInt32         aRowIndex,
>+                                      PRInt32         aColIndex)

Fix the indent.

>+  CellData* origData = GetDataAt(aRowIndex, aColIndex);
>+  if (origData && origData->IsZeroColSpan())

How about just asserting that?  It's sure true at the one and only caller of this method...  If we documented in the header that the caller should ensure this, we'd be good.

For that matter, why not just have the caller pass in the CellData* and in this function just assert that this is in fact the what GetDataAt(aRowIndex, aColIndex) returns and that it's a zero-colspan?

>+    nsTableCellFrame* cell = origData->GetCellFrame();
>+    if (cell) {

When would this be null?  Can we just assert that it's not null?

>+      PRInt32 startRowIndex = aRowIndex - origData->GetRowSpanOffset();

So that's the row index of the original colspanning/rowspanning cell, right?

>+      for (PRInt32 colX = aColIndex; colX < endColIndex; colX++) {
>+        nsColInfo* colInfo = aMap.GetColInfoAt(colX);

When would there be no colInfo?  Document, please.

>+        if (colInfo) {
>+          colInfo->mNumCellsSpan -= rowSpan;

So I'm trying to reconcile this with the comments where the nsColInfo struct is declared.  Those say that a zero-colspan only counts as spanning into a single column to its right.  Are they wrong?  If so, could we update them?  I think documenting what that member means now would help me understand all the changes to code that munges it in this patch.

>+        for (PRInt32 rowX = startRowIndex; rowX < endRowIndex; rowX++)
...
>+          NS_ASSERTION(data->IsZeroColSpan(),
>+                       "Overwriting previous data - memory leak");
>+          data->Init(nsnull); // mark the cell as a dead cell.

So this is OK because we started the column loop at aColIndex, not at startColIndex?  That was highly non-obvious on first read through; please document?

>@@ -1737,36 +1794,29 @@ void nsCellMap::ExpandWithCells(nsTableC

>-        // if the colspan is 0 only count the 1st spanned col
>-        PRBool countAsSpan = PR_FALSE;

So changes like this are safe because CollapseZeroColSpan takes care of it all now?  Or because we changed the meaning of mNumCellsSpan?

>@@ -1786,21 +1836,18 @@ void nsCellMap::ShrinkWithoutRows(nsTabl

>         else if (data->IsColSpan()) {
>-          if ( (!data->IsZeroColSpan()) ||
>-               ((data->IsZeroColSpan()) && (rowX == aStartRowIndex) && (!IsZeroColSpan(rowX, colX - 1)))) {
>             nsColInfo* colInfo = aMap.GetColInfoAt(colX);
>             colInfo->mNumCellsSpan--;
>-          }

Fix the indent of those two remaining lines.

>+void nsCellMap::ExpandZeroColSpans(nsTableCellMap& aMap)
>+            CellData* newData = (aMap.mBCInfo) ? new BCCellData(nsnull) :
>+                                                 new CellData(nsnull);

Again, AllocCellData(nsnull).  Just like the first time I saw it.  ;)

>Index: nsCellMap.h

>+  /** collapse the 0 colspan as it can not be properly handled if another cells
>+    * goes beyond a zero colspan. This would increase the number of columns so
>+    * the zero colspan would have to expand too. In order to break this cycle we
>+    * reset the colspan

How about:

  Function to be called when a cell is added at a location which is spanned to by a zero colspan.  We handle this situation by collapsing the zero colspan, since there is really no good way to deal with it (trying to increase the number of columns to hold the new cell would just mean the zero colspan needs to expand).

If that's not what the comment is trying to say, what _is_ it trying to say?

>+    * @param aMap      - reference to the table cell map
>+    * @param aRowIndex - row where the first collision appears
>+    * @param aColIndex - column where the first collision appears

Document that GetDataAt(aRowIndex, aColIndex) returns a non-null CellData* which has IsZeroColSpan() true?  And again, it might be worth just passing in the CellData*.

>Index: nsTableFrame.cpp
>+  if (numColsToAdd && HasZeroColSpans()) {
>+    SetNeedColSpanExpansion(PR_TRUE);
>+  }
>+  if (NeedColSpanExpansion()) {

I think this could use a comment saying that other places may have called SetNeedColSpanExpansion() already, so even if !numColsToAdd we could need colspan expansion.  Otherwise I see people being tempted to "simplify" this code.

r- pending an explanation for the mNumCellsSpan deal. And AllocCellData, please!
Attachment #242683 - Flags: superreview-
Attachment #242683 - Flags: review-
> (From update of attachment 242683 [details] [diff] [review])
>> >Index: nsCellMap.cpp
>> >@@ -1347,32 +1351,46 @@ nsCellMap::AppendCell(nsTableCellMap&   
>> >+    if (data->IsZeroColSpan() ) {
>> >+      // appending a cell collapses zerospans.
>> >+      CollapseZeroColSpan(aMap, aRowIndex,startColIndex);
> 
> Space after comma, please.
> 
I am not sure, how this did not trigger a whitespace error at
http://www.johnkeiser.com/cgi-bin/jst-review-cgi.pl?patch_file=&patch_url=https%3A%2F%2Fbugzilla.mozilla.org%2Fattachment.cgi%3Fid%3D242683&patch_text=&reason_type=%21&reason_type=A&reason_type=B&reason_type=E&reason_type=F&reason_type=L&reason_type=N&reason_type=O&reason_type=P&reason_type=Q&reason_type=R&reason_type=S&reason_type=W&reason_type=X&reason_type=%7B

fixed

>> >@@ -1441,21 +1460,19 @@ nsCellMap::AppendCell(nsTableCellMap&   
>> >-              // only count the 1st spanned col of a zero col span
>> >-              if (!zeroColSpan || (colX == startColIndex + 1)) {
>> >-                nsColInfo* colInfo = aMap.GetColInfoAt(colX);
>> >-                colInfo->mNumCellsSpan++;
>> >-              }
>> >+              
>> >+              nsColInfo* colInfo = aMap.GetColInfoAt(colX);
>> >+              colInfo->mNumCellsSpan++;
> 
> So what's with this change?  Are we guaranteed now that
> 
>   !zeroColSpan || colX == startColIndex + 1
> 
> holds (because we called CollapseZeroColSpan())?  If so, can we assert it?  If
> not, why is this change correct?  Similar questions in other places where you
> made this sort of change.
> 

*The mNumCellsSpan deal*

The old code made a distinction between the first colspanned cell which was created unconditional and all the others that appeared by repair.
The first was handled like a real colspan that could span into a column the later one did not contribute. The distinction is removed from the code so all the references to this have to go too.


>> >+void nsCellMap::CollapseZeroColSpan(nsTableCellMap&   aMap,
>> >+                                      PRInt32         aRowIndex,
>> >+                                      PRInt32         aColIndex)
> 
> Fix the indent.
> 
fixed

>> >+  CellData* origData = GetDataAt(aRowIndex, aColIndex);
>> >+  if (origData && origData->IsZeroColSpan())
> 
> How about just asserting that?  It's sure true at the one and only caller of
> this method...  If we documented in the header that the caller should ensure
> this, we'd be good.


> For that matter, why not just have the caller pass in the CellData* and in this
> function just assert that this is in fact the what GetDataAt(aRowIndex,
> aColIndex) returns and that it's a zero-colspan?

fixed
 
>> >+    nsTableCellFrame* cell = origData->GetCellFrame();
>> >+    if (cell) {
> 
> When would this be null?  Can we just assert that it's not null?

done, I was very concerned to pass the null cell as reference into GetRowSpanForNewCell if the assertion failes. So I changed the signature to nsTableCellFrame* if this fails we will make a 0 deref which seems safe to me.

>> >+      PRInt32 startRowIndex = aRowIndex - origData->GetRowSpanOffset();
> 
> So that's the row index of the original colspanning/rowspanning cell, right?

yes

 
>> >+      for (PRInt32 colX = aColIndex; colX < endColIndex; colX++) {
>> >+        nsColInfo* colInfo = aMap.GetColInfoAt(colX);
> 
> When would there be no colInfo?  Document, please.
I am paranoid,  its a Pawlow reflex  col --> if(col)

>> >+        if (colInfo) {
>> >+          colInfo->mNumCellsSpan -= rowSpan;
> 
> So I'm trying to reconcile this with the comments where the nsColInfo struct is
> declared.  Those say that a zero-colspan only counts as spanning into a single
> column to its right.  Are they wrong?  If so, could we update them?  I think
> documenting what that member means now would help me understand all the changes
> to code that munges it in this patch.
> 
I removed the offending comment, but the core explanation is above.

>> >+        for (PRInt32 rowX = startRowIndex; rowX < endRowIndex; rowX++)
> ..
>> >+          NS_ASSERTION(data->IsZeroColSpan(),
>> >+                       "Overwriting previous data - memory leak");
>> >+          data->Init(nsnull); // mark the cell as a dead cell.
> 
> So this is OK because we started the column loop at aColIndex, not at
> startColIndex? 

exactly

> That was highly non-obvious on first read through; please
> document?

I changed it so that it really collapses to colspan = 1
> 
>> >@@ -1737,36 +1794,29 @@ void nsCellMap::ExpandWithCells(nsTableC
> 
>> >-        // if the colspan is 0 only count the 1st spanned col
>> >-        PRBool countAsSpan = PR_FALSE;
> 
> So changes like this are safe because CollapseZeroColSpan takes care of it all
> now?  Or because we changed the meaning of mNumCellsSpan?

See second comment
>> >@@ -1786,21 +1836,18 @@ void nsCellMap::ShrinkWithoutRows(nsTabl
> 
>> >         else if (data->IsColSpan()) {
>> >-          if ( (!data->IsZeroColSpan()) ||
>> >-               ((data->IsZeroColSpan()) && (rowX == aStartRowIndex) && (!IsZeroColSpan(rowX, colX - 1)))) {
>> >             nsColInfo* colInfo = aMap.GetColInfoAt(colX);
>> >             colInfo->mNumCellsSpan--;
>> >-          }
> 
> Fix the indent of those two remaining lines.

done

>> >+void nsCellMap::ExpandZeroColSpans(nsTableCellMap& aMap)
>> >+            CellData* newData = (aMap.mBCInfo) ? new BCCellData(nsnull) :
>> >+                                                 new CellData(nsnull);
> 
> Again, AllocCellData(nsnull).  Just like the first time I saw it.   ;) 

I just did not understand what you meant the first time, but now that you insist I somehow see the light (after the recovery from the hit with the clue stick)

fixed

>> >Index: nsCellMap.h
> 
>> >+  /** collapse the 0 colspan as it can not be properly handled if another cells
>> >+    * goes beyond a zero colspan. This would increase the number of columns so
>> >+    * the zero colspan would have to expand too. In order to break this cycle we
>> >+    * reset the colspan
> 
> How about:
> 
>   Function to be called when a cell is added at a location which is spanned to
> by a zero colspan.  We handle this situation by collapsing the zero colspan,
> since there is really no good way to deal with it (trying to increase the
> number of columns to hold the new cell would just mean the zero colspan needs
> to expand).
> 
> If that's not what the comment is trying to say, what _is_ it trying to say?
> 
>> >+    * @param aMap      - reference to the table cell map
>> >+    * @param aRowIndex - row where the first collision appears
>> >+    * @param aColIndex - column where the first collision appears
> 
> Document that GetDataAt(aRowIndex, aColIndex) returns a non-null CellData*
> which has IsZeroColSpan() true?  And again, it might be worth just passing in
> the CellData*.
> 
I went this way.

>> >Index: nsTableFrame.cpp
>> >+  if (numColsToAdd && HasZeroColSpans()) {
>> >+    SetNeedColSpanExpansion(PR_TRUE);
>> >+  }
>> >+  if (NeedColSpanExpansion()) {
> 
> I think this could use a comment saying that other places may have called
> SetNeedColSpanExpansion() already, so even if !numColsToAdd we could need
> colspan expansion.  Otherwise I see people being tempted to "simplify" this
> code.
> 
Hmm, is that a promise if I leave it uncommented I will finally get a sidekick (other than you) who will love to hack tables? I guess NO, comment added.


Attached patch rev4 (obsolete) — Splinter Review
Attachment #242683 - Attachment is obsolete: true
Attachment #242746 - Flags: superreview?(bzbarsky)
Attachment #242746 - Flags: review?(bzbarsky)
Attached patch rev4Splinter Review
Attachment #242746 - Attachment is obsolete: true
Attachment #242747 - Flags: superreview?(bzbarsky)
Attachment #242747 - Flags: review?(bzbarsky)
Attachment #242746 - Flags: superreview?(bzbarsky)
Attachment #242746 - Flags: review?(bzbarsky)
Comment on attachment 242747 [details] [diff] [review]
rev4

>Index: nsCellMap.cpp

>+    // start the  collapse just after the orinating cell as we would
>+    // have colspan = 1

Maybe:

  Start the collapse just after the originating cell, since
  we're basically making the originating cell act as if it
  has colspan="1".



>Index: nsCellMap.h
>+    * increase the number of columns to hold the new cell would just mean the
>+    * zero colspan needs to expand

You lost the ")." part at the end of that comment, I think.

>Index: nsTableFrame.cpp

>+    // this flag can be triggered two fold just by changing the number of
>+    // columns or when a cell with a colspan = 0 gets added to the cellmap
>+    // @see nsCellMap::AppendCell

  This flag can be set in two ways -- either by changing
  the number of columns (that happens in the block above),
  or by adding a cell with colspan="0" to the cellmap.  To
  handle the latter case we need to explicitly check the
  flag here -- it may be set even if the number of columns
  did not change.
  
  @see nsCellMap::AppendCell

With those comment nits, r+sr=bzbarsky.  This looks great!
Attachment #242747 - Flags: superreview?(bzbarsky)
Attachment #242747 - Flags: superreview+
Attachment #242747 - Flags: review?(bzbarsky)
Attachment #242747 - Flags: review+
Attached patch rev5Splinter Review
patch incorporating Boris' comments
I just checked in rev5. It did not change any tp.  But this change makes all jprofs that that have GetDataAt in a prominent position obsolete. (Isn't that broad hint ;-) ?)
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Blocks: 226243
Blocks: 234240
I can't actually find any profiles with a prominent GetDataAt except for bug 352367.
Depends on: 371290
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: