Bug 1551991 - Cleanup a bit the counter style code. r=jwatt
☠☠ backed out by 283b94c196a1 ☠ ☠
authorEmilio Cobos Álvarez <emilio@crisal.io>
Thu, 16 May 2019 14:30:57 +0000
changeset 532922 8a7a0329bdc3fe43cdcb40fb9f9749b34c1c9130
parent 532921 86159475ddd3a0ef6e54492be2dc563e71310d2c
child 532923 6c1f00cc30ca74cf5db1463da06b0c2772e9737a
push id11276
push userrgurzau@mozilla.com
push dateMon, 20 May 2019 13:11:24 +0000
treeherdermozilla-beta@847755a7c325 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjwatt
bugs1551991
milestone68.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1551991 - Cleanup a bit the counter style code. r=jwatt Use more compact types, and remove some manual implementations that can be derived. Differential Revision: https://phabricator.services.mozilla.com/D31315
servo/components/style/counter_style/mod.rs
servo/components/style/gecko/values.rs
--- a/servo/components/style/counter_style/mod.rs
+++ b/servo/components/style/counter_style/mod.rs
@@ -14,17 +14,16 @@ use crate::values::specified::Integer;
 use crate::values::CustomIdent;
 use crate::Atom;
 use cssparser::{AtRuleParser, DeclarationListParser, DeclarationParser};
 use cssparser::{CowRcStr, Parser, SourceLocation, Token};
 use selectors::parser::SelectorParseErrorKind;
 use std::fmt::{self, Write};
 use std::mem;
 use std::num::Wrapping;
-use std::ops::Range;
 use style_traits::{Comma, CssWriter, OneOrMoreSeparated, ParseError};
 use style_traits::{StyleParseErrorKind, ToCss};
 
 /// Parse a counter style name reference.
 ///
 /// This allows the reserved counter style names "decimal" and "disc".
 pub fn parse_counter_style_name<'i, 't>(
     input: &mut Parser<'i, 't>,
@@ -256,17 +255,17 @@ counter_style_descriptors! {
 
     /// <https://drafts.csswg.org/css-counter-styles/#counter-style-prefix>
     "prefix" prefix / set_prefix [_]: Symbol,
 
     /// <https://drafts.csswg.org/css-counter-styles/#counter-style-suffix>
     "suffix" suffix / set_suffix [_]: Symbol,
 
     /// <https://drafts.csswg.org/css-counter-styles/#counter-style-range>
-    "range" range / set_range [_]: Ranges,
+    "range" range / set_range [_]: CounterRanges,
 
     /// <https://drafts.csswg.org/css-counter-styles/#counter-style-pad>
     "pad" pad / set_pad [_]: Pad,
 
     /// <https://drafts.csswg.org/css-counter-styles/#counter-style-fallback>
     "fallback" fallback / set_fallback [_]: Fallback,
 
     /// <https://drafts.csswg.org/css-counter-styles/#descdef-counter-style-symbols>
@@ -366,17 +365,17 @@ impl Parse for System {
         try_match_ident_ignore_ascii_case! { input,
             "cyclic" => Ok(System::Cyclic),
             "numeric" => Ok(System::Numeric),
             "alphabetic" => Ok(System::Alphabetic),
             "symbolic" => Ok(System::Symbolic),
             "additive" => Ok(System::Additive),
             "fixed" => {
                 let first_symbol_value = input.try(|i| Integer::parse(context, i)).ok();
-                Ok(System::Fixed { first_symbol_value: first_symbol_value })
+                Ok(System::Fixed { first_symbol_value })
             }
             "extends" => {
                 let other = parse_counter_style_name(input)?;
                 Ok(System::Extends(other))
             }
         }
     }
 }
@@ -404,36 +403,35 @@ impl ToCss for System {
                 dest.write_str("extends ")?;
                 other.to_css(dest)
             },
         }
     }
 }
 
 /// <https://drafts.csswg.org/css-counter-styles/#typedef-symbol>
