1799732 - Unify the usage of FrameChildListID

Status: RESOLVED FIXED

(Core :: Layout, task)

Description

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

FrameChildListID has many aliases in layout, and its confusing when to use which.

1. nsIFrame redefine all of them again in https://searchfox.org/mozilla-central/rev/0b1543e85d13c30a13c57e959ce9815a3f0fa1d3/layout/generic/nsIFrame.h#1760-1781, and most of the frame class uses them. I guess this is to avoid typing ::layout namespace.

static const ChildListID kPrincipalList = mozilla::layout::kPrincipalList;
static const ChildListID kAbsoluteList = mozilla::layout::kAbsoluteList;
...

2. nsCSSFrameConstructior redefines kPrincipalList in https://searchfox.org/mozilla-central/rev/0b1543e85d13c30a13c57e959ce9815a3f0fa1d3/layout/base/nsCSSFrameConstructor.cpp#134-135

3. Some callers use its original form layout::kPrincipalList, e.g. https://searchfox.org/mozilla-central/rev/0b1543e85d13c30a13c57e959ce9815a3f0fa1d3/layout/xul/nsBoxFrame.cpp#124

4. While some callers spell its full name FrameChildListID::kAbsoluteList, e.g. https://searchfox.org/mozilla-central/rev/0b1543e85d13c30a13c57e959ce9815a3f0fa1d3/layout/generic/ScrollAnchorContainer.cpp#676

Comment 1

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

To improve this, I think we can do the following.

1. Remove all the aliases.
2. Since FrameChildListID is one of the the very few types live under mozilla::layout namespace, we can move it into mozilla namespace, and convert it to an enum class such as

enum class FrameChildListID {
  // The individual concrete child lists.
  PrincipalList,
  PopupList,
  ...

3. Rewrite the callers to use FrameChildListID::PrincipalList instead of kPrincipalList. The only downside is their names become longer ...

Any suggestion to the proposal / naming is welcome.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

I'd avoid having List both in the enum class name and in the variants. FrameChildListID::Principal seems equally clear IMO

Comment 3

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1799732 Part 1 - Replace GetChildList(kPrincipalList) with PrincipalChildList(). r?#layout

Comment 4

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1799732 Part 2 - Flatten the namespace for FrameChildList and co. by removing namespace layout. r?#layout

After moving FrameChildListID into mozilla namespace, kPrincipalList etc. are
also exposed in the mozilla namespace. In the next part, I'll convert
FrameChildListID enum into an enum class, so the naming pollution shouldn't be
an issue.

This patch has a nice side effect that it is now easier to remove all the
aliases of FrameChildListID (kPrincipalList etc.) defined in multiple places
since it is confusion to have the same thing written in different ways, e.g.
nsIFrame::kPrincipalList, mozilla::layout::kPrincipalList,
FrameChildListID::kPrincipalList, kPrincipalList.

This patch doesn't change behavior.

Depends on D161862

Comment 5

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1799732 Part 3 - Convert FrameChildListID to enum class. r?#layout

This patch is first generated by the following script under gecko root folder.

#!/bin/bash

function rename() {
    echo "Renaming $1 to $2"
    rg -l "$1" | xargs sed -i -E -e s/"$1"/"$2"/g
}

rename "kPrincipalList" "FrameChildListID::Principal"
rename "kPopupList" "FrameChildListID::Popup"
rename "kCaptionList" "FrameChildListID::Caption"
rename "kColGroupList" "FrameChildListID::ColGroup"
rename "kAbsoluteList" "FrameChildListID::Absolute"
rename "kFixedList" "FrameChildListID::Fixed"
rename "kOverflowList" "FrameChildListID::Overflow"
rename "kOverflowContainersList" "FrameChildListID::OverflowContainers"
rename "kExcessOverflowContainersList" "FrameChildListID::ExcessOverflowContainers"
rename "kOverflowOutOfFlowList" "FrameChildListID::OverflowOutOfFlow"
rename "kFloatList" "FrameChildListID::Float"
rename "kBulletList" "FrameChildListID::Bullet"
rename "kPushedFloatsList" "FrameChildListID::PushedFloats"
rename "kBackdropList" "FrameChildListID::Backdrop"
rename "kNoReflowPrincipalList" "FrameChildListID::NoReflowPrincipal"

And then:

1. Manually fix FrameChildListID definition in nsFrameList.h.
2. Apply clang-format.

Depends on D161863

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f84bfb41ab8
Part 1 - Replace GetChildList(kPrincipalList) with PrincipalChildList(). r=emilio
https://hg.mozilla.org/integration/autoland/rev/c40fda7a8b0b
Part 2 - Flatten the namespace for FrameChildList and co. by removing namespace layout. r=emilio
https://hg.mozilla.org/integration/autoland/rev/e559f9dbc76f
Part 3 - Convert FrameChildListID to enum class. r=emilio

Comment 7

[Noemi Erli[:noemi_erli]]

Backed out 3 changesets (Bug 1799732) for causing build bustages in nsIFrame.cpp CLOSED TREE

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&collapsedPushes=842382&selectedTaskRun=cnkyQDpWSqWG2tL-kuEEEg.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=e559f9dbc76ff263c8bd9b9f0e15481571e41d5c

Log: https://treeherder.mozilla.org/logviewer?job_id=396323774&repo=autoland&lineNumber=18954

Backout: https://hg.mozilla.org/integration/autoland/rev/df2c2c5f1f58c074eca252982537e204f7d179a8

Comment 8

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Oh, need to convert the changes added in bug 1740365.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6cbd3ceacf7f
Part 1 - Replace GetChildList(kPrincipalList) with PrincipalChildList(). r=emilio
https://hg.mozilla.org/integration/autoland/rev/88e11f80d5d6
Part 2 - Flatten the namespace for FrameChildList and co. by removing namespace layout. r=emilio
https://hg.mozilla.org/integration/autoland/rev/a8df83073f0f
Part 3 - Convert FrameChildListID to enum class. r=emilio

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/6cbd3ceacf7f
https://hg.mozilla.org/mozilla-central/rev/88e11f80d5d6
https://hg.mozilla.org/mozilla-central/rev/a8df83073f0f