Closed Bug 1281962 (stylo-cssom) Opened 8 years ago Closed 7 years ago

Stylo: Support CSSOM access to style sheets when using a Servo style system

Categories

(Core :: CSS Parsing and Computation, defect, P5)

defect

Tracking

()

RESOLVED FIXED
Tracking Status
firefox57 --- fixed

People

(Reporter: heycam, Assigned: xidorn)

References

Details

This bug is for supporting CSSOM access to style sheets with stylo.

At least one thing we'll have to do is fix the handling of style="" attribute serialization.  See comment in nsAttrValue::ToString.
Priority: -- → P3
Xidorn is investigating the Gecko side of CSSOM. Simon is investigating the Servo side.
Assignee: nobody → xidorn+moz
Summary: support CSSOM access to style sheets when using a Servo-backed style system → Stylo: Support CSSOM access to style sheets when using a Servo style system
Whiteboard: [stylo:m2]
I was gonna open a github issue about this, but this works too.

One thing I’m worried about is the life-cycle of CSSOM objects. What is responsible for destroying them and when? Consider for example a document with a `<style>` element, where a script gets a JS reference to a CSSStyleSheet object for that element, then removes the element from the document.

Consider also that some objects have `parentRule` and/or `parentStylesheet` attributes, and we’ll need to deal with reference cycles somehow to avoid leaking memory.

The document tree and some other DOM objects have similar cycles. Servo deals with this having SpiderMonkey’s GC manage every DOM object, but my understanding is that Gecko does not do this.

Cameron, how are CSSOM objects managed currently in Gecko?
Flags: needinfo?(cam)
CSSStyleSheet and all of the rule classes are cycle collected objects.

Given the discussions in London about the difficult using Rust objects as the implementations of Web IDL interfaces in Gecko (coupled with the fact that CSSStyleSheet and the rule classes are yet to be converted to Web IDL) it sounds like we should maintain C++ objects that are the things exposed to script, and to have them hold on to their corresponding Servo objects.

I know that the Servo objects don't have a separate JS wrapper and instead have their Reflector directly in the object, but I don't know whether that's a problem if we never touch the Reflector.

Ensuring that the C++ object tree stays in sync with the Servo object tree might be hazardous.
Flags: needinfo?(cam)
Alias: stylo-cssom
Cameron says the plain mochitests under layout/test/test/ that use property_database.js, excluding those specifically testing animations and transitions, need the following CSSOM APIs:

* all property getters and setters on CSS2Properties
* CSSRuleList.get_length
* CSSStyleDeclaration.cssText get and set
* CSSStyleDeclaration.length get
* CSSStyleDeclaration.getPropertyValue
* CSSStyleDeclaration.removeProperty
* CSSStyleDeclaration.setProperty
* CSSStyleSheet.cssRules get
* CSSStyleSheet.insertRule
* Document.styleSheets get
* HTMLElement.style get and set
* HTMLStyleElement.sheet get
* Window.getComputedStyle
The data structures in servo/components/style currently look like this (simplified):

    pub struct Stylesheet {
        pub rules: Vec<Rule>,
    }

    pub enum Rule {
        Style(StyleRule),
        Media(MediaRule),
        // ...
    }

    pub struct StyleRule {
        pub selectors: Vec<Selector>,
        pub declarations: PropertyDeclarationBlock,
    }

Classic Rust ownership. I think we should introduce Arc (atomic reference counted) pointers that the gecko side can clone (AddRef), but without parent pointers to avoid cycles. Something like:

    pub struct Stylesheet {
        pub rules: Vec<Rule>,
    }

    pub enum Rule {
        Style(Arc<StyleRule>),
        Media(Arc<MediaRule>),
        // ...
    }

    pub struct StyleRule {
        pub selectors: Vec<Selector>,
        pub declarations: Arc<PropertyDeclarationBlock>,
    }