-#[cfg_attr(feature = "gecko", derive(MallocSizeOf))]
-#[derive(Clone, Debug, Eq, PartialEq, ToComputedValue, ToCss, ToShmem)]
+#[derive(Clone, Debug, Eq, PartialEq, ToComputedValue, ToCss, ToShmem, MallocSizeOf)]
 pub enum Symbol {
     /// <string>
-    String(String),
+    String(crate::OwnedStr),
     /// <custom-ident>
     Ident(CustomIdent),
     // Not implemented:
     // /// <image>
     // Image(Image),
 }
 
 impl Parse for Symbol {
     fn parse<'i, 't>(
         _context: &ParserContext,
         input: &mut Parser<'i, 't>,
     ) -> Result<Self, ParseError<'i>> {
         let location = input.current_source_location();
         match *input.next()? {
-            Token::QuotedString(ref s) => Ok(Symbol::String(s.as_ref().to_owned())),
+            Token::QuotedString(ref s) => Ok(Symbol::String(s.as_ref().to_owned().into())),
             Token::Ident(ref s) => Ok(Symbol::Ident(CustomIdent::from_ident(location, s, &[])?)),
             ref t => Err(location.new_unexpected_token_error(t.clone())),
         }
     }
 }
 
 impl Symbol {
     /// Returns whether this symbol is allowed in symbols() function.
@@ -458,100 +456,85 @@ impl Parse for Negative {
         Ok(Negative(
             Symbol::parse(context, input)?,
             input.try(|input| Symbol::parse(context, input)).ok(),
         ))
     }
 }
 
 /// <https://drafts.csswg.org/css-counter-styles/#counter-style-range>
+#[derive(Clone, Debug, ToShmem, ToCss)]
+pub struct CounterRange {
+    /// The start of the range.
+    pub start: CounterBound,
+    /// The end of the range.
+    pub end: CounterBound,
+}
+
+/// <https://drafts.csswg.org/css-counter-styles/#counter-style-range>
 ///
-/// Empty Vec represents 'auto'
-#[derive(Clone, Debug, ToShmem)]
-pub struct Ranges(pub Vec<Range<CounterBound>>);
+/// Empty represents 'auto'
+#[derive(Clone, Debug, ToShmem, ToCss)]
+#[css(comma)]
+pub struct CounterRanges(
+    #[css(iterable, if_empty = "auto")]
+    pub crate::OwnedSlice<CounterRange>,
+);
 
-/// A bound found in `Ranges`.
+/// A bound found in `CounterRanges`.
 #[derive(Clone, Copy, Debug, ToCss, ToShmem)]
 pub enum CounterBound {
     /// An integer bound.
     Integer(Integer),
     /// The infinite bound.
     Infinite,
 }
 
-impl Parse for Ranges {
+impl Parse for CounterRanges {
     fn parse<'i, 't>(
         context: &ParserContext,
         input: &mut Parser<'i, 't>,
     ) -> Result<Self, ParseError<'i>> {
         if input
             .try(|input| input.expect_ident_matching("auto"))
             .is_ok()
         {
-            Ok(Ranges(Vec::new()))
-        } else {
-            input
-                .parse_comma_separated(|input| {
-                    let opt_start = parse_bound(context, input)?;
-                    let opt_end = parse_bound(context, input)?;
-                    if let (CounterBound::Integer(start), CounterBound::Integer(end)) =
-                        (opt_start, opt_end)
-                    {
-                        if start > end {
-                            return Err(
-                                input.new_custom_error(StyleParseErrorKind::UnspecifiedError)
-                            );
-                        }
-                    }
-                    Ok(opt_start..opt_end)
-                })
-                .map(Ranges)
+            return Ok(CounterRanges(Default::default()));
         }
+
+        let ranges = input.parse_comma_separated(|input| {
+            let start = parse_bound(context, input)?;
+            let end = parse_bound(context, input)?;
+            if let (CounterBound::Integer(start), CounterBound::Integer(end)) =
+                (start, end)
+            {
+                if start > end {
+                    return Err(
+                        input.new_custom_error(StyleParseErrorKind::UnspecifiedError)
+                    );
+                }
+            }
+            Ok(CounterRange { start, end })
+        })?;
+
+        Ok(CounterRanges(ranges.into()))
     }
 }
 
 fn parse_bound<'i, 't>(
     context: &ParserContext,
     input: &mut Parser<'i, 't>,
 ) -> Result<CounterBound, ParseError<'i>> {
     if let Ok(integer) = input.try(|input| Integer::parse(context, input)) {
         return Ok(CounterBound::Integer(integer));
     }
     input.expect_ident_matching("infinite")?;
     Ok(CounterBound::Infinite)
 }
 
