Immutable CSSOM #14190

Manishearth · 2016-11-12T23:45:27Z

This PR is intended to add basic support for all CSSOM interfaces, with the ability to index document.styleSheets and css rule lists, and serializing individual css rules. Handling individual interface methods for CSSRule subclasses can probably be done with easy/medium bugs.

Mutation safety isn't dealt with here; if the css rule list is mutated the CSSOM will be in an inconsistent state. I intend to deal with this via zero sized tokens, see https://groups.google.com/forum/#!topic/mozilla.dev.servo/AnxJoVmtMXQ . I'll handle that when I start making the CSSOM mutable. (Getting the immutable bit landed first opens this up for easy bugs)

This doesn't really change style aside from adding an extra arc in the CSS rule list as discussed in the linked thread. So far this same design can be used by stylo as well when the time comes.

f? @SimonSapin @emilio

cc @upsuper

part of #11420
Todo:

Stubs for rest of the CSSRule subclasses
~~ToCSS impls for CSSRules.~~ May make into easy bugs and stub out in this PR #14195
Cache CSSStyleSheet on the relevant node

This change is

highfive · 2016-11-12T23:45:31Z

Heads up! This PR modifies the following files:

@bholley: components/style/selector_matching.rs, components/style/stylesheets.rs
@KiChjang: components/script/dom/cssrule.rs, components/script/dom/document.rs, components/script/dom/webidls/MediaQueryList.idl, components/script/dom/mod.rs, components/script/dom/cssstylerule.rs, components/script/dom/webidls/CSSStyleRule.webidl, components/script/dom/cssrulelist.rs, components/script/dom/htmlmetaelement.rs, components/script/dom/webidls/CSSStyleSheet.webidl, components/script/dom/webidls/CSSRule.webidl, components/script/dom/stylesheetlist.rs, components/script/dom/cssstylesheet.rs, components/script/dom/stylesheet.rs, components/script/dom/webidls/CSSRuleList.webidl
@fitzgen: components/script/dom/cssrule.rs, components/script/dom/document.rs, components/script/dom/webidls/MediaQueryList.idl, components/script/dom/mod.rs, components/script/dom/cssstylerule.rs, components/script/dom/webidls/CSSStyleRule.webidl, components/script/dom/cssrulelist.rs, components/script/dom/htmlmetaelement.rs, components/script/dom/webidls/CSSStyleSheet.webidl, components/script/dom/webidls/CSSRule.webidl, components/script/dom/stylesheetlist.rs, components/script/dom/cssstylesheet.rs, components/script/dom/stylesheet.rs, components/script/dom/webidls/CSSRuleList.webidl
@emilio: components/style/selector_matching.rs, components/style/stylesheets.rs

highfive · 2016-11-12T23:45:32Z

Warning

These commits modify style and script code, but no tests are modified. Please consider adding a test!

upsuper · 2016-11-13T00:59:39Z

components/style/stylesheets.rs

