Refactor and fix src parsing in @font-face

5 files changed, 124 insertions, 169 deletions

diff --git a/src/components/layout/layout_task.rs b/src/components/layout/layout_task.rs
index 029c8f8b4f4..a0e1dd45d89 100644
--- a/src/components/layout/layout_task.rs
+++ b/src/components/layout/layout_task.rs
@@ -495,10 +495,8 @@ impl LayoutTask {
             match rule {
                 &CSSFontFaceRule(ref font_face_rule) => {
                     let mut font_urls = vec!();
-                    for source_line in font_face_rule.source_lines.iter() {
-                        for source in source_line.sources.iter() {
-                            font_urls.push(source.url.clone());
-                        }
+                    for source in font_face_rule.sources.iter() {
+                        font_urls.push(source.url.clone());
                     }
                     self.font_cache_task.add_web_font(font_urls, font_face_rule.family.as_slice());
                 },
diff --git a/src/components/style/font_face.rs b/src/components/style/font_face.rs
index 2c3771ebbcc..2eafa242baf 100644
--- a/src/components/style/font_face.rs
+++ b/src/components/style/font_face.rs
@@ -6,37 +6,29 @@ use cssparser::ast::*;
 use cssparser::parse_declaration_list;
 use errors::{ErrorLoggerIterator, log_css_error};
 use std::ascii::StrAsciiExt;
-use parsing_utils::BufferedIter;
+use parsing_utils::{BufferedIter, ParserIter, parse_slice_comma_separated};
+use properties::longhands::font_family::parse_one_family;
+use properties::computed_values::font_family::FamilyName;
 use stylesheets::{CSSRule, CSSFontFaceRule};
 use url::{Url, UrlParser};
 
-#[deriving(PartialEq)]
-pub enum FontFaceFormat {
-    UnknownFormat,
-    WoffFormat,
-    TtfFormat,
-    SvgFormat,
-    EotFormat,
-}
 
-pub struct FontFaceSource {
-    pub url: Url,
-    pub format_hints: Vec<FontFaceFormat>,
+enum Source {
+    UrlSource(UrlSource),
+    LocalSource(String),
 }
 
-pub struct FontFaceSourceLine {
-    pub sources: Vec<FontFaceSource>
+pub struct UrlSource {
+    pub url: Url,
+    pub format_hints: Vec<String>,
 }
 
 pub struct FontFaceRule {
     pub family: String,
-    pub source_lines: Vec<FontFaceSourceLine>,
+    pub sources: Vec<UrlSource>,  // local() is not supported yet
 }
 
 pub fn parse_font_face_rule(rule: AtRule, parent_rules: &mut Vec<CSSRule>, base_url: &Url) {
-    let mut maybe_family = None;
-    let mut source_lines = vec!();
-
     if rule.prelude.as_slice().skip_whitespace().next().is_some() {
         log_css_error(rule.location, "@font-face prelude contains unexpected characters");
         return;
@@ -50,6 +42,9 @@ pub fn parse_font_face_rule(rule: AtRule, parent_rules: &mut Vec<CSSRule>, base_
         }
     };
 
+    let mut maybe_family = None;
+    let mut maybe_sources = None;
+
     for item in ErrorLoggerIterator(parse_declaration_list(block.move_iter())) {
         match item {
             DeclAtRule(rule) => log_css_error(
@@ -63,8 +58,8 @@ pub fn parse_font_face_rule(rule: AtRule, parent_rules: &mut Vec<CSSRule>, base_
                 match name_lower.as_slice() {
                     "font-family" => {
                         let iter = &mut BufferedIter::new(value.as_slice().skip_whitespace());
-                        match ::properties::longhands::font_family::parse_one_family(iter) {
-                            Ok(::properties::computed_values::font_family::FamilyName(name)) => {
+                        match parse_one_family(iter) {
+                            Ok(FamilyName(name)) => {
                                 maybe_family = Some(name);
                             },
                             // This also includes generic family names:
@@ -72,124 +67,11 @@ pub fn parse_font_face_rule(rule: AtRule, parent_rules: &mut Vec<CSSRule>, base_
                         }
                     },
                     "src" => {
-                        let mut iter = value.as_slice().skip_whitespace();
-                        let mut sources = vec!();
-                        let mut syntax_error = false;
-
-                        'outer: loop {
-
-                            // url() or local() should be next
-                            let maybe_url = match iter.next() {
-                                Some(&URL(ref string_value)) => {
-                                    let maybe_url = UrlParser::new().base_url(base_url).parse(string_value.as_slice());
-                                    let url = maybe_url.unwrap_or_else(|_| Url::parse("about:invalid").unwrap());
-                                    Some(url)
-                                },
-                                Some(&Function(ref string_value, ref _values)) => {
-                                    match string_value.as_slice() {
-                                        "local" => {
-                                            log_css_error(location, "local font face is not supported yet - skipping");
-                                            None
-                                        },
-                                        _ => {
-                                            log_css_error(location, format!("Unexpected token {}", string_value).as_slice());
-                                            syntax_error = true;
-                                            break;
-                                        }
-                                    }
-                                },
-                                _ => {
-                                    log_css_error(location, "Unsupported declaration type");
-                                    syntax_error = true;
-                                    break;
-                                }
-                            };
-
-                            let mut next_token = iter.next();
-
-                            match maybe_url {
-                                Some(url) => {
-                                    let mut source = FontFaceSource {
-                                        url: url,
-                                        format_hints: vec!(),
-                                    };
-
-                                    // optional format, or comma to start loop again
-                                    match next_token {
-                                        Some(&Function(ref string_value, ref values)) => {
-                                            match string_value.as_slice() {
-                                                "format" => {
-                                                    let mut format_iter = values.as_slice().skip_whitespace();
-
-                                                    loop {
-                                                        let fmt_token = format_iter.next();
-                                                        match fmt_token {
-                                                            Some(&String(ref format_hint)) => {
-                                                                let hint = match format_hint.as_slice() {
-                                                                    "embedded-opentype" => EotFormat,
-                                                                    "woff" => WoffFormat,
-                                                                    "truetype" | "opentype" => TtfFormat,
-                                                                    "svg" => SvgFormat,
-                                                                    _ => UnknownFormat,
-                                                                };
-                                                                source.format_hints.push(hint);
-                                                            },
-                                                            _ => {
-                                                                log_css_error(location, "Unexpected token");
-                                                                syntax_error = true;
-                                                                break 'outer;
-                                                            }
-                                                        }
-
-                                                        let comma_token = format_iter.next();
-                                                        match comma_token {
-                                                            Some(&Comma) => {},
-                                                            None => {
-                                                                break;
-                                                            }
-                                                            _ => {
-                                                                log_css_error(location, "Unexpected token");
-                                                                syntax_error = true;
-                                                                break 'outer;
-                                                            }
-                                                        }
-                                                    }
-                                                },
-                                                _ => {
-                                                    log_css_error(location,
-                                                                    format!("Unsupported token {}", string_value).as_slice());
-                                                    syntax_error = true;
-                                                    break;
-                                                }
-                                            }
-                                            next_token = iter.next();
-                                        },
-                                        _ => {}
-                                    }
-
-                                    sources.push(source);
-                                },
-                                None => {},
-                            }
-
-                            // after url or optional format, comes comma or end
-                            match next_token {
-                                Some(&Comma) => {},
-                                None => break,
-                                _ => {
-                                    log_css_error(location, "Unexpected token type");
-                                    syntax_error = true;
-                                    break;
-                                }
-                            }
-                        }
-
-                        if !syntax_error && sources.len() > 0 {
-                            let source_line = FontFaceSourceLine {
-                                sources: sources
-                            };
-                            source_lines.push(source_line);
-                        }
+                        match parse_slice_comma_separated(
+                                value.as_slice(), |iter| parse_one_url_src(iter, base_url)) {
+                            Ok(sources) => maybe_sources = Some(sources),
+                            Err(()) => log_css_error(location, "Invalid src in @font-face"),
+                        };
                     },
                     _ => {
                         log_css_error(location, format!("Unsupported declaration {:s}", name).as_slice());
@@ -199,11 +81,78 @@ pub fn parse_font_face_rule(rule: AtRule, parent_rules: &mut Vec<CSSRule>, base_
         }
     }
 
-    if maybe_family.is_some() && source_lines.len() > 0 {
-        let font_face_rule = FontFaceRule {
-            family: maybe_family.unwrap(),
-            source_lines: source_lines,
-        };
-        parent_rules.push(CSSFontFaceRule(font_face_rule));
+    match (maybe_family, maybe_sources) {
+        (Some(family), Some(sources)) => parent_rules.push(CSSFontFaceRule(FontFaceRule {
+            family: family,
+            sources: sources,
+        })),
+        (None, _) => log_css_error(rule.location, "@font-face without a font-family descriptor"),
+        _ => log_css_error(rule.location, "@font-face without an src descriptor"),
+    }
+}
+
+
+/// local() is not supported yet
+fn parse_one_url_src(iter: ParserIter, base_url: &Url) -> Result<UrlSource, ()> {
+    match parse_one_src(iter, base_url) {
+        Ok(UrlSource(source)) => Ok(source),
+        _ => Err(())
+    }
+}
+
+
+fn parse_one_src(iter: ParserIter, base_url: &Url) -> Result<Source, ()> {
+    let url = match iter.next() {
+        // Parsing url()
+        Some(&URL(ref url)) => {
+            UrlParser::new().base_url(base_url).parse(url.as_slice()).unwrap_or_else(
+                |_error| Url::parse("about:invalid").unwrap())
+        },
+        // Parsing local() with early return()
+        Some(&Function(ref name, ref arguments)) => {
+            if name.as_slice().eq_ignore_ascii_case("local") {
+                let iter = &mut BufferedIter::new(arguments.as_slice().skip_whitespace());
+                match parse_one_family(iter) {
+                    Ok(FamilyName(name)) => return Ok(LocalSource(name)),
+                    _ => return Err(())
+                }
+            }
+            return Err(())
+        },
+        _ => return Err(())
+    };
+
+    // Parsing optional format()
+    let format_hints = match iter.next() {
+        Some(&Function(ref name, ref arguments)) => {
+            if !name.as_slice().eq_ignore_ascii_case("format") {
+                return Err(())
+            }
+            try!(parse_slice_comma_separated(arguments.as_slice(), parse_one_format))
+        }
+        Some(component_value) => {
+            iter.push_back(component_value);
+            vec![]
+        }
+        None => vec![],
+    };
+
+    Ok(UrlSource(UrlSource {
+        url: url,
+        format_hints: format_hints,
+    }))
+}
+
+
+fn parse_one_format(iter: ParserIter) -> Result<String, ()> {
+    match iter.next() {
+        Some(&String(ref value)) => {
+            if iter.next().is_none() {
+                Ok(value.clone())
+            } else {
+                Err(())
+            }
+        }
+        _ => Err(())
     }
 }
diff --git a/src/components/style/parsing_utils.rs b/src/components/style/parsing_utils.rs
index 413cad78601..3afd7ba0353 100644
--- a/src/components/style/parsing_utils.rs
+++ b/src/components/style/parsing_utils.rs
@@ -4,7 +4,7 @@
 
 
 use std::ascii::StrAsciiExt;
-use cssparser::ast::{ComponentValue, Ident, SkipWhitespaceIterable, SkipWhitespaceIterator};
+use cssparser::ast::{ComponentValue, Ident, Comma, SkipWhitespaceIterable, SkipWhitespaceIterator};
 
 
 pub fn one_component_value<'a>(input: &'a [ComponentValue]) -> Result<&'a ComponentValue, ()> {
@@ -56,5 +56,26 @@ impl<E, I: Iterator<E>> Iterator<E> for BufferedIter<E, I> {
     }
 }
 
+pub type ParserIter<'a, 'b> = &'a mut BufferedIter<&'b ComponentValue, SkipWhitespaceIterator<'b>>;
 
-pub type ParserIter<'a> = BufferedIter<&'a ComponentValue, SkipWhitespaceIterator<'a>>;
+
+#[inline]
+pub fn parse_slice_comma_separated<T>(input: &[ComponentValue],
+                                      parse_one: |ParserIter| -> Result<T, ()>)
+                                      -> Result<Vec<T>, ()> {
+    parse_comma_separated(&mut BufferedIter::new(input.skip_whitespace()), parse_one)
+}
+
+#[inline]
+pub fn parse_comma_separated<T>(iter: ParserIter,
+                                parse_one: |ParserIter| -> Result<T, ()>)
+                                -> Result<Vec<T>, ()> {
+    let mut values = vec![try!(parse_one(iter))];
+    for component_value in iter {
+        match component_value {
+            &Comma => values.push(try!(parse_one(iter))),
+            _ => return Err(())
+        }
+    }
+    Ok(values)
+}
diff --git a/src/components/style/properties/mod.rs.mako b/src/components/style/properties/mod.rs.mako
index c3677242ee6..e699c392a77 100644
--- a/src/components/style/properties/mod.rs.mako
+++ b/src/components/style/properties/mod.rs.mako
@@ -727,22 +727,9 @@ pub mod longhands {
         /// <familiy-name> = <string> | [ <ident>+ ]
         /// TODO: <generic-familiy>
         pub fn parse(input: &[ComponentValue], _base_url: &Url) -> Result<SpecifiedValue, ()> {
-            from_iter(input.skip_whitespace())
+            parse_slice_comma_separated(input, parse_one_family)
         }
-        pub fn from_iter<'a>(iter: SkipWhitespaceIterator<'a>) -> Result<SpecifiedValue, ()> {
-            let iter = &mut BufferedIter::new(iter);
-            let mut families = vec![try!(parse_one_family(iter))];
-            for component_value in iter {
-                match component_value {
-                    &Comma => {
-                        families.push(try!(parse_one_family(iter)));
-                    },
-                    _ => return Err(())
-                }
-            }
-            Ok(families)
-        }
-        pub fn parse_one_family<'a>(iter: &mut ParserIter) -> Result<FontFamily, ()> {
+        pub fn parse_one_family<'a>(iter: ParserIter) -> Result<FontFamily, ()> {
             // TODO: avoid copying strings?
             let mut idents = match iter.next() {
                 Some(&String(ref value)) => return Ok(FamilyName(value.clone())),
@@ -1337,15 +1324,15 @@ pub mod shorthands {
             }
             _ => ()
         }
-        let family = font_family::from_iter(iter).ok();
-        if family.is_none() { return Err(()) }
+        let family = try!(parse_comma_separated(
+            &mut BufferedIter::new(iter), font_family::parse_one_family));
         Ok(Longhands {
             font_style: style,
             font_variant: variant,
             font_weight: weight,
             font_size: size,
             line_height: line_height,
-            font_family: family
+            font_family: Some(family)
         })
     </%self:shorthand>
 
diff --git a/src/components/style/style.rs b/src/components/style/style.rs
index 36576c1cae9..043e83993df 100644
--- a/src/components/style/style.rs
+++ b/src/components/style/style.rs
@@ -44,7 +44,7 @@ pub use selectors::{NamespaceConstraint, Selector, CompoundSelector, SimpleSelec
 pub use selectors::{LocalNameSelector, parse_selector_list};
 pub use namespaces::NamespaceMap;
 pub use media_queries::{MediaRule, MediaQueryList, MediaQuery, Device, MediaType, MediaQueryType};
-pub use font_face::{FontFaceFormat, FontFaceRule, FontFaceSource,FontFaceSourceLine, TtfFormat};
+pub use font_face::{FontFaceRule};
 
 mod stylesheets;
 mod errors;