Auto merge of #14232 - servo:moar-locks, r=upsuper

CSSOM: Make Stylesheet fields have their own synchronization

<!-- Please describe your changes on the following line: -->

r? @upsuper

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix bug 1314208.

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14232)
<!-- Reviewable:end -->

components/layout_thread/Cargo.toml

@@ -24,7 +24,6 @@ lazy_static = "0.2"
 log = "0.3.5"
 msg = {path = "../msg"}
 net_traits = {path = "../net_traits"}
-style_traits = {path = "../style_traits"}
 parking_lot = {version = "0.3.3", features = ["nightly"]}
 plugins = {path = "../plugins"}
 profile_traits = {path = "../profile_traits"}

components/layout_thread/lib.rs

@@ -1060,7 +1060,7 @@ impl LayoutThread {
                        .send(ConstellationMsg::ViewportConstrained(self.id, constraints))
                        .unwrap();
             }
-            if data.document_stylesheets.iter().any(|sheet| sheet.dirty_on_viewport_size_change) {
+            if data.document_stylesheets.iter().any(|sheet| sheet.dirty_on_viewport_size_change()) {
                 let mut iter = node.traverse_preorder();
 
                 let mut next = iter.next();
@@ -1533,6 +1533,7 @@ fn get_ua_stylesheets() -> Result<UserAgentStylesheets, &'static str> {
             None,
             None,
             Origin::UserAgent,
+            Default::default(),
             Box::new(StdoutErrorReporter),
             ParserContextExtraData::default()))
     }
@@ -1545,8 +1546,8 @@ fn get_ua_stylesheets() -> Result<UserAgentStylesheets, &'static str> {
     }
     for &(ref contents, ref url) in &opts::get().user_stylesheets {
         user_or_user_agent_stylesheets.push(Stylesheet::from_bytes(
-            &contents, url.clone(), None, None, Origin::User, Box::new(StdoutErrorReporter),
+            &contents, url.clone(), None, None, Origin::User, Default::default(),
-            ParserContextExtraData::default()));
+            Box::new(StdoutErrorReporter), ParserContextExtraData::default()));
     }
 
     let quirks_mode_stylesheet = try!(parse_ua_stylesheet("quirks-mode.css"));

components/script/dom/htmllinkelement.rs

@@ -370,12 +370,10 @@ impl FetchResponseListener for StylesheetContext {
 
             let win = window_from_node(&*elem);
 
-            let mut sheet = Stylesheet::from_bytes(&data, final_url, protocol_encoding_label,
+            let sheet = Arc::new(Stylesheet::from_bytes(
-                                                   Some(environment_encoding), Origin::Author,
+                &data, final_url, protocol_encoding_label, Some(environment_encoding),
-                                                   win.css_error_reporter(),
+                Origin::Author, self.media.take().unwrap(), win.css_error_reporter(),
-                                                   ParserContextExtraData::default());
+                ParserContextExtraData::default()));
-            sheet.set_media(self.media.take().unwrap());
-            let sheet = Arc::new(sheet);
 
             let win = window_from_node(&*elem);
             win.layout_chan().send(Msg::AddStylesheet(sheet.clone())).unwrap();

components/script/dom/htmlmetaelement.rs

@@ -21,6 +21,7 @@ use html5ever_atoms::LocalName;
 use parking_lot::RwLock;
 use std::ascii::AsciiExt;
 use std::sync::Arc;
+use std::sync::atomic::AtomicBool;
 use style::attr::AttrValue;
 use style::str::HTML_SPACE_CHARACTERS;
 use style::stylesheets::{Stylesheet, CssRule, Origin};
@@ -101,7 +102,7 @@ impl HTMLMetaElement {
                         media: Default::default(),
                         // Viewport constraints are always recomputed on resize; they don't need to
                         // force all styles to be recomputed.
-                        dirty_on_viewport_size_change: false,
+                        dirty_on_viewport_size_change: AtomicBool::new(false),
                     }));
                     let doc = document_from_node(self);
                     doc.invalidate_stylesheets();

components/script/dom/htmlstyleelement.rs

@@ -65,10 +65,9 @@ impl HTMLStyleElement {
         };
 
         let data = node.GetTextContent().expect("Element.textContent must be a string");
-        let mut sheet = Stylesheet::from_str(&data, url, Origin::Author, win.css_error_reporter(),
+        let mq = parse_media_query_list(&mut CssParser::new(&mq_str));
-                                             ParserContextExtraData::default());
+        let sheet = Stylesheet::from_str(&data, url, Origin::Author, mq, win.css_error_reporter(),
-        let mut css_parser = CssParser::new(&mq_str);
+                                         ParserContextExtraData::default());
-        sheet.set_media(parse_media_query_list(&mut css_parser));
         let sheet = Arc::new(sheet);
 
         win.layout_chan().send(Msg::AddStylesheet(sheet.clone())).unwrap();

components/style/stylesheets.rs

@@ -22,6 +22,7 @@ use servo_url::ServoUrl;
 use std::cell::Cell;
 use std::fmt;
 use std::sync::Arc;
