Serialize style attributes on demand…

[SimonSapin]

… rather than every time that CSSOM Element.style is modified. Results are not cached in this PR. We could add APIs that work with &mut AttrValue rather than &AttrValue to add some caching.

CC #17399

---

This change is 

[SimonSapin]

@bors-servo try

@glennw, does this help with performance issues in #17399?

[bors-servo]

⌛ Trying commit af24044 with merge 41d7591...

[bors-servo]

💔 Test failed - linux-dev

[SimonSapin]

@bors-servo try

[bors-servo]

⌛ Trying commit b599b2b with merge 86ae135...

[bors-servo]

💔 Test failed - linux-rel-wpt

[SimonSapin]

@bors-servo try

[bors-servo]

⌛ Trying commit 921d142 with merge 9743ab4...

[bors-servo]

💔 Test failed - linux-rel-wpt

[SimonSapin]

Lots of AttrValue::as_* calls panic because the value is not of the expected variants. We might need a more systematic way to track what attribute of what element is supposed to be of what variant, and enforce it at various entry points…

[SimonSapin]

@metajack @glennw I’m gonna put this on hold for now. Glenn’s work-around will be enough for next week’s demo, right?

[metajack]

Yes, the hack is sufficient for next week.

[SimonSapin]

Previously, all variants of the AttrValue enum contained a String of Atom so we could always get (through deref) a &str out of a AttrValue. This PR removes the String from the AttrValue::Declaration to make it generated on demand, and removes the Deref impl.

That means everything that relied on deref needs to do something else. AttrValue already had methods like as_tokens(&self) -> &[Atom] that assume the enum is of a particular variant (in this case AttrValue::TokenList) and panic if it’s not. In that, they’re similar to unwrap. This PR replaces uses of deref with calls to these methods all over the place. But the enum often ends up with a different variant than the one expected, so many tests fail with panics.

I could play whack-a-mole until the test suite passes, but I’m worried that this would leave more panics waiting to happen. So if we want keep pursuing wide usage of as_tokens and friends, I think we need to more systematically track which AttrValue is of which variant. We already have parse_plain_attribute that does it based on element type + attribute name, but not all values go through that code path. We’d need to enforce that more systematically.

For code that needs to deal with potentially any variant, the serialize method returns String. But since AttrValue::Declaration’s contents is protected by a document-wide lock, serialize takes a closure that is only called in the Declaration case to acquire that lock (or use a pre-acquired guard). Using it everywhere would be quite verbose.

Another idea might be to add a method that panics, but only in the AttrValue::Declaration case which is used more rarely.

[pcwalton]

Could we use lazy-init as a more straightforward way to do this? I started doing this and it seemed quite simple. It lets us keep the Deref implementation, and, as an added bonus, it gives us caching.

[SimonSapin]

Looks like lazy-init is a one-time thing, where as a PropertyDeclarationBlock is mutable. Modifying it invalidates a previously-serialized String, which needs to be dropped safely somehow.

[pcwalton]

In that case, we should definitely decide what to do here. I don't like leaving good demo performance gated on temporary hacks; I've been burned by this too many times as folks try our demos and call us out.

[SimonSapin]

I’ll try to hack something with a RwLock<Option<String>> or something like that.

[SimonSapin]

When writing my last comment I forgot that accessing the PropertyDeclarationBlock requires a reference to the per-document lock, so we’re back with the tradeoff from #17476 (comment)

[bors-servo]

☔ The latest upstream changes (presumably #17744) made this pull request unmergeable. Please resolve the merge conflicts.

[jdm]

This does not appear to be going anywhere.