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)
Core
CSS Parsing and Computation
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.
Updated•8 years ago
|
Priority: -- → P3
Comment 1•8 years ago
|
||
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]
Comment 2•8 years ago
|
||
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)
Reporter | ||
Comment 3•8 years ago
|
||
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)
Updated•8 years ago
|
Alias: stylo-cssom
Comment 4•8 years ago
|
||
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
Comment 5•8 years ago
|
||
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.
Comment 6•8 years ago
|
||
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.
Comment 7•8 years ago
|
||
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.)
Comment 8•8 years ago
|
||
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)
Comment 9•8 years ago
|
||
`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)
Assignee | ||
Comment 10•8 years ago
|
||
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)
Comment 11•8 years ago
|
||
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)
Updated•8 years ago
|
Priority: P3 → P1
Whiteboard: [stylo:m2]
Comment 12•7 years ago
|
||
Metabug, P5.
Component: DOM: CSS Object Model → CSS Parsing and Computation
Priority: P1 → P5
Updated•7 years ago
|
Comment 13•7 years ago
|
||
Xidorn, is Stylo support for CSSOM complete? Can we close this CSSOM meta bug?
Flags: needinfo?(xidorn+moz)
Assignee | ||
Comment 14•7 years ago
|
||
Yeah, I think CSSOM support has been completed.
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: needinfo?(xidorn+moz)
Resolution: --- → FIXED
Updated•7 years ago
|
status-firefox57:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•