+use std::sync::atomic::{AtomicBool, Ordering};
 use style_traits::ToCss;
 use viewport::ViewportRule;
 
@@ -57,9 +58,9 @@ pub struct Stylesheet {
     /// cascading order)
     pub rules: CssRules,
     /// List of media associated with the Stylesheet.
-    pub media: MediaList,
+    pub media: Arc<RwLock<MediaList>>,
     pub origin: Origin,
-    pub dirty_on_viewport_size_change: bool,
+    pub dirty_on_viewport_size_change: AtomicBool,
 }
 
 
@@ -228,35 +229,21 @@ impl ToCss for StyleRule {
 
 
 impl Stylesheet {
-    pub fn from_bytes_iter<I: Iterator<Item=Vec<u8>>>(
-            input: I, base_url: ServoUrl, protocol_encoding_label: Option<&str>,
-            environment_encoding: Option<EncodingRef>, origin: Origin,
-            error_reporter: Box<ParseErrorReporter + Send>,
-            extra_data: ParserContextExtraData) -> Stylesheet {
-        let mut bytes = vec![];
-        // TODO: incremental decoding and tokenization/parsing
-        for chunk in input {
-            bytes.extend_from_slice(&chunk)
-        }
-        Stylesheet::from_bytes(&bytes, base_url, protocol_encoding_label,
-                               environment_encoding, origin, error_reporter,
-                               extra_data)
-    }
-
     pub fn from_bytes(bytes: &[u8],
                       base_url: ServoUrl,
                       protocol_encoding_label: Option<&str>,
                       environment_encoding: Option<EncodingRef>,
-                      origin: Origin, error_reporter: Box<ParseErrorReporter + Send>,
+                      origin: Origin,
+                      media: MediaList,
+                      error_reporter: Box<ParseErrorReporter + Send>,
                       extra_data: ParserContextExtraData)
                       -> Stylesheet {
-        // TODO: bytes.as_slice could be bytes.container_as_bytes()
         let (string, _) = decode_stylesheet_bytes(
             bytes, protocol_encoding_label, environment_encoding);
-        Stylesheet::from_str(&string, base_url, origin, error_reporter, extra_data)
+        Stylesheet::from_str(&string, base_url, origin, media, error_reporter, extra_data)
     }
 
-    pub fn from_str(css: &str, base_url: ServoUrl, origin: Origin,
+    pub fn from_str(css: &str, base_url: ServoUrl, origin: Origin, media: MediaList,
                     error_reporter: Box<ParseErrorReporter + Send>,
                     extra_data: ParserContextExtraData) -> Stylesheet {
         let rule_parser = TopLevelRuleParser {
@@ -286,15 +273,28 @@ impl Stylesheet {
         Stylesheet {
             origin: origin,
             rules: rules.into(),
-            media: Default::default(),
+            media: Arc::new(RwLock::new(media)),
-            dirty_on_viewport_size_change:
+            dirty_on_viewport_size_change: AtomicBool::new(input.seen_viewport_percentages()),
-                input.seen_viewport_percentages(),
         }
     }
 
-    /// Set the MediaList associated with the style-sheet.
+    pub fn dirty_on_viewport_size_change(&self) -> bool {
-    pub fn set_media(&mut self, media: MediaList) {
+        self.dirty_on_viewport_size_change.load(Ordering::SeqCst)
-        self.media = media;
+    }
+
+    /// When CSSOM inserts a rule or declaration into this stylesheet, it needs to call this method
+    /// with the return value of `cssparser::Parser::seen_viewport_percentages`.
+    ///
+    /// FIXME: actually make these calls
+    ///
+    /// Note: when *removing* a rule or declaration that contains a viewport percentage,
+    /// to keep the flag accurate we’d need to iterator through the rest of the stylesheet to
+    /// check for *other* such values.
+    ///
+    /// Instead, we conservatively assume there might be some.
+    /// Restyling will some some more work than necessary, but give correct results.
+    pub fn inserted_has_viewport_percentages(&self, has_viewport_percentages: bool) {
+        self.dirty_on_viewport_size_change.fetch_or(has_viewport_percentages, Ordering::SeqCst);
     }
 
     /// Returns whether the style-sheet applies for the current device depending
@@ -302,7 +302,7 @@ impl Stylesheet {
     ///
     /// Always true if no associated MediaList exists.
     pub fn is_effective_for_device(&self, device: &Device) -> bool {
-        self.media.evaluate(device)
+        self.media.read().evaluate(device)
     }
 
     /// Return an iterator over the effective rules within the style-sheet, as

ports/cef/Cargo.lock

@@ -1203,7 +1203,6 @@ dependencies = [
  "serde_json 0.8.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "servo_url 0.0.1",
  "style 0.0.1",
- "style_traits 0.0.1",
  "util 0.0.1",
  "webrender_traits 0.9.0 (git+https://github.com/servo/webrender)",
 ]

ports/geckolib/glue.rs

@@ -178,8 +178,8 @@ pub extern "C" fn Servo_StyleSheet_Empty(mode: SheetParsingMode) -> RawServoStyl
         SheetParsingMode::eUserSheetFeatures => Origin::User,
         SheetParsingMode::eAgentSheetFeatures => Origin::UserAgent,
     };
-    let sheet = Arc::new(Stylesheet::from_str("", url, origin, Box::new(StdoutErrorReporter),
+    let sheet = Arc::new(Stylesheet::from_str(
-                                              extra_data));
+        "", url, origin, Default::default(), Box::new(StdoutErrorReporter), extra_data));
     unsafe {
         transmute(sheet)
     }
@@ -208,8 +208,8 @@ pub extern "C" fn Servo_StyleSheet_FromUTF8Bytes(data: *const nsACString,
         referrer: Some(GeckoArcURI::new(referrer)),
         principal: Some(GeckoArcPrincipal::new(principal)),
     }};
-    let sheet = Arc::new(Stylesheet::from_str(input, url, origin, Box::new(StdoutErrorReporter),
+    let sheet = Arc::new(Stylesheet::from_str(
-                                              extra_data));
+        input, url, origin, Default::default(), Box::new(StdoutErrorReporter), extra_data));
     unsafe {
         transmute(sheet)
     }

ports/servo/Cargo.lock

@@ -1289,7 +1289,6 @@ dependencies = [
  "serde_json 0.8.1 (registry+https://github.com/rust-lang/crates.io-index)",
  "servo_url 0.0.1",
  "style 0.0.1",
- "style_traits 0.0.1",
  "util 0.0.1",
  "webrender_traits 0.9.0 (git+https://github.com/servo/webrender)",
 ]

tests/unit/style/media_queries.rs

@@ -26,8 +26,9 @@ impl ParseErrorReporter for CSSErrorReporterTest {
 
 fn test_media_rule<F>(css: &str, callback: F) where F: Fn(&MediaList, &str) {
     let url = ServoUrl::parse("http://localhost").unwrap();
-    let stylesheet = Stylesheet::from_str(css, url, Origin::Author, Box::new(CSSErrorReporterTest),
+    let stylesheet = Stylesheet::from_str(
-                                          ParserContextExtraData::default());
+        css, url, Origin::Author, Default::default(),
+        Box::new(CSSErrorReporterTest), ParserContextExtraData::default());
     let mut rule_count = 0;
     media_queries(&stylesheet.rules.0.read(), &mut |mq| {
         rule_count += 1;
@@ -49,8 +50,9 @@ fn media_queries<F>(rules: &[CssRule], f: &mut F) where F: FnMut(&MediaList) {
 
 fn media_query_test(device: &Device, css: &str, expected_rule_count: usize) {
     let url = ServoUrl::parse("http://localhost").unwrap();
-    let ss = Stylesheet::from_str(css, url, Origin::Author, Box::new(CSSErrorReporterTest),
+    let ss = Stylesheet::from_str(
-                                  ParserContextExtraData::default());
+        css, url, Origin::Author, Default::default(),
+        Box::new(CSSErrorReporterTest), ParserContextExtraData::default());
     let mut rule_count = 0;
     ss.effective_style_rules(device, |_| rule_count += 1);
     assert!(rule_count == expected_rule_count, css.to_owned());

tests/unit/style/stylesheets.rs

@@ -12,6 +12,7 @@ use servo_url::ServoUrl;
 use std::borrow::ToOwned;
 use std::sync::Arc;
 use std::sync::Mutex;
+use std::sync::atomic::AtomicBool;
 use style::error_reporting::ParseErrorReporter;
 use style::keyframes::{Keyframe, KeyframeSelector, KeyframePercentage};
 use style::parser::ParserContextExtraData;
@@ -49,13 +50,13 @@ fn test_parse_stylesheet() {
             }
         }";
     let url = ServoUrl::parse("about::test").unwrap();
-    let stylesheet = Stylesheet::from_str(css, url, Origin::UserAgent,
+    let stylesheet = Stylesheet::from_str(css, url, Origin::UserAgent, Default::default(),
                                           Box::new(CSSErrorReporterTest),
                                           ParserContextExtraData::default());
     let expected = Stylesheet {
         origin: Origin::UserAgent,
         media: Default::default(),
-        dirty_on_viewport_size_change: false,
+        dirty_on_viewport_size_change: AtomicBool::new(false),
         rules: vec![
             CssRule::Namespace(Arc::new(RwLock::new(NamespaceRule {
                 prefix: None,
@@ -320,7 +321,7 @@ fn test_report_error_stylesheet() {
 
     let errors = error_reporter.errors.clone();
 
-    Stylesheet::from_str(css, url, Origin::UserAgent, error_reporter,
+    Stylesheet::from_str(css, url, Origin::UserAgent, Default::default(), error_reporter,
                          ParserContextExtraData::default());
 
     let mut errors = errors.lock().unwrap();

tests/unit/style/viewport.rs

@@ -23,6 +23,7 @@ macro_rules! stylesheet {
             $css,
             ServoUrl::parse("http://localhost").unwrap(),
             Origin::$origin,
+            Default::default(),
             $error_reporter,
             ParserContextExtraData::default()
         ))