343048 - Make frame construction aware of FrameChildListID::ColGroup (or remove it)

Status: RESOLVED FIXED

(Core :: Layout: Tables, enhancement)

Description

[Mats Palmgren (inactive)]

This bug was spawed off bug 341382 comment 10/11

Make frame construction aware of nsLayoutAtoms::colGroupList (of nsTableFrame).
Then make nsTableFrame.cpp assert that the list is the expected one for the
frame type (SetInitialChildList, AppendFrames, RemoveFrame etc)

Comment 1

[Emilio Cobos Álvarez [:emilio]]

We should just fix this or remove the colgroup list... Daniel, do you happen to know some of the context behind having colgroups (but not rowgroups!) in a separate list? It seems we could probably deal with them similarly to rowgroups which just are on the principal list...

It's non-trivial but worth it IMO, this keeps biting us (see bug 2021043 more recently).

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #1)

Daniel, do you happen to know some of the context behind having colgroups (but not rowgroups!) in a separate list? It seems we could probably deal with them similarly to rowgroups which just are on the principal list...

No, I don't know the context -- sorry -- but I think I found some. Doing a bit of searchfox archeology, it looks like they got split out from the principal list here in 1998:
searchfox view: https://searchfox.org/firefox-main/commit/2c2bdfb430c7256038ab94e93e04d39c33ff6409
github view: https://github.com/mozilla-firefox/firefox/commit/2c2bdfb430c7256038ab94e93e04d39c33ff6409

(Prior to that, it looks like they were part of the main child frame list.)

And the commit message there offers some context, which suggests it was just for convenience to avoid having to check frame types to separate out the different categories (and there wasn't any big reason that we only did it for colgroups but not rowgroups):

the inner table frame now maintains 2 child lists: the main child lists that holds rowgroups and unknown frame
types, and mColGroups. Besides being cleaner and easier to maintain, this should speed some things up slightly
because I don't have to check display types every time I iterate through colgroup frames. I know
mColGroups contains only colgroups. I might do the same for rowgroups (that is, keep unknown frame types
in their own list as well.) But that's optional.

the inner table frame used to incorrectly create actual content objects for anonymous colgroups and cols. Now,
it just creates frames as appropriate, not content. To support this, I added some pseudo style contexts and
related atoms.

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Ok given frame type checks are so much faster now I think we should just try to remove this rather than making the frame constructor deal with this.

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 343048 - Get rid of ColGroup list. r=#layout

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/1e7c1504aef6
https://hg.mozilla.org/integration/autoland/rev/1e3963707013
Get rid of ColGroup list. r=layout-reviewers,dshin

Comment 7

[amarc]

https://hg.mozilla.org/mozilla-central/rev/1e3963707013

Comment 8

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/58473 for changes under testing/web-platform/tests

Comment 9

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot