Bug 1357583: style: Make effective_rules return an iterator, stop refcounting the Device. r=heycam

This makes the code cleaner, and also documents the fact that effective_rules
recurses into imports.

No we're not adding the imported stylesheets twice, and we share code with the
invalidation analysis.

MozReview-Commit-ID: DOF2AViTlmR

components/style/gecko/data.rs

@@ -74,7 +74,7 @@ impl PerDocumentStyleDataImpl {
     ///
     /// Implies also a stylesheet flush.
     pub fn reset_device(&mut self, guard: &SharedRwLockReadGuard) {
-        Arc::get_mut(self.stylist.device_mut()).unwrap().reset();
+        self.stylist.device_mut().reset();
         self.stylesheets.force_dirty();
         self.flush_stylesheets::<GeckoElement>(guard, None);
     }

components/style/gecko_string_cache/mod.rs

@@ -34,9 +34,9 @@ pub mod namespace;
 
 pub use self::namespace::{Namespace, WeakNamespace};
 
-// macro_rules! local_name {
+macro_rules! local_name {
-//     ($s: tt) => { atom!($s) }
+    ($s: tt) => { atom!($s) }
-// }
+}
 
 /// A strong reference to a Gecko atom.
 #[derive(PartialEq, Eq)]

components/style/invalidation/mod.rs

@@ -14,7 +14,7 @@ use fnv::FnvHashSet;
 use selector_parser::SelectorImpl;
 use selectors::parser::{Component, Selector};
 use shared_lock::SharedRwLockReadGuard;
-use stylesheets::{CssRule, CssRules, Stylesheet};
+use stylesheets::{CssRule, Stylesheet};
 use stylist::Stylist;
 
 /// An invalidation scope represents a kind of subtree that may need to be
@@ -97,10 +97,13 @@ impl StylesheetInvalidationSet {
             return; // Nothing to do here.
         }
 
-        self.collect_invalidations_for_rule_list(
+        for rule in stylesheet.effective_rules(stylist.device(), guard) {
-            stylesheet.rules.read_with(guard),
+            self.collect_invalidations_for_rule(rule, guard);
-            stylist,
+            if self.fully_invalid {
-            guard);
+                self.invalid_scopes.clear();
+                break;
+            }
+        }
 
         debug!(" > resulting invalidations: {:?}", self.invalid_scopes);
         debug!(" > fully_invalid: {}", self.fully_invalid);
@@ -181,23 +184,6 @@ impl StylesheetInvalidationSet {
         return any_children_invalid
     }
 
-    /// Collects invalidations for a given list of CSS rules.
-    ///
-    /// TODO(emilio): Convert stylesheet.effective_rules into an iterator to
-    /// share code. This needs the ability to stop ASAP.
-    fn collect_invalidations_for_rule_list(
-        &mut self,
-        rules: &CssRules,
-        stylist: &Stylist,
-        guard: &SharedRwLockReadGuard)
-    {
-        for rule in &rules.0 {
-            if !self.collect_invalidations_for_rule(rule, stylist, guard) {
-                return;
-            }
-        }
-    }
-
     fn scan_component(
         component: &Component<SelectorImpl>,
         scope: &mut Option<InvalidationScope>)
@@ -264,77 +250,32 @@ impl StylesheetInvalidationSet {
     }
 
     /// Collects invalidations for a given CSS rule.
-    ///
-    /// Returns true if it needs to keep collecting invalidations for subsequent
-    /// rules in this list.
     fn collect_invalidations_for_rule(
         &mut self,
         rule: &CssRule,
-        stylist: &Stylist,
         guard: &SharedRwLockReadGuard)
-        -> bool
     {
         use stylesheets::CssRule::*;
         debug!("StylesheetInvalidationSet::collect_invalidations_for_rule");
         debug_assert!(!self.fully_invalid, "Not worth to be here!");
 
         match *rule {
-            Document(ref lock) => {
-                let doc_rule = lock.read_with(guard);
-                if !doc_rule.condition.evaluate(stylist.device()) {
-                    return false; // Won't apply anything else after this.
-                }
-            }
             Style(ref lock) => {
                 let style_rule = lock.read_with(guard);
                 for selector in &style_rule.selectors.0 {
                     self.collect_scopes(selector);
                     if self.fully_invalid {
-                        return false;
+                        return;
                     }
                 }
             }
-            Namespace(..) => {
+            Document(..) |
-                // Irrelevant to which selector scopes match.
+            Namespace(..) |
-            }
+            Import(..) |
-            // NB: We need to do it here, we won't visit the appropriate sheet
+            Media(..) |
-            // otherwise!
+            Supports(..) => {
-            Import(ref lock) => {
+                // Do nothing, relevant nested rules are visited as part of the
-                let import_rule = lock.read_with(guard);
+                // iteration.
-
-                let mq = import_rule.stylesheet.media.read_with(guard);
-                if !mq.evaluate(stylist.device(), stylist.quirks_mode()) {
-                    return true;
-                }
-
-                self.collect_invalidations_for_rule_list(
-                    import_rule.stylesheet.rules.read_with(guard),
-                    stylist,
-                    guard);
-            }
-            Media(ref lock) => {
-                let media_rule = lock.read_with(guard);
-
-                let mq = media_rule.media_queries.read_with(guard);
-                if !mq.evaluate(stylist.device(), stylist.quirks_mode()) {
-                    return true;
-                }
-
-                self.collect_invalidations_for_rule_list(
-                    media_rule.rules.read_with(guard),
-                    stylist,
-                    guard);
-            }
-            Supports(ref lock) => {
-                let supports_rule = lock.read_with(guard);
-                if !supports_rule.enabled {
-                    return true;
-                }
-
-                self.collect_invalidations_for_rule_list(
-                    supports_rule.rules.read_with(guard),
-                    stylist,
-                    guard);
             }
             FontFace(..) |
             CounterStyle(..) |
@@ -351,7 +292,5 @@ impl StylesheetInvalidationSet {
                 self.fully_invalid = true;
             }
         }
-
-        return !self.fully_invalid
     }
 }

components/style/stylesheets.rs

@@ -37,11 +37,13 @@ use servo_config::prefs::PREFS;
 #[cfg(not(feature = "gecko"))]
 use servo_url::ServoUrl;
 use shared_lock::{SharedRwLock, Locked, ToCssWithGuard, SharedRwLockReadGuard};
+use smallvec::SmallVec;
 use std::borrow::Borrow;
 use std::cell::Cell;
 use std::fmt;
 use std::mem::align_of;
 use std::os::raw::c_void;
+use std::slice;
 use std::sync::atomic::{AtomicBool, Ordering};
 use str::starts_with_ignore_ascii_case;
 use style_traits::ToCss;
@@ -432,16 +434,6 @@ pub enum CssRuleType {
     Viewport            = 15,
 }
 
-/// Result type for with_nested_rules_mq_and_doc_rule()
-pub enum NestedRulesResult<'a> {
-    /// Only rules
-    Rules(&'a [CssRule]),
-    /// Rules with media queries
-    RulesWithMediaQueries(&'a [CssRule], &'a MediaList),
-    /// Rules with document rule
-    RulesWithDocument(&'a [CssRule], &'a DocumentRule)
-}
-
 #[allow(missing_docs)]
 pub enum SingleRuleParseError {
     Syntax,
@@ -475,60 +467,6 @@ impl CssRule {
         }
     }
 
-    /// Call `f` with the slice of rules directly contained inside this rule.
-    ///
-    /// Note that only some types of rules can contain rules. An empty slice is
-    /// used for others.
-    ///
-    /// This will not recurse down unsupported @supports rules
-    pub fn with_nested_rules_mq_and_doc_rule<F, R>(&self, guard: &SharedRwLockReadGuard, mut f: F) -> R
-    where F: FnMut(NestedRulesResult) -> R {
-        match *self {
-            CssRule::Import(ref lock) => {
-                let rule = lock.read_with(guard);
-                let media = rule.stylesheet.media.read_with(guard);
-                let rules = rule.stylesheet.rules.read_with(guard);
-                // FIXME(emilio): Include the nested rules if the stylesheet is
-                // loaded.
-                f(NestedRulesResult::RulesWithMediaQueries(&rules.0, &media))
-            }
-            CssRule::Namespace(_) |
-            CssRule::Style(_) |
-            CssRule::FontFace(_) |
-            CssRule::CounterStyle(_) |
-            CssRule::Viewport(_) |
-            CssRule::Keyframes(_) |
-            CssRule::Page(_) => {
-                f(NestedRulesResult::Rules(&[]))
-            }
-            CssRule::Media(ref lock) => {
-                let media_rule = lock.read_with(guard);
-                let mq = media_rule.media_queries.read_with(guard);
-                let rules = &media_rule.rules.read_with(guard).0;
-                f(NestedRulesResult::RulesWithMediaQueries(rules, &mq))
-            }
-            CssRule::Supports(ref lock) => {
-                let supports_rule = lock.read_with(guard);
-                let enabled = supports_rule.enabled;
-                if enabled {
-                    let rules = &supports_rule.rules.read_with(guard).0;
-                    f(NestedRulesResult::Rules(rules))
-                } else {
-                    f(NestedRulesResult::Rules(&[]))
-                }
-            }
-            CssRule::Document(ref lock) => {
-                if cfg!(feature = "gecko") {
-                    let document_rule = lock.read_with(guard);
-                    let rules = &document_rule.rules.read_with(guard).0;
-                    f(NestedRulesResult::RulesWithDocument(rules, &document_rule))
-                } else {
-                    unimplemented!()
-                }
-            }
-        }
-    }
-
     // input state is None for a nested rule
     // Returns a parsed CSS rule and the final state of the parser
     #[allow(missing_docs)]
@@ -1003,6 +941,250 @@ impl DocumentRule {
     }
 }
 