Then the Gecko side can have C++ objects that hold an Arc reference to the corresponding Servo objects, and parent pointers. Cycles are collected like in the DOM.

    class CSSStyleSheet {
        Arc<Stylesheet> servo_object;
        nsTArray<RefPtr<CSSRule>> rules;
    }

    class CSSRule {
        RefPtr<CSSRule> parent_rule;
        RefPtr<CSSStylesheet> parent_stylesheet;
        CSSRuleType type;
    }

    class CSSStyleRule: CSSRule {
        Arc<StyleRule> servo_object;
        RefPtr<CSSStyleDeclaration> declarations;
    }

    class CSSStyleDeclaration {
        Arc<PropertyDeclarationBlock> servo_object;
        RefPtr<CSSRule> parent_rule
    }

I’m less certain about how to model inheritance of CSSRule to CSSStyleRule and others. On one hand Rust doesn’t have classes or classic inheritance, on the other hand C++ doesn’t have algebraic sum types like Rust’s enums.

Xidorn suggested creating the Gecko objects lazily as needed. This sounds good to me.
Sticking with algebraic data types on the Servo side sounds like the right approach. We can use inheritance on the C++ side as needed, and just hold differently-typed RefPtrs to the various concrete servo types.

Lazy creation of the gecko objects sounds great.
Depends on: 1292432
Depends on: 1294299
Depends on: 1294742
from https://public.etherpad-mozilla.org/p/stylo-taipei :

Simon is going to move the existing logic from the script component to the style component with some quick hack to unblock the stylo work before the refactor on that he planned to.

String manipulation is still a block of CSSOM as many parts of CSSOM are communicating with script via strings.

The branch dispatch may be needed extensively for support of some of CSSOM classes, which would need some investigation on how to make coding that simpler. (Xidorn: maybe the branch dispatch isn't urgent for CSSOM. We may just use virtual function for them as accessing CSSOM may not be as performance-sensitive as styling.)
Simon, what do you think about using parking_lot::RwLock (a la bug 1305141) instead of DOMRefCell to solve that tricky synchronization issue we were talking about in Taipei? It's basically RefCell with threadsafe borrow flags, which should give us exactly the kind of dynamic enforcement we need.
Flags: needinfo?(simon.sapin)
`StyleRefCell` is made with the assumption that actual locking is prohibitively expensive, but if we if we have fast-enough lock that sounds great. I’ll look into it.
Flags: needinfo?(simon.sapin)
Depends on: 1307357
It seems currently PropertyDeclarationBlocks are all wrapped inside an RwLock. Is that the final solution for mutable Arc'ed style object?

We would need to make more structs mutable for having the full power of CSSOM, specifically, the CSSRule structs, which are Arc'ed in enum CSSRule.

It isn't something currently blocking me, but I'm not sure how much work would it need for Servo side to do that. If there is a lot, probably you can start investigating?
Flags: needinfo?(simon.sapin)
Yes, we discussed in Bug 1305141 that the "parking lot" implementation of RwLock is probably fast enough to use fine-grained locks (probably one in each Arc).

Yes, I’m currently working on adding RwLock in more stylesheet-related things. Expect a pull request soon :)
Flags: needinfo?(simon.sapin)
Priority: P3 → P1
Whiteboard: [stylo:m2]
Depends on: 1313293
Depends on: 1315601
Depends on: 1319614
Depends on: 1323147
Depends on: 1323664
Metabug, P5.
Component: DOM: CSS Object Model → CSS Parsing and Computation
Priority: P1 → P5
Depends on: 1345696
Depends on: 1345697
Depends on: 1345698
Depends on: 1352968
Depends on: 1355394
Depends on: 1363590
Depends on: 1328319
Depends on: 1355721
Depends on: 1366657
Depends on: 1368651
Depends on: 1373193
Blocks: stylo-nightly
No longer blocks: stylo
Xidorn, is Stylo support for CSSOM complete? Can we close this CSSOM meta bug?
Flags: needinfo?(xidorn+moz)
Yeah, I think CSSOM support has been completed.
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(xidorn+moz)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.