@@ -112,7 +120,7 @@ pub struct KeyframesRule {
 #[derive(Debug)]
 pub struct MediaRule {
    pub media_queries: Arc<RwLock<MediaList>>,
-    pub rules: Vec<CSSRule>,
+    pub rules: CSSRules,


I think this would likely make it meaningless to wrap MediaRule in RwLock anymore. Probably we can do fine-grained lock everywhere...

Yeah a lot of the rwlocks will move around when I work on the mutable stuff.

@upsuper I don’t understand what you mean here.

@SimonSapin Wrapping content of rules into Arc means we need to further wrap it into RwLock as well, otherwise it would not be as mutable as we expect. And wrapping both fields of MediaRule into Arc<RwLock> means there is likely no more need of mutating the struct itself, so it doesn't make sense anymore to have it wrapped in RwLock.

I see, thanks. I think this is ok for now, and I expect that once we have CSSOM functional in Servo and Stylo we’ll likely to a pass at simplifying/optimizing things.

Manishearth · 2016-11-14T02:59:39Z

Ready for review.

r? @SimonSapin

I have not implemented ToCss, so the .cssText() getter will spew out Debug output for everything but style rules. Going to open bugs for these.

CORS isn't handled either; so you can read the contents of a cross-origin stylesheet. This doesn't seem like a big deal (for now) to me, but I can implement this if necessary.

In the current implementation, the CSSOM is lazy at every step. [SameObject] is satisfied, including for the semantics of the indexers, however CSSOM objects don't get created until you ask for them (but once you ask for them asking for the same one will get you the same object). I'm not sure if we need to do this; if just document.styleSheets[i] was lazy most of the problems of lugging around the full CSSOM for every page that doesn't need it would be solved, but being fully lazy is nice. Let me know if I should change that.

Manishearth · 2016-11-14T03:07:57Z

@bors-servo try

bors-servo · 2016-11-14T03:07:59Z

⌛ Trying commit caac2bc with merge c8193d2...


        Auto merge of #14190 - Manishearth:cssom, r=<try>

@SimonSapin

Immutable CSSOM This PR is intended to add basic support for all CSSOM interfaces, with the ability to index `document.styleSheets` and css rule lists, and serializing individual css rules. Handling individual interface methods for CSSRule subclasses can probably be done with easy/medium bugs. Mutation safety isn't dealt with here; if the css rule list is mutated the CSSOM will be in an inconsistent state. I intend to deal with this via zero sized tokens, see https://groups.google.com/forum/#!topic/mozilla.dev.servo/AnxJoVmtMXQ . I'll handle that when I start making the CSSOM mutable. (Getting the immutable bit landed first opens this up for easy bugs) This doesn't really change style aside from adding an extra arc in the CSS rule list as discussed in the linked thread. So far this same design can be used by stylo as well when the time comes. f? @SimonSapin @emilio cc @upsuper Todo: - [x] Stubs for rest of the CSSRule subclasses - [ ] <s>ToCSS impls for CSSRules.</s> May make into easy bugs and stub out in this PR - [x] Cache CSSStyleSheet on the relevant node  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14190)

bors-servo · 2016-11-14T03:27:20Z

💔 Test failed - linux-dev

Manishearth · 2016-11-14T03:30:46Z

Ongoing builds: mac-rel-css, linux-rel-wpt, linux-rel-css

upsuper · 2016-11-14T07:14:49Z

I suspect some of the changes here may conflict with my patches in bug 1307357, so if this pr gets merged first, I hope there could be a merge from servo/servo to incubator/stylo soonish, so that I can update my patches accordingly.

Manishearth · 2016-11-14T07:17:46Z

There are only two conflicting bits -- the first patch (which you can just import), and the ToCss impl (we can keep whichever lands first). I'll try to get a sync in once this lands.

upsuper · 2016-11-14T07:21:08Z

I think you also want to separate the serialization algorithm of selectors from the ToCss of StyleRule like what I did in my patch, because that is needed in CSSOM.

Manishearth · 2016-11-14T07:22:51Z

Ah, okay. In that case I'll just use your serialization code 😄

Manishearth · 2016-11-14T07:33:28Z

Updated tocss code. Made minor adjustments for better code and to match the spec.

Manishearth · 2016-11-16T17:40:22Z

@bors-servo retry

#13360

bors-servo · 2016-11-16T19:01:47Z

⚡ Previous build results for arm32, arm64, linux-dev, linux-rel-css, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev are reusable. Rebuilding only linux-rel-wpt...

bors-servo · 2016-11-16T19:41:26Z

💔 Test failed - linux-rel-wpt

highfive · 2016-11-16T19:41:28Z

  ▶ FAIL [expected PASS] /_mozilla/css/inline_block_opacity_change.html
  └   → /_mozilla/css/inline_block_opacity_change.html aed6845267bba6c52d3aa69c4889449b454d8117
/_mozilla/css/inline_block_opacity_change_ref.html febec1b4730afab195f280694dad47200b09ea3c
Testing aed6845267bba6c52d3aa69c4889449b454d8117 == febec1b4730afab195f280694dad47200b09ea3c

Manishearth · 2016-11-16T19:42:02Z

@bors-servo retry

#13360


        Disable inline_block_opacity_change.html (intermittent #13360)

Manishearth · 2016-11-16T19:52:30Z

@bors-servo r=SimonSapin

(intermittent disabling r+d in irc)

bors-servo · 2016-11-16T19:52:31Z

📌 Commit 7fcad8a has been approved by SimonSapin

bors-servo · 2016-11-16T19:52:48Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: #14039

bors-servo · 2016-11-16T19:52:49Z

📌 Commit 7fcad8a has been approved by SimonSapin

bors-servo · 2016-11-16T21:06:00Z

⌛ Testing commit 7fcad8a with merge afc60be...


        Auto merge of #14190 - Manishearth:cssom, r=SimonSapin

@SimonSapin

Immutable CSSOM This PR is intended to add basic support for all CSSOM interfaces, with the ability to index `document.styleSheets` and css rule lists, and serializing individual css rules. Handling individual interface methods for CSSRule subclasses can probably be done with easy/medium bugs. Mutation safety isn't dealt with here; if the css rule list is mutated the CSSOM will be in an inconsistent state. I intend to deal with this via zero sized tokens, see https://groups.google.com/forum/#!topic/mozilla.dev.servo/AnxJoVmtMXQ . I'll handle that when I start making the CSSOM mutable. (Getting the immutable bit landed first opens this up for easy bugs) This doesn't really change style aside from adding an extra arc in the CSS rule list as discussed in the linked thread. So far this same design can be used by stylo as well when the time comes. f? @SimonSapin @emilio cc @upsuper part of #11420 Todo: - [x] Stubs for rest of the CSSRule subclasses - [x] <s>ToCSS impls for CSSRules.</s> May make into easy bugs and stub out in this PR #14195 - [x] Cache CSSStyleSheet on the relevant node  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14190)

bors-servo · 2016-11-16T22:40:15Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev


        Auto merge of #14241 - Manishearth:mut-cssom, r=<try>

@Xidorn

CSSOM: Add insertRule() and deleteRule() on CSSStyleSheet; LinkStyle impls on <style> and <link> CSSOM is now starting to be useful! Based on #14190. Only the <s>last commit</s> last two commits need review. cc @Xidorn . This doesn't change the style API, but adds useful methods. part of #11420 r? @SimonSapin  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14241)


        Auto merge of #14241 - Manishearth:mut-cssom, r=<try>

@Xidorn

CSSOM: Add insertRule() and deleteRule() on CSSStyleSheet; LinkStyle impls on <style> and <link> CSSOM is now starting to be useful! Based on #14190. Only the <s>last commit</s> last two commits need review. cc @Xidorn . This doesn't change the style API, but adds useful methods. part of #11420 r? @SimonSapin  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14241)


        Auto merge of #14238 - canaltinova:cssom-tocss, r=Manishearth

@Manishearth

Implement ToCss serialization for CSSRules  Implementation of ToCss serialization for CSSRules. It requires #14190 to merge first to uncomment `CssRule::Style` branch in CssRule's ToCss implementation. r? @Manishearth ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14195 (github issue number if applicable). - [X] These changes do not require tests because it's serialization changes and I'm not sure there is a test for that.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14238)


        Auto merge of #14238 - canaltinova:cssom-tocss, r=Manishearth

@Manishearth

Implement ToCss serialization for CSSRules  Implementation of ToCss serialization for CSSRules. It requires #14190 to merge first to uncomment `CssRule::Style` branch in CssRule's ToCss implementation. r? @Manishearth ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14195 (github issue number if applicable). - [X] These changes do not require tests because it's serialization changes and I'm not sure there is a test for that.   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14238)


        Auto merge of #14241 - Manishearth:mut-cssom, r=<try>

@Xidorn

CSSOM: Whole ton of things CSSOM is now starting to be useful! Based on #14190. Only the <s>last commit</s> last two commits need review. cc @Xidorn . This doesn't change the style API, but adds useful methods. part of #11420 This adds: - `insertRule()` and `deleteRule()` on `CSSStyleSheet`, `CSSGroupingRule` - `.style` getters on link and style elements - Keyframes-backed `CSSRules` and `CSSKeyframesRule.cssRules` - `CSSGroupingRule.cssRules` - `prefix` and `namespaceURI` attributes of `CSSNamespaceRule` - Fixups regarding parent stylesheets r? @SimonSapin  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14241)


        Auto merge of #14241 - Manishearth:mut-cssom, r=SimonSapin

@Xidorn

CSSOM: Whole ton of things CSSOM is now starting to be useful! Based on #14190. Only the <s>last commit</s> last two commits need review. cc @Xidorn . This doesn't change the style API, but adds useful methods. part of #11420 This adds: - `insertRule()` and `deleteRule()` on `CSSStyleSheet`, `CSSGroupingRule` - `.style` getters on link and style elements - Keyframes-backed `CSSRules` and `CSSKeyframesRule.cssRules` - `CSSGroupingRule.cssRules` - `prefix` and `namespaceURI` attributes of `CSSNamespaceRule` - Fixups regarding parent stylesheets r? @SimonSapin  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14241)


        Auto merge of #14241 - Manishearth:mut-cssom, r=SimonSapin

@Xidorn

CSSOM: Whole ton of things CSSOM is now starting to be useful! Based on #14190. Only the <s>last commit</s> last two commits need review. cc @Xidorn . This doesn't change the style API, but adds useful methods. part of #11420 This adds: - `insertRule()` and `deleteRule()` on `CSSStyleSheet`, `CSSGroupingRule` - `.style` getters on link and style elements - Keyframes-backed `CSSRules` and `CSSKeyframesRule.cssRules` - `CSSGroupingRule.cssRules` - `prefix` and `namespaceURI` attributes of `CSSNamespaceRule` - Fixups regarding parent stylesheets r? @SimonSapin  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14241)

highfive assigned metajack Nov 12, 2016

highfive added the S-awaiting-review label Nov 12, 2016

upsuper reviewed Nov 13, 2016

View changes

Manishearth force-pushed the Manishearth:cssom branch 4 times, most recently from 5be9528 to d4ef7a0 Nov 13, 2016

Manishearth changed the title ~~[WIP] Immutable CSSOM~~ Immutable CSSOM Nov 14, 2016

Manishearth force-pushed the Manishearth:cssom branch from d4ef7a0 to 339259d Nov 14, 2016

highfive assigned SimonSapin and unassigned metajack Nov 14, 2016

Manishearth force-pushed the Manishearth:cssom branch from 339259d to caac2bc Nov 14, 2016

highfive added the S-tests-failed label Nov 14, 2016

Manishearth force-pushed the Manishearth:cssom branch from caac2bc to d6e95ee Nov 14, 2016

highfive removed the S-tests-failed label Nov 14, 2016

Manishearth mentioned this pull request Nov 14, 2016

Implement ToCss serialization for CSSRules #14195

Closed

Manishearth force-pushed the Manishearth:cssom branch from d6e95ee to 2f80643 Nov 14, 2016

highfive added the S-tests-failed label Nov 16, 2016

Disable inline_block_opacity_change.html (intermittent #13360)

Loading status checks…

7fcad8a

highfive added S-awaiting-review and removed S-tests-failed labels Nov 16, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 16, 2016

Manishearth mentioned this pull request Nov 16, 2016

Intermittent failure of /_mozilla/css/inline_block_opacity_change.html #13360

Open

bors-servo merged commit 7fcad8a into servo:master Nov 16, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Nov 16, 2016

bors-servo mentioned this pull request Nov 16, 2016

CSSOM: Make Stylesheet fields have their own synchronization #14232

Merged

3 of 5 tasks complete

Manishearth deleted the Manishearth:cssom branch Nov 16, 2016

servo / servo

Immutable CSSOM #14190

Immutable CSSOM #14190

Manishearth commented Nov 12, 2016 •

edited

highfive commented Nov 12, 2016

highfive commented Nov 12, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented Nov 14, 2016

Manishearth commented Nov 14, 2016

bors-servo commented Nov 14, 2016

bors-servo commented Nov 14, 2016

Manishearth commented Nov 14, 2016

upsuper commented Nov 14, 2016

Manishearth commented Nov 14, 2016

upsuper commented Nov 14, 2016

Manishearth commented Nov 14, 2016

Manishearth commented Nov 14, 2016

Manishearth commented Nov 16, 2016

bors-servo commented Nov 16, 2016

bors-servo commented Nov 16, 2016

highfive commented Nov 16, 2016

Manishearth commented Nov 16, 2016

Manishearth commented Nov 16, 2016 •

edited

bors-servo commented Nov 16, 2016

bors-servo commented Nov 16, 2016

bors-servo commented Nov 16, 2016

bors-servo commented Nov 16, 2016

bors-servo commented Nov 16, 2016

servo / servo

Join GitHub today

Immutable CSSOM #14190

Immutable CSSOM #14190

Conversation

Manishearth commented Nov 12, 2016 • edited

highfive commented Nov 12, 2016

highfive commented Nov 12, 2016

This comment has been minimized.

upsuper Nov 13, 2016

This comment has been minimized.

Manishearth Nov 13, 2016

This comment has been minimized.

SimonSapin Nov 14, 2016

This comment has been minimized.

upsuper Nov 14, 2016

This comment has been minimized.

SimonSapin Nov 14, 2016

Manishearth commented Nov 14, 2016

Manishearth commented Nov 14, 2016

bors-servo commented Nov 14, 2016

bors-servo commented Nov 14, 2016

Manishearth commented Nov 14, 2016

upsuper commented Nov 14, 2016

Manishearth commented Nov 14, 2016

upsuper commented Nov 14, 2016

Manishearth commented Nov 14, 2016

Manishearth commented Nov 14, 2016

Manishearth commented Nov 16, 2016

bors-servo commented Nov 16, 2016

bors-servo commented Nov 16, 2016

highfive commented Nov 16, 2016

Manishearth commented Nov 16, 2016

Manishearth commented Nov 16, 2016 • edited

bors-servo commented Nov 16, 2016

bors-servo commented Nov 16, 2016

bors-servo commented Nov 16, 2016

bors-servo commented Nov 16, 2016

bors-servo commented Nov 16, 2016

Manishearth commented Nov 12, 2016 •

edited

Manishearth commented Nov 16, 2016 •

edited