Auto merge of #18142 - emilio:less-mess-ns, r=SimonSapin

style: Less messy namespace handling.

This PR accounts for the fact that the namespace table is only needed in
`NestedRuleParser`, and only for style rules, in order to simplify the setup and
be able to fix a few bugs wrt parsing of invalid rules.

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

components/script/dom/css.rs

@@ -36,9 +36,13 @@ impl CSS {
         decl.push_str(&value);
         let decl = Declaration(decl);
         let url = win.Document().url();
-        let context = ParserContext::new_for_cssom(&url, win.css_error_reporter(), Some(CssRuleType::Supports),
+        let context = ParserContext::new_for_cssom(
-                                                   PARSING_MODE_DEFAULT,
+            &url,
-                                                   QuirksMode::NoQuirks);
+            win.css_error_reporter(),
+            Some(CssRuleType::Style),
+            PARSING_MODE_DEFAULT,
+            QuirksMode::NoQuirks
+        );
         decl.eval(&context)
     }
 
@@ -49,9 +53,13 @@ impl CSS {
         let cond = parse_condition_or_declaration(&mut input);
         if let Ok(cond) = cond {
             let url = win.Document().url();
-            let context = ParserContext::new_for_cssom(&url, win.css_error_reporter(), Some(CssRuleType::Supports),
+            let context = ParserContext::new_for_cssom(
-                                                       PARSING_MODE_DEFAULT,
+                &url,
-                                                       QuirksMode::NoQuirks);
+                win.css_error_reporter(),
+                Some(CssRuleType::Style),
+                PARSING_MODE_DEFAULT,
+                QuirksMode::NoQuirks
+            );
             cond.eval(&context)
         } else {
             false

components/style/parser.rs

@@ -61,13 +61,14 @@ pub struct ParserContext<'a> {
 
 impl<'a> ParserContext<'a> {
     /// Create a parser context.
-    pub fn new(stylesheet_origin: Origin,
+    pub fn new(
-               url_data: &'a UrlExtraData,
+        stylesheet_origin: Origin,
-               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> {
         ParserContext {
             stylesheet_origin: stylesheet_origin,
             url_data: url_data,
@@ -88,23 +89,31 @@ impl<'a> ParserContext<'a> {
         parsing_mode: ParsingMode,
         quirks_mode: QuirksMode
     ) -> ParserContext<'a> {
-        Self::new(Origin::Author, url_data, error_reporter, rule_type, parsing_mode, quirks_mode)
+        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,
-        rule_type: Option<CssRuleType>
+        rule_type: CssRuleType,
+        namespaces: &'a Namespaces,
     ) -> ParserContext<'a> {
         ParserContext {
             stylesheet_origin: context.stylesheet_origin,
             url_data: context.url_data,
             error_reporter: context.error_reporter,
-            rule_type: rule_type,
+            rule_type: Some(rule_type),
             line_number_offset: context.line_number_offset,
             parsing_mode: context.parsing_mode,
             quirks_mode: context.quirks_mode,
-            namespaces: context.namespaces,
+            namespaces: Some(namespaces),
         }
     }
 
@@ -115,7 +124,7 @@ impl<'a> ParserContext<'a> {
         error_reporter: &'a ParseErrorReporter,
         line_number_offset: u64,
         parsing_mode: ParsingMode,
-        quirks_mode: QuirksMode
+        quirks_mode: QuirksMode,
     ) -> ParserContext<'a> {
         ParserContext {
             stylesheet_origin: stylesheet_origin,

components/style/stylesheets/font_feature_values_rule.rs

@@ -322,14 +322,17 @@ macro_rules! font_feature_values_blocks {
                 }
             }
 
-            fn parse_block<'t>(&mut self, prelude: Self::Prelude, input: &mut Parser<'i, 't>)
+            fn parse_block<'t>(
-                               -> Result<Self::AtRule, ParseError<'i>> {
+                &mut self,
-                let context = ParserContext::new_with_rule_type(self.context, Some(CssRuleType::FontFeatureValues));
+                prelude: Self::Prelude,
+                input: &mut Parser<'i, 't>
+            ) -> Result<Self::AtRule, ParseError<'i>> {
+                debug_assert_eq!(self.context.rule_type(), CssRuleType::FontFeatureValues);
                 match prelude {
                     $(
                         BlockType::$ident_camel => {
                             let parser = FFVDeclarationsParser {
-                                context: &context,
+                                context: &self.context,
                                 declarations: &mut self.rule.$ident,
                             };
 
@@ -338,7 +341,7 @@ macro_rules! font_feature_values_blocks {
                                 if let Err(err) = declaration {
                                     let error = ContextualParseError::UnsupportedKeyframePropertyDeclaration(
                                         err.slice, err.error);
-                                    context.log_css_error(err.location, error);
+                                    self.context.log_css_error(err.location, error);
                                 }
                             }
                         },

components/style/stylesheets/keyframes_rule.rs

@@ -214,12 +214,16 @@ impl Keyframe {
     ) -> Result<Arc<Locked<Self>>, ParseError<'i>> {
         let url_data = parent_stylesheet_contents.url_data.read();
         let error_reporter = NullReporter;
-        let context = ParserContext::new(parent_stylesheet_contents.origin,
+        let namespaces = parent_stylesheet_contents.namespaces.read();
-                                         &url_data,
+        let mut context = ParserContext::new(
-                                         &error_reporter,
+            parent_stylesheet_contents.origin,
-                                         Some(CssRuleType::Keyframe),
+            &url_data,
-                                         PARSING_MODE_DEFAULT,
+            &error_reporter,
-                                         parent_stylesheet_contents.quirks_mode);
+            Some(CssRuleType::Keyframe),
+            PARSING_MODE_DEFAULT,
+            parent_stylesheet_contents.quirks_mode
+        );
+        context.namespaces = Some(&*namespaces);
         let mut input = ParserInput::new(css);
         let mut input = Parser::new(&mut input);
 
@@ -450,8 +454,14 @@ struct KeyframeListParser<'a> {
 }
 
 /// Parses a keyframe list from CSS input.
-pub fn parse_keyframe_list(context: &ParserContext, input: &mut Parser, shared_lock: &SharedRwLock)
+pub fn parse_keyframe_list(
-                           -> Vec<Arc<Locked<Keyframe>>> {
+    context: &ParserContext,
+    input: &mut Parser,
+    shared_lock: &SharedRwLock
+) -> Vec<Arc<Locked<Keyframe>>> {
+    debug_assert!(context.namespaces.is_some(),
+                  "Parsing a keyframe list from a context without namespaces?");
+
     let mut declarations = SourcePropertyDeclaration::new();
     RuleListParser::new_for_nested_rule(input, KeyframeListParser {
         context: context,
@@ -487,7 +497,13 @@ impl<'a, 'i> QualifiedRuleParser<'i> for KeyframeListParser<'a> {
 
     fn parse_block<'t>(&mut self, prelude: Self::Prelude, input: &mut Parser<'i, 't>)
                        -> Result<Self::QualifiedRule, ParseError<'i>> {
-        let context = ParserContext::new_with_rule_type(self.context, Some(CssRuleType::Keyframe));
+        let context =
+            ParserContext::new_with_rule_type(
+                self.context,
+                CssRuleType::Keyframe,
+                self.context.namespaces.unwrap(),
+            );
+
         let parser = KeyframeDeclarationParser {
             context: &context,
             declarations: self.declarations,

components/style/stylesheets/mod.rs

@@ -134,7 +134,7 @@ impl MallocSizeOfWithGuard for CssRule {
 }
 
 #[allow(missing_docs)]
-#[derive(PartialEq, Eq, Copy, Clone)]
+#[derive(PartialEq, Eq, Copy, Clone, Debug)]
 pub enum CssRuleType {
     // https://drafts.csswg.org/cssom/#the-cssrule-interface
     Style               = 1,
@@ -249,7 +249,7 @@ impl CssRule {
             loader: loader,
             state: state,
             had_hierarchy_error: false,
-            namespaces: Some(&mut *guard),
+            namespaces: &mut *guard,
         };
 
         parse_one_rule(&mut input, &mut rule_parser)

components/style/stylesheets/rule_parser.rs

@@ -42,8 +42,10 @@ pub struct TopLevelRuleParser<'a> {
     pub shared_lock: &'a SharedRwLock,
     /// A reference to a stylesheet loader if applicable, for `@import` rules.
     pub loader: Option<&'a StylesheetLoader>,
-    /// The parser context. This initially won't contain any namespaces, but
+    /// The top-level parser context.
-    /// will be populated after parsing namespace rules, if any.
+    ///
+    /// This won't contain any namespaces, and only nested parsers created with
+    /// `ParserContext::new_with_rule_type` will.
     pub context: ParserContext<'a>,
     /// The current state of the parser.
     pub state: State,
@@ -54,7 +56,7 @@ pub struct TopLevelRuleParser<'a> {
     /// The namespace map we use for parsing. Needs to start as `Some()`, and
     /// will be taken out after parsing namespace rules, and that reference will
     /// be moved to `ParserContext`.
-    pub namespaces: Option<&'a mut Namespaces>,
+    pub namespaces: &'a mut Namespaces,
 }
 
 impl<'b> TopLevelRuleParser<'b> {
@@ -63,14 +65,10 @@ impl<'b> TopLevelRuleParser<'b> {
             stylesheet_origin: self.stylesheet_origin,
             shared_lock: self.shared_lock,
             context: &self.context,
+            namespaces: &self.namespaces,
         }
     }
 
-    /// Returns the associated parser context with this rule parser.
-    pub fn context(&self) -> &ParserContext {
-        &self.context
-    }
-
     /// Returns the current state of the parser.
     pub fn state(&self) -> State {
         self.state
@@ -159,8 +157,10 @@ impl<'a, 'i> AtRuleParser<'i> for TopLevelRuleParser<'a> {
         name: CowRcStr<'i>,
         input: &mut Parser<'i, 't>
     ) -> Result<AtRuleType<AtRulePrelude, CssRule>, ParseError<'i>> {
-        let location = get_location_with_offset(input.current_source_location(),
+        let location = get_location_with_offset(
-                                                self.context.line_number_offset);
+            input.current_source_location(),
+            self.context.line_number_offset,
+        );
         match_ignore_ascii_case! { &*name,
             "import" => {
                 if self.state > State::Imports {
@@ -209,16 +209,14 @@ impl<'a, 'i> AtRuleParser<'i> for TopLevelRuleParser<'a> {
                 let id = register_namespace(&url)
                     .map_err(|()| StyleParseError::UnspecifiedError)?;
 
-                let namespaces = self.namespaces.as_mut().unwrap();
-
                 let opt_prefix = if let Ok(prefix) = prefix_result {
                     let prefix = Prefix::from(prefix.as_ref());
-                    namespaces
+                    self.namespaces
                         .prefixes
                         .insert(prefix.clone(), (url.clone(), id));
                     Some(prefix)
                 } else {
-                    namespaces.default = Some((url.clone(), id));
+                    self.namespaces.default = Some((url.clone(), id));
                     None
                 };
 
@@ -240,18 +238,15 @@ impl<'a, 'i> AtRuleParser<'i> for TopLevelRuleParser<'a> {
         }
         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)
     }
 
     #[inline]
-    fn parse_block<'t>(&mut self, prelude: AtRulePrelude, input: &mut Parser<'i, 't>)
+    fn parse_block<'t>(
-                       -> Result<CssRule, ParseError<'i>> {
+        &mut self,
+        prelude: AtRulePrelude,
+        input: &mut Parser<'i, 't>
+    ) -> Result<CssRule, ParseError<'i>> {
         AtRuleParser::parse_block(&mut self.nested(), prelude, input)
     }
 }
@@ -267,17 +262,11 @@ impl<'a, 'i> QualifiedRuleParser<'i> for TopLevelRuleParser<'a> {
     type Error = SelectorParseError<'i, StyleParseError<'i>>;
 
     #[inline]
-    fn parse_prelude<'t>(&mut self, input: &mut Parser<'i, 't>)
+    fn parse_prelude<'t>(
-                         -> Result<QualifiedRuleParserPrelude, ParseError<'i>> {
+        &mut self,
+        input: &mut Parser<'i, 't>,
+    ) -> Result<QualifiedRuleParserPrelude, ParseError<'i>> {
         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)
     }
 
@@ -296,6 +285,7 @@ struct NestedRuleParser<'a, 'b: 'a> {
     stylesheet_origin: Origin,
     shared_lock: &'a SharedRwLock,
     context: &'a ParserContext<'b>,
+    namespaces: &'a Namespaces,
 }
 
 impl<'a, 'b> NestedRuleParser<'a, 'b> {
@@ -304,12 +294,14 @@ impl<'a, 'b> NestedRuleParser<'a, 'b> {
         input: &mut Parser,
         rule_type: CssRuleType
     ) -> Arc<Locked<CssRules>> {
-        let context = ParserContext::new_with_rule_type(self.context, Some(rule_type));
+        let context =
+            ParserContext::new_with_rule_type(self.context, rule_type, self.namespaces);
 
         let nested_parser = NestedRuleParser {
             stylesheet_origin: self.stylesheet_origin,
             shared_lock: self.shared_lock,
             context: &context,
+            namespaces: self.namespaces,
         };
 
         let mut iter = RuleListParser::new_for_nested_rule(input, nested_parser);
@@ -428,17 +420,33 @@ impl<'a, 'b, 'i> AtRuleParser<'i> for NestedRuleParser<'a, 'b> {
     ) -> Result<CssRule, ParseError<'i>> {
         match prelude {
             AtRulePrelude::FontFace(location) => {
-                let context = ParserContext::new_with_rule_type(self.context, Some(CssRuleType::FontFace));
+                let context =
+                    ParserContext::new_with_rule_type(
+                        self.context,
+                        CssRuleType::FontFace,
+                        self.namespaces,
+                    );
+
                 Ok(CssRule::FontFace(Arc::new(self.shared_lock.wrap(
                    parse_font_face_block(&context, input, location).into()))))
             }
             AtRulePrelude::FontFeatureValues(family_names, location) => {
-                let context = ParserContext::new_with_rule_type(self.context, Some(CssRuleType::FontFeatureValues));
+                let context =
+                    ParserContext::new_with_rule_type(
+                        self.context,
+                        CssRuleType::FontFeatureValues,
+                        self.namespaces,
+                    );
                 Ok(CssRule::FontFeatureValues(Arc::new(self.shared_lock.wrap(
                     FontFeatureValuesRule::parse(&context, input, family_names, location)))))
             }
             AtRulePrelude::CounterStyle(name) => {
-                let context = ParserContext::new_with_rule_type(self.context, Some(CssRuleType::CounterStyle));
+                let context =
+                    ParserContext::new_with_rule_type(
+                        self.context,
+                        CssRuleType::CounterStyle,
+                        self.namespaces,
+                    );
                 Ok(CssRule::CounterStyle(Arc::new(self.shared_lock.wrap(
                    parse_counter_style_body(name, &context, input)?.into()))))
             }
@@ -450,7 +458,13 @@ impl<'a, 'b, 'i> AtRuleParser<'i> for NestedRuleParser<'a, 'b> {
                 }))))
             }
             AtRulePrelude::Supports(cond, location) => {
-                let enabled = cond.eval(self.context);
+                let eval_context =
+                    ParserContext::new_with_rule_type(
+                        self.context,
+                        CssRuleType::Style,
+                        self.namespaces,
+                    );
+                let enabled = cond.eval(&eval_context);
                 Ok(CssRule::Supports(Arc::new(self.shared_lock.wrap(SupportsRule {
                     condition: cond,
                     rules: self.parse_nested_rules(input, CssRuleType::Supports),
@@ -459,12 +473,23 @@ impl<'a, 'b, 'i> AtRuleParser<'i> for NestedRuleParser<'a, 'b> {
                 }))))
             }
             AtRulePrelude::Viewport => {
-                let context = ParserContext::new_with_rule_type(self.context, Some(CssRuleType::Viewport));
+                let context =
+                    ParserContext::new_with_rule_type(
+                        self.context,
+                        CssRuleType::Viewport,
+                        self.namespaces,
+                    );
                 Ok(CssRule::Viewport(Arc::new(self.shared_lock.wrap(
                    ViewportRule::parse(&context, input)?))))
             }
             AtRulePrelude::Keyframes(name, prefix, location) => {
-                let context = ParserContext::new_with_rule_type(self.context, Some(CssRuleType::Keyframes));
+                let context =
+                    ParserContext::new_with_rule_type(
+                        self.context,
+                        CssRuleType::Keyframes,
+                        self.namespaces,
+                    );
+
                 Ok(CssRule::Keyframes(Arc::new(self.shared_lock.wrap(KeyframesRule {
                     name: name,
                     keyframes: parse_keyframe_list(&context, input, self.shared_lock),
@@ -473,7 +498,12 @@ impl<'a, 'b, 'i> AtRuleParser<'i> for NestedRuleParser<'a, 'b> {
                 }))))
             }
             AtRulePrelude::Page(location) => {
-                let context = ParserContext::new_with_rule_type(self.context, Some(CssRuleType::Page));
+                let context =
+                    ParserContext::new_with_rule_type(
+                        self.context,
+                        CssRuleType::Page,
+                        self.namespaces,
+                    );
                 let declarations = parse_property_declaration_list(&context, input);
                 Ok(CssRule::Page(Arc::new(self.shared_lock.wrap(PageRule {
                     block: Arc::new(self.shared_lock.wrap(declarations)),
@@ -500,11 +530,13 @@ impl<'a, 'b, 'i> QualifiedRuleParser<'i> for NestedRuleParser<'a, 'b> {
     type QualifiedRule = CssRule;
     type Error = SelectorParseError<'i, StyleParseError<'i>>;
 
-    fn parse_prelude<'t>(&mut self, input: &mut Parser<'i, 't>)
+    fn parse_prelude<'t>(
-                         -> Result<QualifiedRuleParserPrelude, ParseError<'i>> {
+        &mut self,
+        input: &mut Parser<'i, 't>
+    ) -> Result<QualifiedRuleParserPrelude, ParseError<'i>> {
         let selector_parser = SelectorParser {
             stylesheet_origin: self.stylesheet_origin,
-            namespaces: self.context.namespaces.unwrap(),
+            namespaces: self.namespaces,
             url_data: Some(self.context.url_data),
         };
 
@@ -523,7 +555,12 @@ impl<'a, 'b, 'i> QualifiedRuleParser<'i> for NestedRuleParser<'a, 'b> {
         prelude: QualifiedRuleParserPrelude,
         input: &mut Parser<'i, 't>
     ) -> Result<CssRule, ParseError<'i>> {
-        let context = ParserContext::new_with_rule_type(self.context, Some(CssRuleType::Style));
+        let context =
+            ParserContext::new_with_rule_type(
+                self.context,
+                CssRuleType::Style,
+                self.namespaces,
+            );
         let declarations = parse_property_declaration_list(&context, input);
         Ok(CssRule::Style(Arc::new(self.shared_lock.wrap(StyleRule {
             selectors: prelude.selectors,

components/style/stylesheets/stylesheet.rs

@@ -343,7 +343,7 @@ impl Stylesheet {
             context: context,
             state: State::Start,
             had_hierarchy_error: false,
-            namespaces: Some(namespaces),
+            namespaces: namespaces,
         };
 
         input.look_for_viewport_percentages();
@@ -357,7 +357,7 @@ impl Stylesheet {
                     Ok(rule) => rules.push(rule),
                     Err(err) => {
                         let error = ContextualParseError::InvalidRule(err.slice, err.error);
-                        iter.parser.context().log_css_error(err.location, error);
+                        iter.parser.context.log_css_error(err.location, error);
                     }
                 }
             }

components/style/stylesheets/supports_rule.rs

@@ -241,16 +241,19 @@ impl Declaration {
     /// Determine if a declaration parses
     ///
     /// https://drafts.csswg.org/css-conditional-3/#support-definition
-    pub fn eval(&self, cx: &ParserContext) -> bool {
+    pub fn eval(&self, context: &ParserContext) -> bool {
+        debug_assert_eq!(context.rule_type(), CssRuleType::Style);
+
         let mut input = ParserInput::new(&self.0);
         let mut input = Parser::new(&mut input);
         input.parse_entirely(|input| {
             let prop = input.expect_ident().unwrap().as_ref().to_owned();
             input.expect_colon().unwrap();
-            let context = ParserContext::new_with_rule_type(cx, Some(CssRuleType::Style));
+
             let property_context = PropertyParserContext::new(&context);
             let id = PropertyId::parse(&prop, Some(&property_context))
-                    .map_err(|_| StyleParseError::UnspecifiedError)?;
+                        .map_err(|_| StyleParseError::UnspecifiedError)?;
+
             let mut declarations = SourcePropertyDeclaration::new();
             input.parse_until_before(Delimiter::Bang, |input| {
                 PropertyDeclaration::parse_into(&mut declarations, id, &context, input)

ports/geckolib/glue.rs

@@ -1508,6 +1508,7 @@ pub extern "C" fn Servo_KeyframesRule_AppendRule(
     let css = unsafe { css.as_ref().unwrap().as_str_unchecked() };
     let contents = StylesheetContents::as_arc(&contents);
     let global_style_data = &*GLOBAL_STYLE_DATA;
+
     match Keyframe::parse(css, &contents, &global_style_data.shared_lock) {
         Ok(keyframe) => {
             write_locked_arc(rule, |rule: &mut KeyframesRule| {
@@ -2811,9 +2812,16 @@ pub extern "C" fn Servo_CSSSupports(cond: *const nsACString) -> bool {
     if let Ok(cond) = cond {
         let url_data = unsafe { dummy_url_data() };
         let reporter = NullReporter;
-        let context = ParserContext::new_for_cssom(url_data, &reporter, Some(CssRuleType::Style),
+        // NOTE(emilio): The supports API is not associated to any stylesheet,
-                                                   PARSING_MODE_DEFAULT,
+        // so the fact that there are no namespace map here is fine.
-                                                   QuirksMode::NoQuirks);
+        let context =
+            ParserContext::new_for_cssom(
+                url_data,
+                &reporter,
+                Some(CssRuleType::Style),
+                PARSING_MODE_DEFAULT,
+                QuirksMode::NoQuirks,
+            );
         cond.eval(&context)
     } else {
         false