Auto merge of #17139 - emilio:parsing-simplify, r=nox

style: Simplify a bit the parsing code.

I plan to simplify it further, but this is worth landing on its own.

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

components/style/parser.rs

@@ -7,7 +7,6 @@
 use context::QuirksMode;
 use cssparser::{Parser, SourcePosition, UnicodeRange};
 use error_reporting::ParseErrorReporter;
-use parking_lot::RwLock;
 use style_traits::OneOrMoreCommaSeparated;
 use stylesheets::{CssRuleType, Origin, UrlExtraData, Namespaces};
 
@@ -82,8 +81,8 @@ pub struct ParserContext<'a> {
     pub parsing_mode: ParsingMode,
     /// The quirks mode of this stylesheet.
     pub quirks_mode: QuirksMode,
-    /// The list of all namespaces active in the current stylesheet
+    /// The currently active namespaces.
-    pub namespaces: Option<&'a RwLock<Namespaces>>,
+    pub namespaces: Option<&'a Namespaces>,
 }
 
 impl<'a> ParserContext<'a> {
@@ -108,19 +107,21 @@ impl<'a> ParserContext<'a> {
     }
 
     /// Create a parser context for on-the-fly parsing in CSSOM
-    pub fn new_for_cssom(url_data: &'a UrlExtraData,
+    pub fn new_for_cssom(
-                         error_reporter: &'a ParseErrorReporter,
+        url_data: &'a UrlExtraData,
-                         rule_type: Option<CssRuleType>,
+        error_reporter: &'a ParseErrorReporter,
-                         parsing_mode: ParsingMode,
+        rule_type: Option<CssRuleType>,
-                         quirks_mode: QuirksMode)
+        parsing_mode: ParsingMode,
-                         -> ParserContext<'a> {
+        quirks_mode: QuirksMode
+    ) -> ParserContext<'a> {
         Self::new(Origin::Author, url_data, error_reporter, rule_type, parsing_mode, quirks_mode)
     }
 
     /// Create a parser context based on a previous context, but with a modified rule type.