-impl ToCss for Ranges {
-    fn to_css<W>(&self, dest: &mut CssWriter<W>) -> fmt::Result
-    where
-        W: Write,
-    {
-        let mut iter = self.0.iter();
-        if let Some(first) = iter.next() {
-            range_to_css(first, dest)?;
-            for item in iter {
-                dest.write_str(", ")?;
-                range_to_css(item, dest)?;
-            }
-            Ok(())
-        } else {
-            dest.write_str("auto")
-        }
-    }
-}
-
-fn range_to_css<W>(range: &Range<CounterBound>, dest: &mut CssWriter<W>) -> fmt::Result
-where
-    W: Write,
-{
-    range.start.to_css(dest)?;
-    dest.write_char(' ')?;
-    range.end.to_css(dest)
-}
-
 /// <https://drafts.csswg.org/css-counter-styles/#counter-style-pad>
 #[derive(Clone, Debug, ToCss, ToShmem)]
 pub struct Pad(pub Integer, pub Symbol);
 
 impl Parse for Pad {
     fn parse<'i, 't>(
         context: &ParserContext,
         input: &mut Parser<'i, 't>,
@@ -567,63 +550,59 @@ impl Parse for Pad {
 #[derive(Clone, Debug, ToCss, ToShmem)]
 pub struct Fallback(pub CustomIdent);
 
 impl Parse for Fallback {
     fn parse<'i, 't>(
         _context: &ParserContext,
         input: &mut Parser<'i, 't>,
     ) -> Result<Self, ParseError<'i>> {
-        parse_counter_style_name(input).map(Fallback)
+        Ok(Fallback(parse_counter_style_name(input)?))
     }
 }
 
 /// <https://drafts.csswg.org/css-counter-styles/#descdef-counter-style-symbols>
-#[cfg_attr(feature = "gecko", derive(MallocSizeOf))]
-#[derive(Clone, Debug, Eq, PartialEq, ToComputedValue, ToCss, ToShmem)]
-pub struct Symbols(#[css(iterable)] pub Vec<Symbol>);
+#[derive(Clone, Debug, Eq, PartialEq, MallocSizeOf, ToComputedValue, ToCss, ToShmem)]
+pub struct Symbols(#[css(iterable)] pub crate::OwnedSlice<Symbol>);
 
 impl Parse for Symbols {
     fn parse<'i, 't>(
         context: &ParserContext,
         input: &mut Parser<'i, 't>,
     ) -> Result<Self, ParseError<'i>> {
         let mut symbols = Vec::new();
-        loop {
-            if let Ok(s) = input.try(|input| Symbol::parse(context, input)) {
-                symbols.push(s)
-            } else {
-                if symbols.is_empty() {
-                    return Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError));
-                } else {
-                    return Ok(Symbols(symbols));
-                }
-            }
+        while let Ok(s) = input.try(|input| Symbol::parse(context, input)) {
+            symbols.push(s);
         }
+        if symbols.is_empty() {
+            return Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError));
+        }
+        Ok(Symbols(symbols.into()))
     }
 }
 
 /// <https://drafts.csswg.org/css-counter-styles/#descdef-counter-style-additive-symbols>
 #[derive(Clone, Debug, ToCss, ToShmem)]
