Make PropertyDeclaration::parse return an enum rather than push to a Vec.

6 files changed, 83 insertions, 113 deletions

diff --git a/components/script/dom/cssstyledeclaration.rs b/components/script/dom/cssstyledeclaration.rs
index f658ed5375e..450d0b7da4b 100644
--- a/components/script/dom/cssstyledeclaration.rs
+++ b/components/script/dom/cssstyledeclaration.rs
@@ -250,14 +250,14 @@ impl CSSStyleDeclaration {
 
             // Step 6
             let window = self.owner.window();
-            let declarations =
+            let result =
                 parse_one_declaration(id, &value, &self.owner.base_url(),
                                       window.css_error_reporter(),
                                       ParserContextExtraData::default());
 
             // Step 7
-            let declarations = match declarations {
-                Ok(declarations) => declarations,
+            let parsed = match result {
+                Ok(parsed) => parsed,
                 Err(_) => {
                     *changed = false;
                     return Ok(());
@@ -267,9 +267,9 @@ impl CSSStyleDeclaration {
             // Step 8
             // Step 9
             *changed = false;
-            for declaration in declarations {
-                *changed |= pdb.set_parsed_declaration(declaration.0, importance);
-            }
+            parsed.expand(|declaration| {
+                *changed |= pdb.set_parsed_declaration(declaration, importance);
+            });
 
             Ok(())
         })
diff --git a/components/style/keyframes.rs b/components/style/keyframes.rs
index 048d9904043..e48e8c4e909 100644
--- a/components/style/keyframes.rs
+++ b/components/style/keyframes.rs
@@ -11,7 +11,7 @@ use cssparser::{DeclarationListParser, DeclarationParser, parse_one_rule};
 use parking_lot::RwLock;
 use parser::{ParserContext, ParserContextExtraData, log_css_error};
 use properties::{Importance, PropertyDeclaration, PropertyDeclarationBlock, PropertyId};
-use properties::{PropertyDeclarationId, LonghandId, DeclaredValue};
+use properties::{PropertyDeclarationId, LonghandId, DeclaredValue, ParsedDeclaration};
 use properties::LonghandIdSet;
 use properties::animated_properties::TransitionProperty;
 use properties::longhands::transition_timing_function::single_value::SpecifiedValue as SpecifiedTimingFunction;
@@ -360,12 +360,12 @@ impl<'a> QualifiedRuleParser for KeyframeListParser<'a> {
                    -> Result<Self::QualifiedRule, ()> {
         let parser = KeyframeDeclarationParser {
             context: self.context,
-            declarations: vec![],
         };
         let mut iter = DeclarationListParser::new(input, parser);
+        let mut declarations = Vec::new();
         while let Some(declaration) = iter.next() {
             match declaration {
-                Ok(_) => (),
+                Ok(parsed) => parsed.expand(|d| declarations.push((d, Importance::Normal))),
                 Err(range) => {
                     let pos = range.start;
                     let message = format!("Unsupported keyframe property declaration: '{}'",
@@ -378,7 +378,7 @@ impl<'a> QualifiedRuleParser for KeyframeListParser<'a> {
         Ok(Arc::new(RwLock::new(Keyframe {
             selector: prelude,
             block: Arc::new(RwLock::new(PropertyDeclarationBlock {
-                declarations: iter.parser.declarations,
+                declarations: declarations,
                 important_count: 0,
             })),
         })))
@@ -387,32 +387,30 @@ impl<'a> QualifiedRuleParser for KeyframeListParser<'a> {
 
 struct KeyframeDeclarationParser<'a, 'b: 'a> {
     context: &'a ParserContext<'b>,
-    declarations: Vec<(PropertyDeclaration, Importance)>
 }
 
 /// Default methods reject all at rules.
 impl<'a, 'b> AtRuleParser for KeyframeDeclarationParser<'a, 'b> {
     type Prelude = ();
-    type AtRule = ();
+    type AtRule = ParsedDeclaration;
 }
 
 impl<'a, 'b> DeclarationParser for KeyframeDeclarationParser<'a, 'b> {
     /// We parse rules directly into the declarations object
-    type Declaration = ();
+    type Declaration = ParsedDeclaration;
 
-    fn parse_value(&mut self, name: &str, input: &mut Parser) -> Result<(), ()> {
+    fn parse_value(&mut self, name: &str, input: &mut Parser) -> Result<ParsedDeclaration, ()> {
         let id = try!(PropertyId::parse(name.into()));
-        let old_len = self.declarations.len();
-        if PropertyDeclaration::parse(id, self.context, input, &mut self.declarations, true).is_err() {
-            self.declarations.truncate(old_len);
-            return Err(())
-        }
-        // In case there is still unparsed text in the declaration, we should roll back.
-        if !input.is_exhausted() {
-            self.declarations.truncate(old_len);
-            Err(())
-        } else {
-            Ok(())
+        match PropertyDeclaration::parse(id, self.context, input, true) {
+            Ok(parsed) => {
+                // In case there is still unparsed text in the declaration, we should roll back.
+                if !input.is_exhausted() {
+                    Err(())
+                } else {
+                    Ok(parsed)
+                }
+            }
+            Err(_) => Err(())
         }
     }
 }
diff --git a/components/style/properties/declaration_block.rs b/components/style/properties/declaration_block.rs
index 243bc7cbc5c..cb63c97f50c 100644
--- a/components/style/properties/declaration_block.rs
+++ b/components/style/properties/declaration_block.rs
@@ -56,7 +56,7 @@ pub struct PropertyDeclarationBlock {
     pub declarations: Vec<(PropertyDeclaration, Importance)>,
 
     /// The number of entries in `self.declaration` with `Importance::Important`
-    pub important_count: u32,
+    pub important_count: usize,
 }
 
 impl PropertyDeclarationBlock {
@@ -75,7 +75,7 @@ impl PropertyDeclarationBlock {
     /// which should be maintained whenever `declarations` is changed.
     // FIXME: make fields private and maintain it here in methods?
     pub fn any_normal(&self) -> bool {
-        self.declarations.len() > self.important_count as usize
+        self.declarations.len() > self.important_count
     }
 
     /// Get a declaration for a given property.
@@ -554,63 +554,46 @@ pub fn parse_one_declaration(id: PropertyId,
                              base_url: &ServoUrl,
                              error_reporter: StdBox<ParseErrorReporter + Send>,
                              extra_data: ParserContextExtraData)
-                             -> Result<Vec<(PropertyDeclaration, Importance)>, ()> {
+                             -> Result<ParsedDeclaration, ()> {
     let context = ParserContext::new_with_extra_data(Origin::Author, base_url, error_reporter, extra_data);
     Parser::new(input).parse_entirely(|parser| {
-        let mut results = vec![];
-        match PropertyDeclaration::parse(id, &context, parser, &mut results, false) {
-            Ok(()) => Ok(results),
-            Err(_) => Err(())
-        }
+        PropertyDeclaration::parse(id, &context, parser, false)
+            .map_err(|_| ())
     })
 }
 
 /// A struct to parse property declarations.
 struct PropertyDeclarationParser<'a, 'b: 'a> {
     context: &'a ParserContext<'b>,
-    declarations: Vec<(PropertyDeclaration, Importance)>
 }
 
 
 /// Default methods reject all at rules.
 impl<'a, 'b> AtRuleParser for PropertyDeclarationParser<'a, 'b> {
     type Prelude = ();
-    type AtRule = (u32, Importance);
+    type AtRule = (ParsedDeclaration, Importance);
 }
 
 
 impl<'a, 'b> DeclarationParser for PropertyDeclarationParser<'a, 'b> {
-    /// Declarations are pushed to the internal vector. This will only
-    /// let you know if the decl was !important and how many longhands
-    /// it expanded to
-    type Declaration = (u32, Importance);
+    type Declaration = (ParsedDeclaration, Importance);
 
     fn parse_value(&mut self, name: &str, input: &mut Parser)
-                   -> Result<(u32, Importance), ()> {
+                   -> Result<(ParsedDeclaration, Importance), ()> {
         let id = try!(PropertyId::parse(name.into()));
-        let old_len = self.declarations.len();
-        let parse_result = input.parse_until_before(Delimiter::Bang, |input| {
-            PropertyDeclaration::parse(id, self.context, input, &mut self.declarations, false)
-            .map_err(|_| ())
-        });
-        if let Err(_) = parse_result {
-            // rollback
-            self.declarations.truncate(old_len);
-            return Err(())
-        }
+        let parsed = input.parse_until_before(Delimiter::Bang, |input| {
+            PropertyDeclaration::parse(id, self.context, input, false)
+                .map_err(|_| ())
+        })?;
         let importance = match input.try(parse_important) {
             Ok(()) => Importance::Important,
             Err(()) => Importance::Normal,
         };
         // In case there is still unparsed text in the declaration, we should roll back.
         if !input.is_exhausted() {
-            self.declarations.truncate(old_len);
             return Err(())
         }
-        for decl in &mut self.declarations[old_len..] {
-            decl.1 = importance
-        }
-        Ok(((self.declarations.len() - old_len) as u32, importance))
+        Ok((parsed, importance))
     }
 }
 
@@ -620,17 +603,19 @@ impl<'a, 'b> DeclarationParser for PropertyDeclarationParser<'a, 'b> {
 pub fn parse_property_declaration_list(context: &ParserContext,
                                        input: &mut Parser)
                                        -> PropertyDeclarationBlock {
+    let mut declarations = Vec::new();
     let mut important_count = 0;
     let parser = PropertyDeclarationParser {
         context: context,
-        declarations: vec![],
     };
     let mut iter = DeclarationListParser::new(input, parser);
     while let Some(declaration) = iter.next() {
         match declaration {
-            Ok((count, importance)) => {
+            Ok((parsed, importance)) => {
+                let old_len = declarations.len();
+                parsed.expand(|d| declarations.push((d, importance)));
                 if importance.important() {
-                    important_count += count;
+                    important_count += declarations.len() - old_len;
                 }
             }
             Err(range) => {
@@ -642,7 +627,7 @@ pub fn parse_property_declaration_list(context: &ParserContext,
         }
     }
     let mut block = PropertyDeclarationBlock {
-        declarations: iter.parser.declarations,
+        declarations: declarations,
         important_count: important_count,
     };
     block.deduplicate();
diff --git a/components/style/properties/properties.mako.rs b/components/style/properties/properties.mako.rs
index 9b2c94798d5..8730680db9b 100644
--- a/components/style/properties/properties.mako.rs
+++ b/components/style/properties/properties.mako.rs
@@ -800,11 +800,14 @@ pub enum ParsedDeclaration {
     % for shorthand in data.shorthands:
         /// ${shorthand.name}
         ${shorthand.camel_case}(shorthands::${shorthand.ident}::Longhands),
-    % endfor
-    % for shorthand in data.shorthands:
+
+        /// ${shorthand.name} with a CSS-wide keyword
+        ${shorthand.camel_case}CSSWideKeyword(CSSWideKeyword),
+
         /// ${shorthand.name} with var() functions
         ${shorthand.camel_case}WithVariables(Arc<UnparsedValue>),
     % endfor
+
     /// Not a shorthand
     LonghandOrCustom(PropertyDeclaration),
 }
@@ -832,8 +835,13 @@ impl ParsedDeclaration {
                         ));
                     % endfor
                 }
-            % endfor
-            % for shorthand in data.shorthands:
+                ParsedDeclaration::${shorthand.camel_case}CSSWideKeyword(keyword) => {
+                    % for sub_property in shorthand.sub_properties:
+                        f(PropertyDeclaration::${sub_property.camel_case}(
+                            DeclaredValue::CSSWideKeyword(keyword)
+                        ));
+                    % endfor
+                }
                 ParsedDeclaration::${shorthand.camel_case}WithVariables(value) => {
                     debug_assert_eq!(
                         value.from_shorthand,
@@ -841,7 +849,8 @@ impl ParsedDeclaration {
                     );
                     % for sub_property in shorthand.sub_properties:
                         f(PropertyDeclaration::${sub_property.camel_case}(
-                            DeclaredValue::WithVariables(value.clone())));
+                            DeclaredValue::WithVariables(value.clone())
+                        ));
                     % endfor
                 }
             % endfor
@@ -1053,9 +1062,8 @@ impl PropertyDeclaration {
     /// to Importance::Normal. Parsing Importance values is the job of PropertyDeclarationParser,
     /// we only set them here so that we don't have to reallocate
     pub fn parse(id: PropertyId, context: &ParserContext, input: &mut Parser,
-                 result_list: &mut Vec<(PropertyDeclaration, Importance)>,
                  in_keyframe_block: bool)
-                 -> Result<(), PropertyDeclarationParseError> {
+                 -> Result<ParsedDeclaration, PropertyDeclarationParseError> {
         match id {
             PropertyId::Custom(name) => {
                 let value = match input.try(|i| CSSWideKeyword::parse(context, i)) {
@@ -1065,9 +1073,7 @@ impl PropertyDeclaration {
                         Err(()) => return Err(PropertyDeclarationParseError::InvalidValue),
                     }
                 };
-                result_list.push((PropertyDeclaration::Custom(name, value),
-                                  Importance::Normal));
-                return Ok(());
+                Ok(ParsedDeclaration::LonghandOrCustom(PropertyDeclaration::Custom(name, value)))
             }
             PropertyId::Longhand(id) => match id {
             % for property in data.longhands:
@@ -1088,9 +1094,9 @@ impl PropertyDeclaration {
 
                         match longhands::${property.ident}::parse_declared(context, input) {
                             Ok(value) => {
-                                result_list.push((PropertyDeclaration::${property.camel_case}(value),
-                                                  Importance::Normal));
-                                Ok(())
+                                Ok(ParsedDeclaration::LonghandOrCustom(
+                                    PropertyDeclaration::${property.camel_case}(value)
+                                ))
                             },
                             Err(()) => Err(PropertyDeclarationParseError::InvalidValue),
                         }
@@ -1118,21 +1124,11 @@ impl PropertyDeclaration {
 
                     match input.try(|i| CSSWideKeyword::parse(context, i)) {
                         Ok(keyword) => {
-                            % for sub_property in shorthand.sub_properties:
-                                result_list.push((
-                                    PropertyDeclaration::${sub_property.camel_case}(
-                                        DeclaredValue::CSSWideKeyword(keyword)), Importance::Normal));
-                            % endfor
-                            Ok(())
+                            Ok(ParsedDeclaration::${shorthand.camel_case}CSSWideKeyword(keyword))
                         },
-                        Err(()) => match shorthands::${shorthand.ident}::parse(context, input) {
-                            Ok(parsed) => {
-                                parsed.expand(|declaration| {
-                                    result_list.push((declaration, Importance::Normal))
-                                });
-                                Ok(())
-                            }
-                            Err(()) => Err(PropertyDeclarationParseError::InvalidValue),
+                        Err(()) => {
+                            shorthands::${shorthand.ident}::parse(context, input)
+                                .map_err(|()| PropertyDeclarationParseError::InvalidValue)
                         }
                     }
                 }
diff --git a/components/style/supports.rs b/components/style/supports.rs
index 170d874801e..ccdde3231bb 100644
--- a/components/style/supports.rs
+++ b/components/style/supports.rs
@@ -211,13 +211,8 @@ impl Declaration {
             return false
         };
         let mut input = Parser::new(&self.val);
-        let mut list = Vec::new();
-        let res = PropertyDeclaration::parse(id, cx, &mut input,
-                                             &mut list, /* in_keyframe */ false);
+        let res = PropertyDeclaration::parse(id, cx, &mut input, /* in_keyframe */ false);
         let _ = input.try(parse_important);
-        if !input.is_exhausted() {
-            return false;
-        }
-        res.is_ok()
+        res.is_ok() && input.is_exhausted()
     }
 }
diff --git a/ports/geckolib/glue.rs b/ports/geckolib/glue.rs
index 3047b727293..c8b5d7013b6 100644
--- a/ports/geckolib/glue.rs
+++ b/ports/geckolib/glue.rs
@@ -713,18 +713,17 @@ pub extern "C" fn Servo_ParseProperty(property: *const nsACString, value: *const
                                                      Box::new(StdoutErrorReporter),
                                                      extra_data);
 
-    let mut results = vec![];
-    let result = PropertyDeclaration::parse(
-        id, &context, &mut Parser::new(value), &mut results, false
-    );
-    if result.is_err() {
-        return RawServoDeclarationBlockStrong::null()
+    match PropertyDeclaration::parse(id, &context, &mut Parser::new(value), false) {
+        Ok(parsed) => {
+            let mut declarations = Vec::new();
+            parsed.expand(|d| declarations.push((d, Importance::Normal)));
+            Arc::new(RwLock::new(PropertyDeclarationBlock {
+                declarations: declarations,
+                important_count: 0,
+            })).into_strong()
+        }
+        Err(_) => RawServoDeclarationBlockStrong::null()
     }
-
-    Arc::new(RwLock::new(PropertyDeclarationBlock {
-        declarations: results,
-        important_count: 0,
-    })).into_strong()
 }
 
 #[no_mangle]
@@ -834,14 +833,14 @@ fn set_property(declarations: RawServoDeclarationBlockBorrowed, property_id: Pro
     // FIXME Needs real URL and ParserContextExtraData.
     let base_url = &*DUMMY_BASE_URL;
     let extra_data = ParserContextExtraData::default();
-    if let Ok(decls) = parse_one_declaration(property_id, value, &base_url,
-                                             Box::new(StdoutErrorReporter), extra_data) {
+    if let Ok(parsed) = parse_one_declaration(property_id, value, &base_url,
+                                              Box::new(StdoutErrorReporter), extra_data) {
         let mut declarations = RwLock::<PropertyDeclarationBlock>::as_arc(&declarations).write();
         let importance = if is_important { Importance::Important } else { Importance::Normal };
         let mut changed = false;
-        for decl in decls.into_iter() {
-            changed |= declarations.set_parsed_declaration(decl.0, importance);
-        }
+        parsed.expand(|decl| {
+            changed |= declarations.set_parsed_declaration(decl, importance);
+        });
         changed
     } else {
         false
@@ -1166,10 +1165,7 @@ pub extern "C" fn Servo_CSSSupports2(property: *const nsACString, value: *const
     let base_url = &*DUMMY_BASE_URL;
     let extra_data = ParserContextExtraData::default();
 
-    match parse_one_declaration(id, &value, &base_url, Box::new(StdoutErrorReporter), extra_data) {
-        Ok(decls) => !decls.is_empty(),
-        Err(()) => false,
-    }
+    parse_one_declaration(id, &value, &base_url, Box::new(StdoutErrorReporter), extra_data).is_ok()
 }
 
 #[no_mangle]