238999 - document colframe creation

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Bernd]

this bug is for boris.

Comment 1

[Bernd]

Attachment: patch for nsTableColgroup.h

Comment 2

[Bernd]

Attachment: patch

Comment 3

[Boris Zbarsky [:bzbarsky]]

Bernd, I'll try to get to this in the next few days....  Thank you for doing it!

Comment 4

[Bernd]

just a few review hints:
http://bugzilla.mozilla.org/attachment.cgi?id=146869&action=diff#mozilla/layout/html/table/src/nsTableColGroupFrame.cpp_sec3
and 
http://bugzilla.mozilla.org/attachment.cgi?id=146869&action=diff#mozilla/layout/html/table/src/nsTableFrame.cpp_sec3

are fixes for the crashes that I wallpapered over in bug 240854 I believe this
code is wrong since its checkin in 1999. So it deserves special attention.

I believe that in 
http://bugzilla.mozilla.org/attachment.cgi?id=146869&action=diff#mozilla/layout/html/table/src/nsTableFrame.cpp_sec5

    // remove the col frames, the colGroup frame and reset col indices
    colGroup->RemoveChildrenAtEnd(*aPresContext, colGroup->GetColCount());
    mColGroups.DestroyFrame(aPresContext, aOldFrame);
these calls are redundant as we first remove all children of colgroup and then
destroy the frame, I hope I did not overlook here something.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 146869 [details] [diff] [review]
patch

>Index: nsTableColGroupFrame.cpp

>@@ -121,71 +121,26 @@ nsTableColGroupFrame::AddColsToTable(nsI
>+  // we did set already the colindex for all colframes in this colgroup after
>+  // the first inserted colframe, but there could be consequitive colgroups
>+  // and their colframes need correct colindices too.

Maybe more like:

 "We have already set the colindex for all the colframes in this
  colgroup that come after the first inserted colframe, but there could
  be other colgroups following this one and their colframes need
  correct colindices too."

I was looking at ResetColIndices, and it looks like it's casting before
checking the type, which is just evil.	Feel free to file a separate bug on
that....

>Index: nsTableColGroupFrame.h

>+  /** Return the colgroup type from the bit 31 and 32 of the colgroups frame
>+    * state. A colgroup can be caused by the content model, as wrapper of
>+    * a content column or as wrapper for cells that need a column and a
>+    * column group around this column

The bit 31/32 thing is an implementation detail; no need to mention it here. 
I'd say something like:

 "A colgroup can be caused by three things:
    1)	An element with table-column-group display
    2)	An element with a table-column display without a
	table-column-group parent
    3)	Cells that are not in a column (and hence get an anonymous
	column and colgroup)."

I'm not sure whether this comment should live next to this function or near
where nsTableColGroupType is defined... arguably, nsTableColGroupType should be
a inner struct (and called nsTableColGroupFrame::eType or something.....). 
Separate bug on that, if desired.

>+  /** Real in this context are colgroups that either derive from context

s/context/content/ ?  That's not so clear... you mean "come from an element
with table-column-group display"?

>+    * or wrap around columns that derive from content.

Same.

> colgroups that are
>+    * the result of creating columns for the cells and those columns need
>+    * then to be wrapped are assumed to be non real.

 "Colgroups that are the result of wrapping cells in an anonymous
  column and colgroup are not considered real here."

>+  /** remove the column aChild from the column group, if requested reenumerate

s/reenumeate/renumber/

>+    * the subsequent columns in this column group and all following col groups.

Consistency?  "column group" vs "col groups"?  Pick one.

>+  /** Add column frames to the table storages: colframe cache and cellmap
>+    * @param aPresContext - the presentation context
>+    * @param aFirstColIndex - at this index aFirstFrame will inserted into
>+    *                         the colframe cache.

"the index at which aFirstFrame should be inserted into the colframe
 cache"

Does this assume anything about whether the colframes are already in the frame
list of the table?  If so, document what it assumes.


>+  /** next sibling to aChildFrame that is column frame, first column
>+    *if aChildFrame is zero
>+    */
>+  nsTableColFrame * GetNextColumn(nsIFrame *aChildFrame);

s/zero/null/?  "that is a column frame".

>+    * @param aStartColFrame - if specified the reset starts with this column
>+    *                         inside the colgroup

add "; if not specified, the reset starts with the first column"

>+inline void nsTableColGroupFrame::SetStartColumnIndex (int aIndex)

That should take a PRInt32, not an int.... (yes, I know the old code was
bogus).

>Index: nsTableFrame.cpp
>-    // remove the col frames, the colGroup frame and reset col indices
>-    colGroup->RemoveChildrenAtEnd(*aPresContext, colGroup->GetColCount());

Yeah, this is totally redundant code.

Don't you need to remove the painting hack that was added in bug 240854?

r+sr=bzbarsky with that

Bug 240245 is next, right?  ;)

Comment 6

[Bernd]

fix checked in