-    pub fn new_with_rule_type(context: &'a ParserContext,
+    pub fn new_with_rule_type(
-                              rule_type: Option<CssRuleType>)
+        context: &'a ParserContext,
-                              -> ParserContext<'a> {
+        rule_type: Option<CssRuleType>
+    ) -> ParserContext<'a> {
         ParserContext {
             stylesheet_origin: context.stylesheet_origin,
             url_data: context.url_data,
@@ -134,13 +135,14 @@ impl<'a> ParserContext<'a> {
     }
 
     /// Create a parser context for inline CSS which accepts additional line offset argument.
-    pub fn new_with_line_number_offset(stylesheet_origin: Origin,
+    pub fn new_with_line_number_offset(
-                                       url_data: &'a UrlExtraData,
+        stylesheet_origin: Origin,
-                                       error_reporter: &'a ParseErrorReporter,
+        url_data: &'a UrlExtraData,
-                                       line_number_offset: u64,
+        error_reporter: &'a ParseErrorReporter,
-                                       parsing_mode: ParsingMode,
+        line_number_offset: u64,
-                                       quirks_mode: QuirksMode)
+        parsing_mode: ParsingMode,
-                                       -> ParserContext<'a> {
+        quirks_mode: QuirksMode
+    ) -> ParserContext<'a> {
         ParserContext {
             stylesheet_origin: stylesheet_origin,
             url_data: url_data,

components/style/stylesheets.rs

@@ -40,7 +40,6 @@ use shared_lock::{SharedRwLock, Locked, ToCssWithGuard, SharedRwLockReadGuard};
 use smallvec::SmallVec;
 use std::{fmt, mem};
 use std::borrow::Borrow;
-use std::cell::Cell;
 use std::mem::align_of;
 use std::os::raw::c_void;
 use std::slice;
@@ -479,15 +478,19 @@ impl CssRule {
                  loader: Option<&StylesheetLoader>)
                  -> Result<(Self, State), SingleRuleParseError> {
         let error_reporter = NullReporter;
-        let mut context = ParserContext::new(parent_stylesheet.origin,
+        let context = ParserContext::new(
-                                             &parent_stylesheet.url_data,
+            parent_stylesheet.origin,
-                                             &error_reporter,
+            &parent_stylesheet.url_data,
-                                             None,
+            &error_reporter,
-                                             PARSING_MODE_DEFAULT,
+            None,
-                                             parent_stylesheet.quirks_mode);
+            PARSING_MODE_DEFAULT,
-        context.namespaces = Some(&parent_stylesheet.namespaces);
+            parent_stylesheet.quirks_mode
+        );
+
         let mut input = Parser::new(css);
 
+        let mut guard = parent_stylesheet.namespaces.write();
+
         // nested rules are in the body state
         let state = state.unwrap_or(State::Body);
         let mut rule_parser = TopLevelRuleParser {
@@ -495,12 +498,13 @@ impl CssRule {
             context: context,
             shared_lock: &parent_stylesheet.shared_lock,
             loader: loader,
-            state: Cell::new(state),
+            state: state,
+            namespaces: Some(&mut *guard),
         };
         match parse_one_rule(&mut input, &mut rule_parser) {
-            Ok(result) => Ok((result, rule_parser.state.get())),
+            Ok(result) => Ok((result, rule_parser.state)),
             Err(_) => {
-                if let State::Invalid = rule_parser.state.get() {
+                if let State::Invalid = rule_parser.state {
                     Err(SingleRuleParseError::Hierarchy)
                 } else {
                     Err(SingleRuleParseError::Syntax)
@@ -510,8 +514,10 @@ impl CssRule {
     }
 
     /// Deep clones this CssRule.
-    fn deep_clone_with_lock(&self,
+    fn deep_clone_with_lock(
-                            lock: &SharedRwLock) -> CssRule {
+        &self,
+        lock: &SharedRwLock
+    ) -> CssRule {
         let guard = lock.read();
         match *self {
             CssRule::Namespace(ref arc) => {
@@ -1215,10 +1221,19 @@ impl Stylesheet {
         let namespaces = RwLock::new(Namespaces::default());
         // FIXME: we really should update existing.url_data with the given url_data,
         // otherwise newly inserted rule may not have the right base url.
-        let (rules, dirty_on_viewport_size_change) = Stylesheet::parse_rules(
+        let (rules, dirty_on_viewport_size_change) =
-            css, url_data, existing.origin, &namespaces,
+            Stylesheet::parse_rules(
-            &existing.shared_lock, stylesheet_loader, error_reporter,
+                css,
-            existing.quirks_mode, line_number_offset);
+                url_data,
+                existing.origin,
+                &mut *namespaces.write(),
+                &existing.shared_lock,
+                stylesheet_loader,
+                error_reporter,
+                existing.quirks_mode,
+                line_number_offset
+            );
+
         mem::swap(&mut *existing.namespaces.write(), &mut *namespaces.write());
         existing.dirty_on_viewport_size_change
             .store(dirty_on_viewport_size_change, Ordering::Release);
@@ -1228,35 +1243,45 @@ impl Stylesheet {
         *existing.rules.write_with(&mut guard) = CssRules(rules);
     }
 
-    fn parse_rules(css: &str,
+    fn parse_rules(
-                   url_data: &UrlExtraData,
+        css: &str,
-                   origin: Origin,
+        url_data: &UrlExtraData,
-                   namespaces: &RwLock<Namespaces>,
+        origin: Origin,
-                   shared_lock: &SharedRwLock,
+        namespaces: &mut Namespaces,
-                   stylesheet_loader: Option<&StylesheetLoader>,
+        shared_lock: &SharedRwLock,
-                   error_reporter: &ParseErrorReporter,
+        stylesheet_loader: Option<&StylesheetLoader>,
-                   quirks_mode: QuirksMode,
+        error_reporter: &ParseErrorReporter,
-                   line_number_offset: u64)
+        quirks_mode: QuirksMode,
-                   -> (Vec<CssRule>, bool) {
+        line_number_offset: u64
+    ) -> (Vec<CssRule>, bool) {
         let mut rules = Vec::new();
         let mut input = Parser::new(css);
-        let mut context = ParserContext::new_with_line_number_offset(origin, url_data, error_reporter,
+
-                                                                     line_number_offset,
+        let context =
-                                                                     PARSING_MODE_DEFAULT,
+            ParserContext::new_with_line_number_offset(
-                                                                     quirks_mode);
+                origin,
-        context.namespaces = Some(namespaces);
+                url_data,
+                error_reporter,
+                line_number_offset,
+                PARSING_MODE_DEFAULT,
+                quirks_mode
+            );
+
         let rule_parser = TopLevelRuleParser {
             stylesheet_origin: origin,
             shared_lock: shared_lock,
             loader: stylesheet_loader,
             context: context,
-            state: Cell::new(State::Start),
+            state: State::Start,
+            namespaces: Some(namespaces),
         };
 
         input.look_for_viewport_percentages();
 
         {
-            let mut iter = RuleListParser::new_for_stylesheet(&mut input, rule_parser);
+            let mut iter =
+                RuleListParser::new_for_stylesheet(&mut input, rule_parser);
+
             while let Some(result) = iter.next() {
                 match result {
                     Ok(rule) => rules.push(rule),
@@ -1289,9 +1314,17 @@ impl Stylesheet {
                     -> Stylesheet {
         let namespaces = RwLock::new(Namespaces::default());
         let (rules, dirty_on_viewport_size_change) = Stylesheet::parse_rules(
-            css, &url_data, origin, &namespaces,
+            css,
-            &shared_lock, stylesheet_loader, error_reporter, quirks_mode, line_number_offset,
+            &url_data,
+            origin,
+            &mut *namespaces.write(),
+            &shared_lock,
+            stylesheet_loader,
+            error_reporter,
+            quirks_mode,
+            line_number_offset,
         );
+
         Stylesheet {
             origin: origin,
             url_data: url_data,
@@ -1477,7 +1510,8 @@ struct TopLevelRuleParser<'a> {
     shared_lock: &'a SharedRwLock,
     loader: Option<&'a StylesheetLoader>,
     context: ParserContext<'a>,
-    state: Cell<State>,
+    state: State,
+    namespaces: Option<&'a mut Namespaces>,
 }
 
 impl<'b> TopLevelRuleParser<'b> {
@@ -1549,84 +1583,89 @@ impl<'a> AtRuleParser for TopLevelRuleParser<'a> {
     type Prelude = AtRulePrelude;
     type AtRule = CssRule;
 
-    fn parse_prelude(&mut self, name: &str, input: &mut Parser)
+    fn parse_prelude(
-                     -> Result<AtRuleType<AtRulePrelude, CssRule>, ()> {
+        &mut self,
+        name: &str,
+        input: &mut Parser
+    ) -> Result<AtRuleType<AtRulePrelude, CssRule>, ()> {
         let location = get_location_with_offset(input.current_source_location(),
                                                 self.context.line_number_offset);
         match_ignore_ascii_case! { name,
             "import" => {
-                if self.state.get() <= State::Imports {
+                if self.state > State::Imports {
-                    self.state.set(State::Imports);
+                    self.state = State::Invalid;
-                    let url_string = input.expect_url_or_string()?;
-                    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() {
-                        self.loader.expect("Expected a stylesheet loader for @import")
-                    } else {
-                        &noop_loader
-                    };
-
-                    let mut specified_url = Some(specified_url);
-                    let arc = loader.request_stylesheet(media, &mut |media| {
-                        ImportRule {
-                            url: specified_url.take().unwrap(),
-                            stylesheet: Arc::new(Stylesheet {
-                                rules: CssRules::new(Vec::new(), self.shared_lock),
-                                media: media,
-                                shared_lock: self.shared_lock.clone(),
-                                origin: self.context.stylesheet_origin,
-                                url_data: self.context.url_data.clone(),
-                                namespaces: RwLock::new(Namespaces::default()),
-                                dirty_on_viewport_size_change: AtomicBool::new(false),
-                                disabled: AtomicBool::new(false),
-                                quirks_mode: self.context.quirks_mode,
-                            }),
-                            source_location: location,
-                        }
-                    }, &mut |import_rule| {
-                        Arc::new(self.shared_lock.wrap(import_rule))
-                    });
-                    return Ok(AtRuleType::WithoutBlock(CssRule::Import(arc)))
-                } else {
-                    self.state.set(State::Invalid);
                     return Err(())  // "@import must be before any rule but @charset"
                 }
+
+                self.state = State::Imports;
+                let url_string = input.expect_url_or_string()?;
+                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() {
+                    self.loader.expect("Expected a stylesheet loader for @import")
+                } else {
+                    &noop_loader
+                };
+
+                let mut specified_url = Some(specified_url);
+                let arc = loader.request_stylesheet(media, &mut |media| {
+                    ImportRule {
+                        url: specified_url.take().unwrap(),
+                        stylesheet: Arc::new(Stylesheet {
+                            rules: CssRules::new(Vec::new(), self.shared_lock),
+                            media: media,
+                            shared_lock: self.shared_lock.clone(),
+                            origin: self.context.stylesheet_origin,
+                            url_data: self.context.url_data.clone(),
+                            namespaces: RwLock::new(Namespaces::default()),
+                            dirty_on_viewport_size_change: AtomicBool::new(false),
+                            disabled: AtomicBool::new(false),
+                            quirks_mode: self.context.quirks_mode,
+                        }),
+                        source_location: location,
+                    }
+                }, &mut |import_rule| {
+                    Arc::new(self.shared_lock.wrap(import_rule))
+                });
+
+                return Ok(AtRuleType::WithoutBlock(CssRule::Import(arc)))
             },
             "namespace" => {
-                if self.state.get() <= State::Namespaces {
+                if self.state > State::Namespaces {
-                    self.state.set(State::Namespaces);
+                    self.state = State::Invalid;
-
-                    let prefix_result = input.try(|input| input.expect_ident());
-                    let url = Namespace::from(try!(input.expect_url_or_string()));
-
-                    let id = register_namespace(&url)?;
-
-                    let opt_prefix = if let Ok(prefix) = prefix_result {
-                        let prefix = Prefix::from(prefix);
-                        self.context.namespaces.expect("namespaces must be set whilst parsing rules")
-                                               .write().prefixes.insert(prefix.clone(), (url.clone(), id));
-                        Some(prefix)
-                    } else {
-                        self.context.namespaces.expect("namespaces must be set whilst parsing rules")
-                                               .write().default = Some((url.clone(), id));
-                        None
-                    };
-
-                    return Ok(AtRuleType::WithoutBlock(CssRule::Namespace(Arc::new(
-                        self.shared_lock.wrap(NamespaceRule {
-                            prefix: opt_prefix,
-                            url: url,
-                            source_location: location,
-                        })
-                    ))))
-                } else {
-                    self.state.set(State::Invalid);
                     return Err(())  // "@namespace must be before any rule but @charset and @import"
                 }
+                self.state = State::Namespaces;
+
+                let prefix_result = input.try(|input| input.expect_ident());
+                let url = Namespace::from(try!(input.expect_url_or_string()));
+
+                let id = register_namespace(&url)?;
+
+                let mut namespaces = self.namespaces.as_mut().unwrap();
+
+                let opt_prefix = if let Ok(prefix) = prefix_result {
+                    let prefix = Prefix::from(prefix);
+                    namespaces
+                        .prefixes
+                        .insert(prefix.clone(), (url.clone(), id));
+                    Some(prefix)
+                } else {
+                    namespaces.default = Some((url.clone(), id));
+                    None
+                };
+
+                return Ok(AtRuleType::WithoutBlock(CssRule::Namespace(Arc::new(
+                    self.shared_lock.wrap(NamespaceRule {
+                        prefix: opt_prefix,
+                        url: url,
+                        source_location: location,
+                    })
+                ))))
             },
             // @charset is removed by rust-cssparser if it’s the first rule in the stylesheet
             // anything left is invalid.
@@ -1634,11 +1673,18 @@ impl<'a> AtRuleParser for TopLevelRuleParser<'a> {
             _ => {}
         }
         // Don't allow starting with an invalid state
-        if self.state.get() > State::Body {
+        if self.state > State::Body {
-            self.state.set(State::Invalid);
+            self.state = State::Invalid;
             return Err(());
         }
-        self.state.set(State::Body);
+        self.state = State::Body;
+
+        // "Freeze" the namespace map (no more namespace rules can be parsed
+        // after this point), and stick it in the context.
+        if self.namespaces.is_some() {
+            let namespaces = &*self.namespaces.take().unwrap();
+            self.context.namespaces = Some(namespaces);
+        }
         AtRuleParser::parse_prelude(&mut self.nested(), name, input)
     }
 
@@ -1655,13 +1701,24 @@ impl<'a> QualifiedRuleParser for TopLevelRuleParser<'a> {
 
     #[inline]
     fn parse_prelude(&mut self, input: &mut Parser) -> Result<SelectorList<SelectorImpl>, ()> {
-        self.state.set(State::Body);
+        self.state = State::Body;
+
+        // "Freeze" the namespace map (no more namespace rules can be parsed
+        // after this point), and stick it in the context.
+        if self.namespaces.is_some() {
+            let namespaces = &*self.namespaces.take().unwrap();
+            self.context.namespaces = Some(namespaces);
+        }
+
         QualifiedRuleParser::parse_prelude(&mut self.nested(), input)
     }
 
     #[inline]
-    fn parse_block(&mut self, prelude: SelectorList<SelectorImpl>, input: &mut Parser)
+    fn parse_block(
-                   -> Result<CssRule, ()> {
+        &mut self,
+        prelude: SelectorList<SelectorImpl>,
+        input: &mut Parser
+    ) -> Result<CssRule, ()> {
         QualifiedRuleParser::parse_block(&mut self.nested(), prelude, input)
     }
 }
@@ -1674,13 +1731,19 @@ struct NestedRuleParser<'a, 'b: 'a> {
 }
 
 impl<'a, 'b> NestedRuleParser<'a, 'b> {
-    fn parse_nested_rules(&self, input: &mut Parser, rule_type: CssRuleType) -> Arc<Locked<CssRules>> {
+    fn parse_nested_rules(
+        &mut self,
+        input: &mut Parser,
+        rule_type: CssRuleType
+    ) -> Arc<Locked<CssRules>> {
         let context = ParserContext::new_with_rule_type(self.context, Some(rule_type));
+
         let nested_parser = NestedRuleParser {
             stylesheet_origin: self.stylesheet_origin,
             shared_lock: self.shared_lock,
             context: &context,
         };
+
         let mut iter = RuleListParser::new_for_nested_rule(input, nested_parser);
         let mut rules = Vec::new();
         while let Some(result) = iter.next() {
@@ -1711,10 +1774,17 @@ impl<'a, 'b> AtRuleParser for NestedRuleParser<'a, 'b> {
     type Prelude = AtRulePrelude;
     type AtRule = CssRule;
 
-    fn parse_prelude(&mut self, name: &str, input: &mut Parser)
+    fn parse_prelude(
-                     -> Result<AtRuleType<AtRulePrelude, CssRule>, ()> {
+        &mut self,
-        let location = get_location_with_offset(input.current_source_location(),
+        name: &str,
-                                                self.context.line_number_offset);
+        input: &mut Parser
+    ) -> Result<AtRuleType<AtRulePrelude, CssRule>, ()> {
+        let location =
+            get_location_with_offset(
+                input.current_source_location(),
+                self.context.line_number_offset
+            );
+
         match_ignore_ascii_case! { name,
             "media" => {
                 let media_queries = parse_media_query_list(self.context, input);
@@ -1855,16 +1925,19 @@ impl<'a, 'b> QualifiedRuleParser for NestedRuleParser<'a, 'b> {
     type QualifiedRule = CssRule;
 
     fn parse_prelude(&mut self, input: &mut Parser) -> Result<SelectorList<SelectorImpl>, ()> {
-        let ns = self.context.namespaces.expect("namespaces must be set when parsing rules").read();
         let selector_parser = SelectorParser {
             stylesheet_origin: self.stylesheet_origin,
-            namespaces: &*ns,
+            namespaces: self.context.namespaces.unwrap(),
         };
+
         SelectorList::parse(&selector_parser, input)
     }
 
-    fn parse_block(&mut self, prelude: SelectorList<SelectorImpl>, input: &mut Parser)
+    fn parse_block(
-                   -> Result<CssRule, ()> {
+        &mut self,
+        prelude: SelectorList<SelectorImpl>,
+        input: &mut Parser
+    ) -> Result<CssRule, ()> {
         let location = get_location_with_offset(input.current_source_location(),
                                                 self.context.line_number_offset);
         let context = ParserContext::new_with_rule_type(self.context, Some(CssRuleType::Style));

components/style/values/specified/mod.rs

@@ -1311,23 +1311,25 @@ impl Parse for Attr {
 
 #[cfg(feature = "gecko")]
 /// Get the namespace id from the namespace map
-pub fn get_id_for_namespace(namespace: &Namespace, context: &ParserContext) -> Result<NamespaceId, ()> {
+fn get_id_for_namespace(namespace: &Namespace, context: &ParserContext) -> Result<NamespaceId, ()> {
-    if let Some(map) = context.namespaces {
+    let namespaces_map = match context.namespaces {
-        if let Some(ref entry) = map.read().prefixes.get(&namespace.0) {
+        Some(map) => map,
-            Ok(entry.1)
+        None => {
-        } else {
+            // If we don't have a namespace map (e.g. in inline styles)
-            Err(())
+            // we can't parse namespaces
+            return Err(());
         }
-    } else {
+    };
-        // if we don't have a namespace map (e.g. in inline styles)
+
-        // we can't parse namespaces
+    match namespaces_map.prefixes.get(&namespace.0) {
-        Err(())
+        Some(entry) => Ok(entry.1),
+        None => Err(()),
     }
 }
 
 #[cfg(feature = "servo")]
 /// Get the namespace id from the namespace map
-pub fn get_id_for_namespace(_: &Namespace, _: &ParserContext) -> Result<NamespaceId, ()> {
+fn get_id_for_namespace(_: &Namespace, _: &ParserContext) -> Result<NamespaceId, ()> {
     Ok(())
 }
 
@@ -1340,27 +1342,28 @@ impl Attr {
         let first = input.try(|i| i.expect_ident()).ok();
         if let Ok(token) = input.try(|i| i.next_including_whitespace()) {
             match token {
-                Token::Delim('|') => {
+                Token::Delim('|') => {}
-                    // must be followed by an ident
+                _ => return Err(()),
-                    let second_token = match input.next_including_whitespace()? {
-                        Token::Ident(second) => second,
-                        _ => return Err(()),
-                    };
-                    let ns_with_id = if let Some(ns) = first {
-                        let ns: Namespace = ns.into();
-                        let id = get_id_for_namespace(&ns, context)?;
-                        Some((ns, id))
-                    } else {
-                        None
-                    };
-                    return Ok(Attr {
-                        namespace: ns_with_id,
-                        attribute: second_token.into_owned(),
-                    })
-                }
-                _ => return Err(())
             }
+            // must be followed by an ident
+            let second_token = match input.next_including_whitespace()? {
+                Token::Ident(second) => second,
+                _ => return Err(()),
+            };
+
+            let ns_with_id = if let Some(ns) = first {
+                let ns: Namespace = ns.into();
+                let id = get_id_for_namespace(&ns, context)?;
+                Some((ns, id))
+            } else {
+                None
+            };
+            return Ok(Attr {
+                namespace: ns_with_id,
+                attribute: second_token.into_owned(),
+            })
         }
+
         if let Some(first) = first {
             Ok(Attr {
                 namespace: None,