1298634 - Refactor nsCSSShadowArray to be more generic

Status: NEW

(Core :: MFBT, defect)

Description

[Manish Goregaokar [:manishearth]]

nsCSSShadowArray is currently implemented as a struct with two fields: one for the length, and one field of type nsCSSShadowItem[1]. The last field is actually a variable-length array -- when the shadow array is allocated, space for extra elements is taken into account. This struct can't be used on the stack, and the length is immutable.

This is quite similar to how nsTArray works, but we can't use it here because the refcount would need to be stored in a separate heap allocation. We probably could add an optional refcount field to the header via templates somehow, but it seems like assumptions are made about the layout of nsTArray.

We should add a new helper type which works exactly as like nsCSSShadowArray does, but with a generic parameter (probably should make it possible to choose the method of refcounting too) instead of a hardcoded nsCSSShadowItem. I imagine it might be able to be used in other places in style -- most array-like properties are never mutated (only overwritten), and refcounted. nsStyleAutoArray has a capacity field which might be unused in these cases.

Comment 1

[Bobby Holley (:bholley)]

Well, the point is nsStyleAutoArray is that it lets us have inline storage of one element (the common case) within the style struct while entirely avoiding the heap allocation, with the ability to dynamically spill more entries onto the heap if needed. I'm not sure this would solve that use-case.

Comment 2

[Manish Goregaokar [:manishearth]]

Oh, good point -- the heap allocation needs to happen either way if we stick in a refcount (even if we do tweak our generic nsCSSShadowArray to have an inline T)

Though this means that cascading is done via a deep copy with a new allocation, which seems expensive (though the common case is a simple copy). Would be better to switch to something that doesn't do that, but as you noted in IRC we'd need to avoid mutation somehow. Sounds tricky. We already use refptr for things like calc and other properties, but I'll look into it when fixing this.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

I left this on IRC, but anyway: The reason why nsCSSShadowArray is refcounted is most likely due text-shadow, which is an inherited property.

So my guess is you don't want to copy it any time it's inherited, but deep-copying when it's mutated would be acceptable IMO (better ask Cameron or David Baron about that though), something like what we do already for style structs in Rust using Arc::make_mut.