-pub struct AdditiveSymbols(pub Vec<AdditiveTuple>);
+#[css(comma)]
+pub struct AdditiveSymbols(#[css(iterable)] pub crate::OwnedSlice<AdditiveTuple>);
 
 impl Parse for AdditiveSymbols {
     fn parse<'i, 't>(
         context: &ParserContext,
         input: &mut Parser<'i, 't>,
     ) -> Result<Self, ParseError<'i>> {
         let tuples = Vec::<AdditiveTuple>::parse(context, input)?;
         // FIXME maybe? https://github.com/w3c/csswg-drafts/issues/1220
         if tuples
             .windows(2)
             .any(|window| window[0].weight <= window[1].weight)
         {
             return Err(input.new_custom_error(StyleParseErrorKind::UnspecifiedError));
         }
-        Ok(AdditiveSymbols(tuples))
+        Ok(AdditiveSymbols(tuples.into()))
     }
 }
 
 /// <integer> && <symbol>
 #[derive(Clone, Debug, ToCss, ToShmem)]
 pub struct AdditiveTuple {
     /// <integer>
     pub weight: Integer,
@@ -638,20 +617,17 @@ impl OneOrMoreSeparated for AdditiveTupl
 impl Parse for AdditiveTuple {
     fn parse<'i, 't>(
         context: &ParserContext,
         input: &mut Parser<'i, 't>,
     ) -> Result<Self, ParseError<'i>> {
         let symbol = input.try(|input| Symbol::parse(context, input));
         let weight = Integer::parse_non_negative(context, input)?;
         let symbol = symbol.or_else(|_| Symbol::parse(context, input))?;
-        Ok(AdditiveTuple {
-            weight: weight,
-            symbol: symbol,
-        })
+        Ok(Self { weight, symbol })
     }
 }
 
 /// <https://drafts.csswg.org/css-counter-styles/#counter-style-speak-as>
 #[derive(Clone, Debug, ToCss, ToShmem)]
 pub enum SpeakAs {
     /// auto
     Auto,
--- a/servo/components/style/gecko/values.rs
+++ b/servo/components/style/gecko/values.rs
@@ -283,17 +283,17 @@ impl CounterStyleOrNone {
             CounterStyleOrNone::Name(name) => unsafe {
                 set_name(gecko_value, name.0.into_addrefed());
             },
             CounterStyleOrNone::Symbols(symbols_type, symbols) => {
                 let symbols: Vec<_> = symbols
                     .0
                     .iter()
                     .map(|symbol| match *symbol {
-                        Symbol::String(ref s) => nsCStr::from(s),
+                        Symbol::String(ref s) => nsCStr::from(&**s),
                         Symbol::Ident(_) => unreachable!("Should not have identifier in symbols()"),
                     })
                     .collect();
                 let symbols: Vec<_> = symbols
                     .iter()
                     .map(|symbol| symbol as &nsACString as *const _)
                     .collect();
                 unsafe {
@@ -328,15 +328,15 @@ impl CounterStyleOrNone {
             let symbols = &anonymous.mSymbols;
             if anonymous.mSingleString {
                 debug_assert_eq!(symbols.len(), 1);
                 Either::Second(symbols[0].to_string())
             } else {
                 let symbol_type = SymbolsType::from_gecko_keyword(anonymous.mSystem as u32);
                 let symbols = symbols
                     .iter()
-                    .map(|gecko_symbol| Symbol::String(gecko_symbol.to_string()))
+                    .map(|gecko_symbol| Symbol::String(gecko_symbol.to_string().into()))
                     .collect();
                 Either::First(CounterStyleOrNone::Symbols(symbol_type, Symbols(symbols)))
             }
         }
     }
 }