Closed Bug 478956 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Layout, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Proposed fix (obsolete) — Splinter Review
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.
Attachment #362794 - Flags: superreview?(roc)
Attachment #362794 - Flags: review?(bernd_mozilla)
Attachment #362794 - Flags: superreview?(roc) → superreview+
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
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.
Attachment #362794 - Flags: review?(bernd_mozilla) → review+
Boris: please rename this function to something sensible the last thing that this function does is to be a ordinary getter
> 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.
Attachment #362794 - Attachment is obsolete: true
Pushed http://hg.mozilla.org/mozilla-central/rev/4acae1aa5b28
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.