478956 - [FIX]Merge pseudo-parent handling for outer tables and non-table-related frames

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Outer tables and non-table-related frames should really behave identically in terms of pseudo-frame parents: both want a non-table frame or a cell-inner.

Right now we have the relevant code in two different places, and one of those (the one for non-table-related frames) is actually wrong.  Patch coming up that merges the two into a single codepath.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

This patch applies on top of the patches for bug 478820.

The main change is to stop setting the "table part" flag for tables so that they follow the same codepath as non-table-related frames in AdjustParentFrame and remove the code that used to do the same thing in ConstructTableFrame.

The latter checked for table-column or table-column-group parents in addition to what the former checks for, but those can't happen anyway due to AdjustParentFrame suppressing kids of colgroups and due to cols being leaves.

The other difference was in the processing of preexisting pseudo-frames; the code in AdjustParentFrame was wrong.  The attached test part (b) caught that.

Comment 2

[Bernd]

Random notes just to keep them organized I keep them in this bug:

- I have tough times to review this as the function naming does not reflect what the functions are doing.

Namely: nsCSSFrameConstructor::GetParentFrame

It looks to me that the only callers are 
- nsCSSFrameConstructor::ConstructTableFrame
- nsCSSFrameConstructor::ConstructTableCaptionFrame
- nsCSSFrameConstructor::ConstructTableRowGroupFrame
- nsCSSFrameConstructor::ConstructTableColGroupFrame
- nsCSSFrameConstructor::ConstructTableRowFrame
- nsCSSFrameConstructor::ConstructTableColFrame
- nsCSSFrameConstructor::ConstructTableCellFrame

So the function name should have "Table" somewhere to indicate that this something only related to tables. Having "Pseudo" in the function name would give extra bonus points.

What this function does is to walk over the child frame types that the different callers feed into and then creates table pseudo frames if necessary.

So naming candidate would be:

CreateRequiredPseudoFrames

the foreign frame code will never be executed.
so hunk 2 
@@ -3056,26 +3048,21 @@ nsCSSFrameConstructor::GetParentFrame(PR

is correct.

An outer table frame is like most of the frames and can be distiguished from the internal table frames, if those other frames would require a pseudo above then the outer will require a pseudo too.

So hunk 1 requires corresponding actions in nsCSSFrameConstructor::ConstructTableFrame

Comment 3

[Bernd]

GetParentFrame and AdjustParenFrame accomplish roughly the same task: create required pseudo frames if necessary.

One could refactor this  

PRBool tablePart = ((aFCData->mBits & FCDATA_IS_TABLE_PART) != 0);

if (tablePart) call GetParentFrame

and then some part of the table pseudo mumble could be done by createframe internal call to adjustparentframe.

The change to table frame looks accomplish just what hunk 1 required. 

The change to AdjustParentFrame is correct otherwise we will process the pseudo stack wrong.

Comment 4

[Bernd]

Boris: please rename this function to something sensible the last thing that this function does is to be a ordinary getter

Comment 5

[Boris Zbarsky [:bzbarsky]]

> One could refactor this  

Yes.  I plan to do that, as part of a bigger table pseudo refactoring.

Good call on renaming GetParentFrame; I've does exactly that.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: With the renaming

Comment 7

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/4acae1aa5b28