+/// A trait that describes statically which rules are iterated for a given
+/// RulesIterator.
+pub trait NestedRuleIterationCondition {
+    /// Whether we should process the nested rules in a given `@import` rule.
+    fn process_import(
+        guard: &SharedRwLockReadGuard,
+        device: &Device,
+        quirks_mode: QuirksMode,
+        rule: &ImportRule)
+        -> bool;
+
+    /// Whether we should process the nested rules in a given `@media` rule.
+    fn process_media(
+        guard: &SharedRwLockReadGuard,
+        device: &Device,
+        quirks_mode: QuirksMode,
+        rule: &MediaRule)
+        -> bool;
+
+    /// Whether we should process the nested rules in a given `@-moz-document` rule.
+    fn process_document(
+        guard: &SharedRwLockReadGuard,
+        device: &Device,
+        quirks_mode: QuirksMode,
+        rule: &DocumentRule)
+        -> bool;
+
+    /// Whether we should process the nested rules in a given `@supports` rule.
+    fn process_supports(
+        guard: &SharedRwLockReadGuard,
+        device: &Device,
+        quirks_mode: QuirksMode,
+        rule: &SupportsRule)
+        -> bool;
+}
+
+/// A struct that represents the condition that a rule applies to the document.
+pub struct EffectiveRules;
+
+impl NestedRuleIterationCondition for EffectiveRules {
+    fn process_import(
+        guard: &SharedRwLockReadGuard,
+        device: &Device,
+        quirks_mode: QuirksMode,
+        rule: &ImportRule)
+        -> bool
+    {
+        rule.stylesheet.media.read_with(guard).evaluate(device, quirks_mode)
+    }
+
+    fn process_media(
+        guard: &SharedRwLockReadGuard,
+        device: &Device,
+        quirks_mode: QuirksMode,
+        rule: &MediaRule)
+        -> bool
+    {
+        rule.media_queries.read_with(guard).evaluate(device, quirks_mode)
+    }
+
+    fn process_document(
+        _: &SharedRwLockReadGuard,
+        device: &Device,
+        _: QuirksMode,
+        rule: &DocumentRule)
+        -> bool
+    {
+        rule.condition.evaluate(device)
+    }
+
+    fn process_supports(
+        _: &SharedRwLockReadGuard,
+        _: &Device,
+        _: QuirksMode,
+        rule: &SupportsRule)
+        -> bool
+    {
+        rule.enabled
+    }
+}
+
+/// A filter that processes all the rules in a rule list.
+pub struct AllRules;
+
+impl NestedRuleIterationCondition for AllRules {
+    fn process_import(
+        _: &SharedRwLockReadGuard,
+        _: &Device,
+        _: QuirksMode,
+        _: &ImportRule)
+        -> bool
+    {
+        true
+    }
+
+    /// Whether we should process the nested rules in a given `@media` rule.
+    fn process_media(
+        _: &SharedRwLockReadGuard,
+        _: &Device,
+        _: QuirksMode,
+        _: &MediaRule)
+        -> bool
+    {
+        true
+    }
+
+    /// Whether we should process the nested rules in a given `@-moz-document` rule.
+    fn process_document(
+        _: &SharedRwLockReadGuard,
+        _: &Device,
+        _: QuirksMode,
+        _: &DocumentRule)
+        -> bool
+    {
+        true
+    }
+
+    /// Whether we should process the nested rules in a given `@supports` rule.
+    fn process_supports(
+        _: &SharedRwLockReadGuard,
+        _: &Device,
+        _: QuirksMode,
+        _: &SupportsRule)
+        -> bool
+    {
+        true
+    }
+}
+
+/// An iterator over all the effective rules of a stylesheet.
+///
+/// NOTE: This iterator recurses into `@import` rules.
+pub type EffectiveRulesIterator<'a, 'b> = RulesIterator<'a, 'b, EffectiveRules>;
+
+/// An iterator over a list of rules.
+pub struct RulesIterator<'a, 'b, C>
+    where 'b: 'a,
+          C: NestedRuleIterationCondition + 'static,
+{
+    device: &'a Device,
+    quirks_mode: QuirksMode,
+    guard: &'a SharedRwLockReadGuard<'b>,
+    stack: SmallVec<[slice::Iter<'a, CssRule>; 3]>,
+    _phantom: ::std::marker::PhantomData<C>,
+}
+
+impl<'a, 'b, C> RulesIterator<'a, 'b, C>
+    where 'b: 'a,
+          C: NestedRuleIterationCondition + 'static,
+{
+    /// Creates a new `RulesIterator` to iterate over `rules`.
+    pub fn new(
+        device: &'a Device,
+        quirks_mode: QuirksMode,
+        guard: &'a SharedRwLockReadGuard<'b>,
+        rules: &'a CssRules)
+        -> Self
+    {
+        let mut stack = SmallVec::new();
+        stack.push(rules.0.iter());
+        Self {
+            device: device,
+            quirks_mode: quirks_mode,
+            guard: guard,
+            stack: stack,
+            _phantom: ::std::marker::PhantomData,
+        }
+    }
+
+    /// Skips all the remaining children of the last nested rule processed.
+    pub fn skip_children(&mut self) {
+        self.stack.pop();
+    }
+}
+
+impl<'a, 'b, C> Iterator for RulesIterator<'a, 'b, C>
+    where 'b: 'a,
+          C: NestedRuleIterationCondition + 'static,
+{
+    type Item = &'a CssRule;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        while let Some(mut nested_iter) = self.stack.pop() {
+             let rule = match nested_iter.next() {
+                Some(r) => r,
+                None => continue,
+            };
+
+            let sub_iter = match *rule {
+                CssRule::Import(ref import_rule) => {
+                    let import_rule = import_rule.read_with(self.guard);
+
+                    if C::process_import(self.guard, self.device, self.quirks_mode, import_rule) {
+                        Some(import_rule.stylesheet.rules.read_with(self.guard).0.iter())
+                    } else {
+                        None
+                    }
+                }
+                CssRule::Document(ref doc_rule) => {
+                    let doc_rule = doc_rule.read_with(self.guard);
+                    if C::process_document(self.guard, self.device, self.quirks_mode, doc_rule) {
+                        Some(doc_rule.rules.read_with(self.guard).0.iter())
+                    } else {
+                        None
+                    }
+                }
+                CssRule::Media(ref lock) => {
+                    let media_rule = lock.read_with(self.guard);
+                    if C::process_media(self.guard, self.device, self.quirks_mode, media_rule) {
+                        Some(media_rule.rules.read_with(self.guard).0.iter())
+                    } else {
+                        None
+                    }
+                }
+                CssRule::Supports(ref lock) => {
+                    let supports_rule = lock.read_with(self.guard);
+                    if C::process_supports(self.guard, self.device, self.quirks_mode, supports_rule) {
+                        Some(supports_rule.rules.read_with(self.guard).0.iter())
+                    } else {
+                        None
+                    }
+                }
+                CssRule::Namespace(_) |
+                CssRule::Style(_) |
+                CssRule::FontFace(_) |
+                CssRule::CounterStyle(_) |
+                CssRule::Viewport(_) |
+                CssRule::Keyframes(_) |
+                CssRule::Page(_) => None,
+            };
+
+            self.stack.push(nested_iter);
+
+            if let Some(sub_iter) = sub_iter {
+                self.stack.push(sub_iter);
+            }
+
+            return Some(rule);
+        }
+
+        None
+    }
+}
+
 impl Stylesheet {
     /// Updates an empty stylesheet from a given string of text.
     pub fn update_from_str(existing: &Stylesheet,
@@ -1132,14 +1314,30 @@ impl Stylesheet {
 
     /// Return an iterator over the effective rules within the style-sheet, as
     /// according to the supplied `Device`.
-    ///
-    /// If a condition does not hold, its associated conditional group rule and
-    /// nested rules will be skipped. Use `rules` if all rules need to be
-    /// examined.
     #[inline]
-    pub fn effective_rules<F>(&self, device: &Device, guard: &SharedRwLockReadGuard, mut f: F)
+    pub fn effective_rules<'a, 'b>(
-    where F: FnMut(&CssRule) {
+        &'a self,
-        effective_rules(&self.rules.read_with(guard).0, device, self.quirks_mode, guard, &mut f);
+        device: &'a Device,
+        guard: &'a SharedRwLockReadGuard<'b>)
+        -> EffectiveRulesIterator<'a, 'b>
+    {
+        self.iter_rules::<'a, 'b, EffectiveRules>(device, guard)
+    }
+
+    /// Return an iterator using the condition `C`.
+    #[inline]
+    pub fn iter_rules<'a, 'b, C>(
+        &'a self,
+        device: &'a Device,
+        guard: &'a SharedRwLockReadGuard<'b>)
+        -> RulesIterator<'a, 'b, C>
+        where C: NestedRuleIterationCondition,
+    {
+        RulesIterator::new(
+            device,
+            self.quirks_mode,
+            guard,
+            &self.rules.read_with(guard))
     }
 
     /// Returns whether the stylesheet has been explicitly disabled through the
@@ -1189,51 +1387,20 @@ impl Clone for Stylesheet {
     }
 }
 
-fn effective_rules<F>(rules: &[CssRule],
-                      device: &Device,
-                      quirks_mode: QuirksMode,
-                      guard: &SharedRwLockReadGuard,
-                      f: &mut F)
-    where F: FnMut(&CssRule)
-{
-    for rule in rules {
-        f(rule);
-        rule.with_nested_rules_mq_and_doc_rule(guard, |result| {
-            let rules = match result {
-                NestedRulesResult::Rules(rules) => {
-                    rules
-                },
-                NestedRulesResult::RulesWithMediaQueries(rules, media_queries) => {
-                    if !media_queries.evaluate(device, quirks_mode) {
-                        return;
-                    }
-                    rules
-                },
-                NestedRulesResult::RulesWithDocument(rules, doc_rule) => {
-                    if !doc_rule.condition.evaluate(device) {
-                        return;
-                    }
-                    rules
-                },
-            };
-            effective_rules(rules, device, quirks_mode, guard, f)
-        })
-    }
-}
-
 macro_rules! rule_filter {
     ($( $method: ident($variant:ident => $rule_type: ident), )+) => {
         impl Stylesheet {
             $(
                 #[allow(missing_docs)]
                 pub fn $method<F>(&self, device: &Device, guard: &SharedRwLockReadGuard, mut f: F)
-                where F: FnMut(&$rule_type) {
+                    where F: FnMut(&$rule_type),
-                    self.effective_rules(device, guard, |rule| {
+                {
+                    for rule in self.effective_rules(device, guard) {
                         if let CssRule::$variant(ref lock) = *rule {
                             let rule = lock.read_with(guard);
                             f(&rule)
                         }
-                    })
+                    }
                 }
             )+
         }

components/style/stylist.rs

@@ -39,10 +39,9 @@ use style_traits::viewport::ViewportConstraints;
 use stylearc::Arc;
 #[cfg(feature = "gecko")]
 use stylesheets::{CounterStyleRule, FontFaceRule};
-use stylesheets::{CssRule, Origin};
+use stylesheets::{CssRule, DocumentRule, ImportRule, MediaRule, StyleRule, SupportsRule};
-use stylesheets::{StyleRule, Stylesheet, UserAgentStylesheets};
+use stylesheets::{Stylesheet, Origin, UserAgentStylesheets};
-#[cfg(feature = "servo")]
+use stylesheets::NestedRuleIterationCondition;
-use stylesheets::NestedRulesResult;
 use thread_state;
 use viewport::{self, MaybeNew, ViewportRule};
 
@@ -79,10 +78,7 @@ pub struct Stylist {
     /// On Servo, on the other hand, the device is a really cheap representation
     /// that is recreated each time some constraint changes and calling
     /// `set_device`.
-    ///
+    device: Device,
-    /// In both cases, the device is actually _owned_ by the Stylist, and it's
-    /// only an `Arc` so we can implement `add_stylesheet` more idiomatically.
-    device: Arc<Device>,
 
     /// Viewport constraints based on the current device.
     viewport_constraints: Option<ViewportConstraints>,
@@ -226,6 +222,57 @@ impl From<StyleRuleInclusion> for RuleInclusion {
     }
 }
 
+/// A filter that filters over effective rules, but allowing all potentially
+/// effective `@media` rules.
+pub struct PotentiallyEffectiveMediaRules;
+
+impl NestedRuleIterationCondition for PotentiallyEffectiveMediaRules {
+    fn process_import(
+        _: &SharedRwLockReadGuard,
+        _: &Device,
+        _: QuirksMode,
+        _: &ImportRule)
+        -> bool
+    {
+        true
+    }
+
+    fn process_media(
+        _: &SharedRwLockReadGuard,
+        _: &Device,
+        _: QuirksMode,
+        _: &MediaRule)
+        -> bool
+    {
+        true
+    }
+
+    /// Whether we should process the nested rules in a given `@-moz-document` rule.
+    fn process_document(
+        guard: &SharedRwLockReadGuard,
+        device: &Device,
+        quirks_mode: QuirksMode,
+        rule: &DocumentRule)
+        -> bool
+    {
+        use stylesheets::EffectiveRules;
+        EffectiveRules::process_document(guard, device, quirks_mode, rule)
+    }
+
+    /// Whether we should process the nested rules in a given `@supports` rule.
+    fn process_supports(
+        guard: &SharedRwLockReadGuard,
+        device: &Device,
+        quirks_mode: QuirksMode,
+        rule: &SupportsRule)
+        -> bool
+    {
+        use stylesheets::EffectiveRules;
+        EffectiveRules::process_supports(guard, device, quirks_mode, rule)
+    }
+}
+
+
 impl Stylist {
     /// Construct a new `Stylist`, using given `Device` and `QuirksMode`.
     /// If more members are added here, think about whether they should
@@ -234,7 +281,7 @@ impl Stylist {
     pub fn new(device: Device, quirks_mode: QuirksMode) -> Self {
         let mut stylist = Stylist {
             viewport_constraints: None,
-            device: Arc::new(device),
+            device: device,
             is_device_dirty: true,
             is_cleared: true,
             quirks_mode: quirks_mode,
@@ -364,8 +411,7 @@ impl Stylist {
             ViewportConstraints::maybe_new(&self.device, &cascaded_rule, self.quirks_mode);
 
         if let Some(ref constraints) = self.viewport_constraints {
-            Arc::get_mut(&mut self.device).unwrap()
+            self.device.account_for_viewport_rule(constraints);
-                .account_for_viewport_rule(constraints);
         }
 
         SelectorImpl::each_eagerly_cascaded_pseudo_element(|pseudo| {
@@ -431,31 +477,47 @@ impl Stylist {
     fn add_stylesheet<'a>(&mut self,
                           stylesheet: &Stylesheet,
                           guard: &SharedRwLockReadGuard,
-                          extra_data: &mut ExtraStyleData<'a>) {
+                          _extra_data: &mut ExtraStyleData<'a>) {
         if stylesheet.disabled() || !stylesheet.is_effective_for_device(&self.device, guard) {
             return;
         }
 
-        // Cheap `Arc` clone so that the closure below can borrow `&mut Stylist`.
+        for rule in stylesheet.effective_rules(&self.device, guard) {
-        let device = self.device.clone();
-
-        stylesheet.effective_rules(&device, guard, |rule| {
             match *rule {
                 CssRule::Style(ref locked) => {
                     let style_rule = locked.read_with(&guard);
                     self.num_declarations += style_rule.block.read_with(&guard).len();
                     for selector in &style_rule.selectors.0 {
                         self.num_selectors += 1;
-                        self.add_rule_to_map(selector, locked, stylesheet);
+
+                        let map = if let Some(pseudo) = selector.pseudo_element() {
+                            self.pseudos_map
+                                .entry(pseudo.canonical())
+                                .or_insert_with(PerPseudoElementSelectorMap::new)
+                                .borrow_for_origin(&stylesheet.origin)
+                        } else {
+                            self.element_map.borrow_for_origin(&stylesheet.origin)
+                        };
+
+                        map.insert(Rule::new(selector.clone(),
+                                             locked.clone(),
+                                             self.rules_source_order));
+
                         self.dependencies.note_selector(selector);
-                        self.note_for_revalidation(selector);
+                        if needs_revalidation(selector) {
-                        self.note_attribute_and_state_dependencies(selector);
+                            self.selectors_for_cache_revalidation.insert(selector.inner.clone());
+                        }
+                        selector.visit(&mut AttributeAndStateDependencyVisitor {
+                            attribute_dependencies: &mut self.attribute_dependencies,
+                            style_attribute_dependency: &mut self.style_attribute_dependency,
+                            state_dependencies: &mut self.state_dependencies,
+                        });
                     }
                     self.rules_source_order += 1;
                 }
-                CssRule::Import(ref import) => {
+                CssRule::Import(..) => {
-                    let import = import.read_with(guard);
+                    // effective_rules visits the inner stylesheet if
-                    self.add_stylesheet(&import.stylesheet, guard, extra_data)
+                    // appropriate.
                 }
                 CssRule::Keyframes(ref keyframes_rule) => {
                     let keyframes_rule = keyframes_rule.read_with(guard);
@@ -474,42 +536,15 @@ impl Stylist {
                 }
                 #[cfg(feature = "gecko")]
                 CssRule::FontFace(ref rule) => {
-                    extra_data.add_font_face(&rule, stylesheet.origin);
+                    _extra_data.add_font_face(&rule, stylesheet.origin);
                 }
                 #[cfg(feature = "gecko")]
                 CssRule::CounterStyle(ref rule) => {
-                    extra_data.add_counter_style(guard, &rule);
+                    _extra_data.add_counter_style(guard, &rule);
                 }
                 // We don't care about any other rule.
                 _ => {}
             }
-        });
-    }
-
-    #[inline]
-    fn add_rule_to_map(&mut self,
-                       selector: &Selector<SelectorImpl>,
-                       rule: &Arc<Locked<StyleRule>>,
-                       stylesheet: &Stylesheet)
-    {
-        let map = if let Some(pseudo) = selector.pseudo_element() {
-            self.pseudos_map
-                .entry(pseudo.canonical())
-                .or_insert_with(PerPseudoElementSelectorMap::new)
-                .borrow_for_origin(&stylesheet.origin)
-        } else {
-            self.element_map.borrow_for_origin(&stylesheet.origin)
-        };
-
-        map.insert(Rule::new(selector.clone(),
-                             rule.clone(),
-                             self.rules_source_order));
-    }
-
-    #[inline]
-    fn note_for_revalidation(&mut self, selector: &Selector<SelectorImpl>) {
-        if needs_revalidation(selector) {
-            self.selectors_for_cache_revalidation.insert(selector.inner.clone());
         }
     }
 
@@ -518,12 +553,7 @@ impl Stylist {
     pub fn might_have_attribute_dependency(&self,
                                            local_name: &LocalName)
                                            -> bool {
-        #[cfg(feature = "servo")]
+        if *local_name == local_name!("style") {
-        let style_lower_name = local_name!("style");
-        #[cfg(feature = "gecko")]
-        let style_lower_name = atom!("style");
-
-        if *local_name == style_lower_name {
             self.style_attribute_dependency
         } else {
             self.attribute_dependencies.might_contain(local_name)
@@ -536,11 +566,6 @@ impl Stylist {
         self.state_dependencies.intersects(state)
     }
 
-    #[inline]
-    fn note_attribute_and_state_dependencies(&mut self, selector: &Selector<SelectorImpl>) {
-        selector.visit(&mut AttributeAndStateDependencyVisitor(self));
-    }
-
     /// Computes the style for a given "precomputed" pseudo-element, taking the
     /// universal rules and applying them.
     ///
@@ -791,44 +816,62 @@ impl Stylist {
             device.account_for_viewport_rule(constraints);
         }
 
-        fn mq_eval_changed(guard: &SharedRwLockReadGuard, rules: &[CssRule],
-                           before: &Device, after: &Device, quirks_mode: QuirksMode) -> bool {
-            for rule in rules {
-                let changed = rule.with_nested_rules_mq_and_doc_rule(guard,
-                                                                     |result| {
-                    let rules = match result {
-                        NestedRulesResult::Rules(rules) => rules,
-                        NestedRulesResult::RulesWithMediaQueries(rules, mq) => {
-                            if mq.evaluate(before, quirks_mode) != mq.evaluate(after, quirks_mode) {
-                                return true;
-                            }
-                            rules
-                        },
-                        NestedRulesResult::RulesWithDocument(rules, doc_rule) => {
-                            if !doc_rule.condition.evaluate(before) {
-                                return false;
-                            }
-                            rules
-                        },
-                    };
-                    mq_eval_changed(guard, rules, before, after, quirks_mode)
-                });
-                if changed {
-                    return true
-                }
-            }
-            false
-        }
         self.is_device_dirty |= stylesheets.iter().any(|stylesheet| {
             let mq = stylesheet.media.read_with(guard);
             if mq.evaluate(&self.device, self.quirks_mode) != mq.evaluate(&device, self.quirks_mode) {
                 return true
             }
 
-            mq_eval_changed(guard, &stylesheet.rules.read_with(guard).0, &self.device, &device, self.quirks_mode)
+            let mut iter =
+                stylesheet.iter_rules::<PotentiallyEffectiveMediaRules>(
+                    &self.device,
+                    guard);
+
+            while let Some(rule) = iter.next() {
+                match *rule {
+                    CssRule::Style(..) |
+                    CssRule::Namespace(..) |
+                    CssRule::FontFace(..) |
+                    CssRule::CounterStyle(..) |
+                    CssRule::Supports(..) |
+                    CssRule::Keyframes(..) |
+                    CssRule::Page(..) |
+                    CssRule::Viewport(..) |
+                    CssRule::Document(..) => {
+                        // Not affected by device changes.
+                        continue;
+                    }
+                    CssRule::Import(ref lock) => {
+                        let import_rule = lock.read_with(guard);
+                        let mq = import_rule.stylesheet.media.read_with(guard);
+                        let effective_now = mq.evaluate(&self.device, self.quirks_mode);
+                        if effective_now != mq.evaluate(&device, self.quirks_mode) {
+                            return true;
+                        }
+
+                        if !effective_now {
+                            iter.skip_children();
+                        }
+                    }
+                    CssRule::Media(ref lock) => {
+                        let media_rule = lock.read_with(guard);
+                        let mq = media_rule.media_queries.read_with(guard);
+                        let effective_now = mq.evaluate(&self.device, self.quirks_mode);
+                        if effective_now != mq.evaluate(&device, self.quirks_mode) {
+                            return true;
+                        }
+
+                        if !effective_now {
+                            iter.skip_children();
+                        }
+                    }
+                }
+            }
+
+            return false;
         });
 
-        self.device = Arc::new(device);
+        self.device = device;
     }
 
     /// Returns the viewport constraints that apply to this document because of
@@ -1119,7 +1162,7 @@ impl Stylist {
     }
 
     /// Accessor for a mutable reference to the device.
-    pub fn device_mut(&mut self) -> &mut Arc<Device> {
+    pub fn device_mut(&mut self) -> &mut Device {
         &mut self.device
     }
 
@@ -1146,7 +1189,11 @@ impl Drop for Stylist {
 
 /// Visitor to collect names that appear in attribute selectors and any
 /// dependencies on ElementState bits.
-struct AttributeAndStateDependencyVisitor<'a>(&'a mut Stylist);
+struct AttributeAndStateDependencyVisitor<'a> {
+    attribute_dependencies: &'a mut BloomFilter,
+    style_attribute_dependency: &'a mut bool,
+    state_dependencies: &'a mut ElementState,
+}
 
 impl<'a> SelectorVisitor for AttributeAndStateDependencyVisitor<'a> {
     type Impl = SelectorImpl;
@@ -1160,17 +1207,17 @@ impl<'a> SelectorVisitor for AttributeAndStateDependencyVisitor<'a> {
         let style_lower_name = atom!("style");
 
         if *lower_name == style_lower_name {
-            self.0.style_attribute_dependency = true;
+            *self.style_attribute_dependency = true;
         } else {
-            self.0.attribute_dependencies.insert(&name);
+            self.attribute_dependencies.insert(&name);
-            self.0.attribute_dependencies.insert(&lower_name);
+            self.attribute_dependencies.insert(&lower_name);
         }
         true
     }
 
     fn visit_simple_selector(&mut self, s: &Component<SelectorImpl>) -> bool {
         if let Component::NonTSPseudoClass(ref p) = *s {
-            self.0.state_dependencies.insert(p.state_flag());
+            self.state_dependencies.insert(p.state_flag());
         }
         true
     }

tests/unit/style/media_queries.rs

@@ -13,7 +13,7 @@ use style::media_queries::*;
 use style::servo::media_queries::*;
 use style::shared_lock::{SharedRwLock, SharedRwLockReadGuard};
 use style::stylearc::Arc;
-use style::stylesheets::{Stylesheet, Origin, CssRule, NestedRulesResult};
+use style::stylesheets::{AllRules, Stylesheet, Origin, CssRule};
 use style::values::specified;
 use style_traits::ToCss;
 
@@ -39,33 +39,18 @@ fn test_media_rule<F>(css: &str, callback: F)
     let stylesheet = Stylesheet::from_str(
         css, url, Origin::Author, media_list, lock,
         None, &CSSErrorReporterTest, QuirksMode::NoQuirks, 0u64);
+    let dummy = Device::new(MediaType::Screen, TypedSize2D::new(200.0, 100.0));
     let mut rule_count = 0;
     let guard = stylesheet.shared_lock.read();
-    media_queries(&guard, &stylesheet.rules.read_with(&guard).0, &mut |mq| {
+    for rule in stylesheet.iter_rules::<AllRules>(&dummy, &guard) {
+        if let CssRule::Media(ref lock) = *rule {
             rule_count += 1;
-        callback(mq, css);
+            callback(&lock.read_with(&guard).media_queries.read_with(&guard), css);
-    });
+        }
+    }
     assert!(rule_count > 0, css_str);
 }
 
-fn media_queries<F>(guard: &SharedRwLockReadGuard, rules: &[CssRule], f: &mut F)
-    where F: FnMut(&MediaList),
-{
-    for rule in rules {
-        rule.with_nested_rules_mq_and_doc_rule(guard, |result| {
-            match result {
-                NestedRulesResult::Rules(rules) |
-                NestedRulesResult::RulesWithDocument(rules, _) => {
-                    media_queries(guard, rules, f)
-                },
-                NestedRulesResult::RulesWithMediaQueries(_, mq) => {
-                    f(mq)
-                }
-            }
-        })
-    }
-}
-
 fn media_query_test(device: &Device, css: &str, expected_rule_count: usize) {
     let url = ServoUrl::parse("http://localhost").unwrap();
     let lock = SharedRwLock::new();