Closed Bug 1517080 Opened 10 months ago Closed 7 months ago

Replace the usage of nsFrameItems with nsFrameList

Categories

(Core :: Layout, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

Details

Attachments

(6 files, 1 obsolete file)

nsFrameItems is an nsFrameList with an additional method AddChild(). I think eliminating nsFrameItems and replacing it by nsFrameList can make the API in nsCSSFrameConstructor more consistent, and avoid the confusion with FrameConstructionItem.
C++ struct defaults to public inheritance. I made this clearer by
writing "public" explicitly. Also, rename AddChild()'s argument to
aFrame so that it's consistent with AppendFrame().

Note that nsFrameItems will be removed in Part 3.
Getting rid of nsFrameItems is nice, but I don't think we should move
AddChild to nsFrameList to achieve that (it was actually intentional
to NOT do that when this was written).  It's just a variation of
AppendFrames IMO (despite its name being singular), so it adds
confusion to the nsFrameList API.  Also, it contains frame tree
assumptions (the OOF assertion) that we shouldn't have in nsFrameList.
Also, we shouldn't encourage passing nsIFrame* that is really a frame
list, since it's slow (have to iterate the entire list to find
the end with nsLayoutUtils::GetLastSibling).

Could we fix the issue with the caption frames that the comment
is talking about and just use nsFrameList::AppendFrames instead?
Maybe the caption frame issue has been fixed?
The methods to add child frames to nsTableWrapperFrame don't have
any special child frame handling AFAICT:
https://searchfox.org/mozilla-central/rev/fef7f858efb695a76010b4c624da5277c16e95b3/layout/tables/nsTableWrapperFrame.cpp#79

nsTableFrame still does though:
https://searchfox.org/mozilla-central/rev/fef7f858efb695a76010b4c624da5277c16e95b3/layout/tables/nsTableFrame.cpp#315-317

Re comment 4:

Mats, are you suggesting that we fixed all the call sites that use nsFrameItems::AddChild() to just use nsFrameList::AppendFrames() so that we can get rid of nsFrameItems::AddChild and nsFrameItems altogether?

Does it matter if we lose the the OOF assertion in nsFrameItems::AddChild()? I guess maybe that's OK because we still have an assertion in nsIFrame::GetPlaceholderFrame() and nsCSSFrameConstructor::CreatePlaceholderFrameFor() is stable enough.

Flags: needinfo?(mats)

(In reply to Ting-Yu Lin [:TYLin] (UTC-8) from comment #6)

Mats, are you suggesting that we fixed all the call sites that use nsFrameItems::AddChild() to just use nsFrameList::AppendFrames() so that we can get rid of nsFrameItems::AddChild and nsFrameItems altogether?

Sounds good to me.

Does it matter if we lose the the OOF assertion in nsFrameItems::AddChild()? I guess maybe that's OK because we still have an assertion in nsIFrame::GetPlaceholderFrame() and nsCSSFrameConstructor::CreatePlaceholderFrameFor() is stable enough.

Yeah, I don't feel that strongly about this assertion, but maybe we can move it to ProcessFrameInsertions instead?

Flags: needinfo?(mats)

This is a straightforward rename. In next part, I am adding back
IsTableCaption() for checking only the table caption style.

In order to get rid of nsFrameItems::AddChild() and use
nsFrameList::AppendFrame() instead, we need to keep table captions'
parent to be nsTableFrame when they're created (no adjust in
ConstructFramesFromItem) so that their parent remain the same as other
frames in the table when appending into the temporary aFrameItems.

The idea to fix this is to ask nsTableWrapperFrame to adopt captions,
i.e. adjust captions' parent frame, when captions are inserting or
appending into nsTableWrapperFrame.

Depends on D25334

This patch is a mechanical replacement without any reparent, i.e.
passing nullptr as parent into nsFrameList::AppendFrame().

Depends on D25336

