While fixing Bug 1544895, gw asserted to me that clips should only be parented by clips, and not clip-chains. And indeed, changing the code in question to respect this fixed the bug.

However gecko currently has many paths which parent clips with clip-chains. In fact, it's the default for DefineClip, which only the ClipManager overrides (but it's responsible for most of our clips, so that's good).

Things to clarify:

• Are we really not supposed to parent clips with clip-chains?
• Should we change define_clip to only take a (usize, PipelineId) (which is what ClipId::Clip is)?
• Do we need to fix all the callers of DefineClip?

The bot thinks this bug is a task, but please change it back in case of error.

The bullets and legend-border in the attached example with today's nightly both have clips that should be considered "corrupted" under this model. Although only the legend border comes out looking corrupted (scrolling on top of the page).

I think the notable difference is the bullets are like:

Item(spaceAndClip: Clip(parent: ClipChain[...]))

while the legend is:

Item(spaceAndClip: ClipChain[Clip(parent: ClipChain[...])])

But I think the former working is mostly an accident?

ok talked over some of this with gw, and we quickly concluded he was working off of an outdated understanding of the clip model. In particular, kvark rewrote the clip system in https://github.com/servo/webrender/pull/3251/

ni?kvark so he'll see this and hopefully have some context when I talk to him about it tomorrow morning :)

Talked this over with Alexis. We are doing things wrong w.r.t. parenting clips, although my refactor didn't affect this (merely exposed this more clearly). The plan is to not have clips parented to anything, and potentially only allow items to be clipped by clip chains.

concrete proposal:

• ClipDisplayItem.space_and_clip => ClipDisplayItem.parent_spatial_id, removing the ability for clips to have any relation to eachother (bonus: smaller clips in DL).

• replace ClipId with ClipChainId in almost all places (mandate that only clip chains are used for display item clipping -- which gecko mostly already does already, the only exceptions being the sometimes-corrupted clips that spurred the creation of this bug).

• unclear: replace a ClipChainItem's Vec<ClipId> with NonClipChainClipId (placeholder name)? Either clip chains aren't supposed to be containing ClipChains, or else ClipChain.parent is pointless..?

• change WebRenderAPI.h/.cpp and the users of DefineClip/DefineClipChain to reflect these changes (~10 places in the codebase). Ideally should be ~foolproof given the above webrender-side changes.

I worked on this for a bit but didn't like what it was doing to the API. We don't have any currently known bugs from this, so I'm de-prioritizing it. Might push up what I had just for future work if we ever get around to this.