Auto merge of #16378 - emilio:media-fixes, r=upsuper

stylo: A bunch of media query fixes.

<!-- 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/16378)
<!-- Reviewable:end -->

components/style/encoding_support.rs

@@ -12,6 +12,7 @@ use media_queries::MediaList;
 use self::encoding::{EncodingRef, DecoderTrap};
 use shared_lock::SharedRwLock;
 use std::str;
+use std::sync::Arc;
 use stylesheets::{Stylesheet, StylesheetLoader, Origin, UrlExtraData};
 
 struct RustEncoding;
@@ -62,7 +63,7 @@ impl Stylesheet {
         Stylesheet::from_str(&string,
                              url_data,
                              origin,
-                             media,
+                             Arc::new(shared_lock.wrap(media)),
                              shared_lock,
                              stylesheet_loader,
                              error_reporter)

components/style/gecko_bindings/bindings.rs

@@ -463,7 +463,7 @@ extern "C" {
                                 child_sheet: RawServoStyleSheetBorrowed,
                                 base_url_data: *mut RawGeckoURLExtraData,
                                 url_bytes: *const u8, url_length: u32,
-                                media_bytes: *const u8, media_length: u32);
+                                media_list: RawServoMediaListStrong);
 }
 extern "C" {
     pub fn Gecko_MaybeCreateStyleChildrenIterator(node: RawGeckoNodeBorrowed)
@@ -1461,6 +1461,8 @@ extern "C" {
                                               *mut ServoStyleSheet,
                                           data: *const nsACString,
                                           parsing_mode: SheetParsingMode,
+                                          media_list:
+                                              *const RawServoMediaList,
                                           extra_data:
                                               *mut RawGeckoURLExtraData)
      -> RawServoStyleSheetStrong;
@@ -1914,6 +1916,17 @@ extern "C" {
     pub fn Servo_DeclarationBlock_SetTextDecorationColorOverride(declarations:
                                                                      RawServoDeclarationBlockBorrowed);
 }
+extern "C" {
+    pub fn Servo_MediaList_Create() -> RawServoMediaListStrong;
+}
+extern "C" {
+    pub fn Servo_MediaList_DeepClone(list: RawServoMediaListBorrowed)
+     -> RawServoMediaListStrong;
+}
+extern "C" {
+    pub fn Servo_MediaList_Matches(list: RawServoMediaListBorrowed,
+                                   set: RawServoStyleSetBorrowed) -> bool;
+}
 extern "C" {
     pub fn Servo_MediaList_GetText(list: RawServoMediaListBorrowed,
                                    result: *mut nsAString);

components/style/media_queries.rs

@@ -20,7 +20,7 @@ pub use servo::media_queries::{Device, Expression};
 pub use gecko::media_queries::{Device, Expression};
 
 /// A type that encapsulates a media query list.
-#[derive(Debug)]
+#[derive(Debug, Clone)]
 #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
 pub struct MediaList {
     /// The list of media queries.
@@ -35,8 +35,9 @@ impl ToCss for MediaList {
     }
 }
 
-impl Default for MediaList {
-    fn default() -> MediaList {
+impl MediaList {
+    /// Create an empty MediaList.
+    pub fn empty() -> Self {
         MediaList { media_queries: vec![] }
     }
 }
@@ -251,7 +252,7 @@ impl MediaQuery {
 /// https://drafts.csswg.org/mediaqueries/#error-handling
 pub fn parse_media_query_list(context: &ParserContext, input: &mut Parser) -> MediaList {
     if input.is_exhausted() {
-        return Default::default()
+        return MediaList::empty()
     }
 
     let mut media_queries = vec![];

components/style/stylesheets.rs

@@ -135,53 +135,6 @@ impl CssRules {
         })
     }
 
-    /// https://drafts.csswg.org/cssom/#insert-a-css-rule
-    pub fn insert_rule(&mut self,
-                       rule: &str,
-                       parent_stylesheet: &Stylesheet,
-                       index: usize,
-                       nested: bool,
-                       loader: Option<&StylesheetLoader>)
-                       -> Result<CssRule, RulesMutateError> {
-        // Step 1, 2
-        if index > self.0.len() {
-            return Err(RulesMutateError::IndexSize);
-        }
-
-        // Computes the parser state at the given index
-        let state = if nested {
-            None
-        } else if index == 0 {
-            Some(State::Start)
-        } else {
-            self.0.get(index - 1).map(CssRule::rule_state)
-        };
-
-        // Step 3, 4
-        // XXXManishearth should we also store the namespace map?
-        let (new_rule, new_state) =
-            try!(CssRule::parse(&rule, parent_stylesheet, state, loader));
-
-        // Step 5
-        // Computes the maximum allowed parser state at a given index.
-        let rev_state = self.0.get(index).map_or(State::Body, CssRule::rule_state);
-        if new_state > rev_state {
-            // We inserted a rule too early, e.g. inserting
-            // a regular style rule before @namespace rules
-            return Err(RulesMutateError::HierarchyRequest);
-        }
-
-        // Step 6
-        if let CssRule::Namespace(..) = new_rule {
-            if !self.only_ns_or_import() {
-                return Err(RulesMutateError::InvalidState);
-            }
-        }
-
-        self.0.insert(index, new_rule.clone());
-        Ok(new_rule)
-    }
-
     /// https://drafts.csswg.org/cssom/#remove-a-css-rule
     pub fn remove_rule(&mut self, index: usize) -> Result<(), RulesMutateError> {
         // Step 1, 2
@@ -207,6 +160,86 @@ impl CssRules {
     }
 }
 
+/// A trait to implement helpers for `Arc<Locked<CssRules>>`.
+pub trait CssRulesHelpers {
+    /// https://drafts.csswg.org/cssom/#insert-a-css-rule
+    ///
+    /// Written in this funky way because parsing an @import rule may cause us
+    /// to clone a stylesheet from the same document due to caching in the CSS
+    /// loader.
+    ///
+    /// TODO(emilio): We could also pass the write guard down into the loader
+    /// instead, but that seems overkill.
+    fn insert_rule(&self,
+                   lock: &SharedRwLock,
+                   rule: &str,
+                   parent_stylesheet: &Stylesheet,
+                   index: usize,
+                   nested: bool,
+                   loader: Option<&StylesheetLoader>)
+                   -> Result<CssRule, RulesMutateError>;
+}
+
+impl CssRulesHelpers for Arc<Locked<CssRules>> {
+    fn insert_rule(&self,
+                   lock: &SharedRwLock,
+                   rule: &str,
+                   parent_stylesheet: &Stylesheet,
+                   index: usize,
+                   nested: bool,
+                   loader: Option<&StylesheetLoader>)
+                   -> Result<CssRule, RulesMutateError> {
+        let state = {
+            let read_guard = lock.read();
+            let rules = self.read_with(&read_guard);
+
+            // Step 1, 2
+            if index > rules.0.len() {
+                return Err(RulesMutateError::IndexSize);
+            }
+
+            // Computes the parser state at the given index
+            if nested {
+                None
+            } else if index == 0 {
+                Some(State::Start)
+            } else {
+                rules.0.get(index - 1).map(CssRule::rule_state)
+            }
+        };
+
+        // Step 3, 4
+        // XXXManishearth should we also store the namespace map?
+        let (new_rule, new_state) =
+            try!(CssRule::parse(&rule, parent_stylesheet, state, loader));
+
+        {
+            let mut write_guard = lock.write();
+            let mut rules = self.write_with(&mut write_guard);
+            // Step 5
+            // Computes the maximum allowed parser state at a given index.
+            let rev_state = rules.0.get(index).map_or(State::Body, CssRule::rule_state);
+            if new_state > rev_state {
+                // We inserted a rule too early, e.g. inserting
+                // a regular style rule before @namespace rules
+                return Err(RulesMutateError::HierarchyRequest);
+            }
+
+            // Step 6
+            if let CssRule::Namespace(..) = new_rule {
+                if !rules.only_ns_or_import() {
+                    return Err(RulesMutateError::InvalidState);
+                }
+            }
+
+            rules.0.insert(index, new_rule.clone());
+        }
+
+        Ok(new_rule)
+    }
+
+}
+
 /// The structure servo uses to represent a stylesheet.
 #[derive(Debug)]
 pub struct Stylesheet {
@@ -690,7 +723,7 @@ impl Stylesheet {
     pub fn from_str(css: &str,
                     url_data: UrlExtraData,
                     origin: Origin,
-                    media: MediaList,
+                    media: Arc<Locked<MediaList>>,
                     shared_lock: SharedRwLock,
                     stylesheet_loader: Option<&StylesheetLoader>,
                     error_reporter: &ParseErrorReporter) -> Stylesheet {
@@ -704,7 +737,7 @@ impl Stylesheet {
             url_data: url_data,
             namespaces: RwLock::new(namespaces),
             rules: CssRules::new(rules, &shared_lock),
-            media: Arc::new(shared_lock.wrap(media)),
+            media: media,
             shared_lock: shared_lock,
             dirty_on_viewport_size_change: AtomicBool::new(dirty_on_viewport_size_change),
             disabled: AtomicBool::new(false),
@@ -825,8 +858,8 @@ pub trait StylesheetLoader {
     /// before they’re locked, while keeping the trait object-safe.
     fn request_stylesheet(
         &self,
-        media: MediaList,
-        make_import: &mut FnMut(MediaList) -> ImportRule,
+        media: Arc<Locked<MediaList>>,
+        make_import: &mut FnMut(Arc<Locked<MediaList>>) -> ImportRule,
         make_arc: &mut FnMut(ImportRule) -> Arc<Locked<ImportRule>>,
     ) -> Arc<Locked<ImportRule>>;
 }
@@ -836,8 +869,8 @@ struct NoOpLoader;
 impl StylesheetLoader for NoOpLoader {
     fn request_stylesheet(
         &self,
-        media: MediaList,
-        make_import: &mut FnMut(MediaList) -> ImportRule,
+        media: Arc<Locked<MediaList>>,
+        make_import: &mut FnMut(Arc<Locked<MediaList>>) -> ImportRule,
         make_arc: &mut FnMut(ImportRule) -> Arc<Locked<ImportRule>>,
     ) -> Arc<Locked<ImportRule>> {
         make_arc(make_import(media))
@@ -906,6 +939,7 @@ impl<'a> AtRuleParser for TopLevelRuleParser<'a> {
                     let specified_url = SpecifiedUrl::parse_from_string(url_string, &self.context)?;
 
                     let media = parse_media_query_list(&self.context, input);
+                    let media = Arc::new(self.shared_lock.wrap(media));
 
                     let noop_loader = NoOpLoader;
                     let loader = if !specified_url.is_invalid() {
@@ -920,7 +954,7 @@ impl<'a> AtRuleParser for TopLevelRuleParser<'a> {
                             url: specified_url.take().unwrap(),
                             stylesheet: Arc::new(Stylesheet {
                                 rules: CssRules::new(Vec::new(), self.shared_lock),
-                                media: Arc::new(self.shared_lock.wrap(media)),
+                                media: media,
                                 shared_lock: self.shared_lock.clone(),
                                 origin: self.context.stylesheet_origin,
                                 url_data: self.context.url_data.clone(),