1314208 - stylo: Make content of Stylesheet mutable

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Xidorn Quan [:xidorn] UTC+11]

The rules and media of Stylesheet are mutable from CSSOM, so we may need to wrap Stylesheet into RwLock as well.

I tried it a bit myself, but it seems to be a larger change than I thought. Naively replacing all appearance of Stylesheet with RwLock<Stylesheet>, and changing its callsites to either .read() or .write() may not be that hard, but I suppose in at least some of the locations, we want to hold the read lock in advance for a set of stylesheets rather than let every stylesheet be locked separately.

Simon, could you have a look at this?

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Alternatively, we can make only rules wrapped in RwLock rather than the whole Stylesheet. (We may need media to be in RwLock in the future in that case, though).

In addition to that, I hope MediaList can always be wrapped in Arc<RwLock<_>> as well, so that we can reuse code for handling MediaList interface.

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

We can probably do a finer-grained lock, which is to apply RwLock on rules and media, and let Gecko objects hold a reference to RwLock of them. The Option on Stylesheet.media is a problem, though...

Comment 3

[Simon Sapin (:SimonSapin)]

I’ve opened https://github.com/servo/servo/pull/14232.

As to holding locks in advance, it would require allocating a Vec of lock guards so I don’t know if it’s worth it. I think the only case where this would help is in Stylist::set_device which in this PR locks every stylesheet twice.

(By the way, can I make assignments appear in Bugzilla’s "action queue" whose count shows up in red near the top?)

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

So with servo/servo#14190 and servo/servo#14232 get merged, this bug should be considered fixed.