544251 - Replace nsPopupFrameList with nsFrameList

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Mats Palmgren (inactive)]

nsPopupSetFrame uses a custom frame list class, nsPopupFrameList,
for its ::popupList children.  It looks like the features it provides
isn't used anymore and can be replaced by nsFrameList.

The features are:
 * the child frames in a nsPopupFrameList has its prev/next-sibling
   pointers set to nsnull
 * it seems to support having nsIContent pointers without a corresponding
   frame, and "filling in" the frame at a later point

None of the above seems to be required/used anymore.

Another thing that is special about ::popupList is that calling
nsPopupSetFrame::GetChildList(nsGkAtoms::popupList) always returns
an empty list.  This causes nsFrameConstructorState::ProcessFrameInsertions
to call SetInitialChildList() when there are existing child frames when it
should have called Append/InsertFrames depending on tree position for
the child content.  Currently, calling nsPopupSetFrame::SetInitialChildList()
*prepends* the frames to the list, so calling it multiple times
is sort of working.  BTW, both AppendFrames and InsertFrames does the same.
All three just calls AddPopupFrameList():
http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsPopupSetFrame.cpp#272

Comment 1

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Comment 2

[Mats Palmgren (inactive)]

BTW, is there any reason we didn't expose ::popupList by implementing
GetChildList(), GetAdditionalChildListName() ?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 425204 [details] [diff] [review]
Patch rev. 1

I'd really like Neil and roc to look this over.  I seem to recall that exposing is not good in terms of hit-testing behavior and the like (as in, we have consumers who absolutely depend on it not being exposed).  I'm not sure why we needed the frames not linked and such, so can't judge whether it's ok to relink them.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hit-testing won't be a problem anymore (if it ever was), because for a frame to show up in hit-testing you have to traverse it during BuildDisplayList. Just adding it to GetAdditionalChildListName won't do anything.

Off the top of my head I don't know of any reason to not expose it.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If Neil doesn't know of a reason, then I think we should go ahead and expose it.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I'd be fine with that if we give the GetAdditionalChildListName consumers a brief look in the process.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 425204 [details] [diff] [review]
Patch rev. 1

I think this patch is fine though

Comment 8

[Mats Palmgren (inactive)]

Filed bug 545156 about exposing ::popupList

http://hg.mozilla.org/mozilla-central/rev/e6d3799ef997