Attachment #9033842 - Attachment description: Bug 1517080 Part 1 - Move nsFrameItems::AddChild() to nsFrameList, and make nsFrameItems an alias of nFrameList. → Bug 1517080 Part 5 - Make nsFrameItems an alias of nFrameList, and remove nsFrameItems.
Attachment #9033843 - Attachment description: Bug 1517080 Part 2 - Rename nsAbsoluteItems to mozilla::AbsoluteFrameList. → Bug 1517080 Part 6 - Rename nsAbsoluteItems to mozilla::AbsoluteFrameList.
Attachment #9033844 - Attachment description: Bug 1517080 Part 3 - Remove nsFrameItems, and rename various variables with suffix *Items to *List. → Bug 1517080 Part 7 - Remove nsFrameItems alias, and rename variables with suffix "Items" to "List".

FYI, I'm investigating if we can assert that "GetParent() == this"
for the children in SetInitialChildList:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fef6a83f0be1328e48300c23278fcbe160d1fbc0
That invariant seems to hold except for one ColumnSetFrame case,
which I consider a bug (and which was easy to fix).

Attachment #9054380 - Attachment description: Bug 1517080 Part 2 - Do not adjust table captions' parent when they're created. → Bug 1517080 Part 1 - Do not adjust table captions' parent when they're created.
Attachment #9054381 - Attachment description: Bug 1517080 Part 3 - Replace nsFrameItems::AddChild() with nsFrameList::AppendFrame() in FinishBuildingScrollFrame(). → Bug 1517080 Part 2 - Replace nsFrameItems::AddChild() with nsFrameList::AppendFrame() in FinishBuildingScrollFrame().
Attachment #9054382 - Attachment description: Bug 1517080 Part 4 - Replace remaining nsFrameItem::Add() with nsFrameList::AppendFrame(). → Bug 1517080 Part 3 - Replace remaining nsFrameItem::Add() with nsFrameList::AppendFrame().
Attachment #9033842 - Attachment description: Bug 1517080 Part 5 - Make nsFrameItems an alias of nFrameList, and remove nsFrameItems. → Bug 1517080 Part 4 - Make nsFrameItems an alias of nFrameList, and remove nsFrameItems.
Attachment #9033843 - Attachment description: Bug 1517080 Part 6 - Rename nsAbsoluteItems to mozilla::AbsoluteFrameList. → Bug 1517080 Part 5 - Rename nsAbsoluteItems to mozilla::AbsoluteFrameList.
Attachment #9033844 - Attachment description: Bug 1517080 Part 7 - Remove nsFrameItems alias, and rename variables with suffix "Items" to "List". → Bug 1517080 Part 6 - Remove nsFrameItems alias, and rename variables with suffix "Items" to "List".
Attachment #9054379 - Attachment is obsolete: true
Pushed by aethanyc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ae2fe9b13897
Part 1 - Do not adjust table captions' parent when they're created. r=mats
https://hg.mozilla.org/integration/autoland/rev/2721d70f1b6a
Part 2 - Replace nsFrameItems::AddChild() with nsFrameList::AppendFrame() in FinishBuildingScrollFrame(). r=mats
https://hg.mozilla.org/integration/autoland/rev/c838c81514b3
Part 3 - Replace remaining nsFrameItem::Add() with nsFrameList::AppendFrame(). r=mats
https://hg.mozilla.org/integration/autoland/rev/7d5192894473
Part 4 - Make nsFrameItems an alias of nFrameList, and remove nsFrameItems. r=mats
https://hg.mozilla.org/integration/autoland/rev/ede8668de94c
Part 5 - Rename nsAbsoluteItems to mozilla::AbsoluteFrameList. r=mats
https://hg.mozilla.org/integration/autoland/rev/83ef6d9515c9
Part 6 - Remove nsFrameItems alias, and rename variables with suffix "Items" to "List". r=mats
You need to log in before you can comment on or make changes to this bug.