[librsvg: 1/2] Filter property: refactor to Filter::None and Filter::List(_)

[Federico Mena Quintero <federico src gnome org>]

commit 7ae65b4834a02dbb7c98019864178d903fd1ef0e
Author: John Ledbetter <john ledbetter gmail com>
Date:   Wed Sep 9 13:53:06 2020 -0400

    Filter property: refactor to Filter::None and Filter::List(_)
    
    Previously, a Filter was always a FilterValueList, which could
    potentially be empty.  This commit updates a Filter to be either a
    Filter::None or a Filter::List which contains a FilterValueList.
    
    Additionally, we remove the ability for a FilterValueList to parse
    a 'none' element, since this is now handled at the Filter level.

 rsvg_internals/src/drawing_ctx.rs     | 102 ++++++++++++++++++++--------------
 rsvg_internals/src/filter.rs          |  13 +----
 rsvg_internals/src/property_defs.rs   |  23 +++++++-
 rsvg_internals/src/property_macros.rs |  12 ++++
 4 files changed, 94 insertions(+), 56 deletions(-)
---
diff --git a/rsvg_internals/src/drawing_ctx.rs b/rsvg_internals/src/drawing_ctx.rs
index 3687e098..e4c77d72 100644
--- a/rsvg_internals/src/drawing_ctx.rs
+++ b/rsvg_internals/src/drawing_ctx.rs
@@ -18,7 +18,7 @@ use crate::document::AcquiredNodes;
 use crate::dpi::Dpi;
 use crate::element::Element;
 use crate::error::{AcquireError, RenderingError};
-use crate::filter::{FilterValue, FilterValueList};
+use crate::filter::FilterValue;
 use crate::filters;
 use crate::float_eq_cairo::ApproxEqCairo;
 use crate::gradient::{Gradient, GradientUnits, GradientVariant, SpreadMethod};
@@ -29,7 +29,7 @@ use crate::path_builder::*;
 use crate::pattern::{PatternContentUnits, PatternUnits, ResolvedPattern};
 use crate::properties::ComputedValues;
 use crate::property_defs::{
-    ClipRule, FillRule, MixBlendMode, Opacity, Overflow, PaintTarget, ShapeRendering,
+    ClipRule, FillRule, Filter, MixBlendMode, Opacity, Overflow, PaintTarget, ShapeRendering,
     StrokeDasharray, StrokeLinecap, StrokeLinejoin, TextRendering,
 };
 use crate::rect::Rect;
@@ -531,11 +531,11 @@ impl DrawingCtx {
 
                 let filters = if node.is_element() {
                     match *node.borrow_element() {
-                        Element::Mask(_) => FilterValueList::default(),
-                        _ => values.filter().0,
+                        Element::Mask(_) => Filter::None,
+                        _ => values.filter(),
                     }
                 } else {
-                    values.filter().0
+                    values.filter()
                 };
 
                 let UnitInterval(opacity) = values.opacity().0;
@@ -550,7 +550,7 @@ impl DrawingCtx {
 
                 let is_opaque = approx_eq!(f64, opacity, 1.0);
                 let needs_temporary_surface = !(is_opaque
-                    && filters.is_empty()
+                    && filters == Filter::None
                     && mask.is_none()
                     && values.mix_blend_mode() == MixBlendMode::Normal
                     && clip_in_object_space.is_none());
@@ -566,12 +566,13 @@ impl DrawingCtx {
 
                     // Create temporary surface and its cr
 
-                    let cr = if filters.is_empty() {
-                        cairo::Context::new(
+                    let cr = match filters {
+                        Filter::None => cairo::Context::new(
                             &dc.create_similar_surface_for_toplevel_viewport(&dc.cr.get_target())?,
-                        )
-                    } else {
-                        cairo::Context::new(&*dc.create_surface_for_toplevel_viewport()?)
+                        ),
+                        Filter::List(_) => {
+                            cairo::Context::new(&*dc.create_surface_for_toplevel_viewport()?)
+                        }
                     };
 
                     cr.set_matrix(affines.for_temporary_surface.into());
@@ -589,37 +590,8 @@ impl DrawingCtx {
                     };
 
                     // Filter
-
-                    let source_surface = if filters.is_applicable(node, acquired_nodes) {
-                        // The target surface has multiple references.
-                        // We need to copy it to a new surface to have a unique
-                        // reference to be able to safely access the pixel data.
-                        let child_surface = SharedImageSurface::copy_from_surface(
-                            &cairo::ImageSurface::try_from(dc.cr.get_target()).unwrap(),
-                        )?;
-
-                        let img_surface = filters
-                            .iter()
-                            .try_fold(
-                                child_surface,
-                                |surface, filter| -> Result<_, RenderingError> {
-                                    let FilterValue::URL(filter_uri) = filter;
-                                    dc.run_filter(
-                                        acquired_nodes,
-                                        &filter_uri,
-                                        node,
-                                        values,
-                                        surface,
-                                        bbox,
-                                    )
-                                },
-                            )?
-                            .into_image_surface()?;
-                        // turn ImageSurface into a Surface
-                        (*img_surface).clone()
-                    } else {
-                        dc.cr.get_target()
-                    };
+                    let source_surface =
+                        dc.run_filters(&filters, acquired_nodes, node, values, bbox)?;
 
                     dc.pop_cairo_context();
 
@@ -805,6 +777,52 @@ impl DrawingCtx {
         res
     }
 
+    fn run_filters(
+        &mut self,
+        filters: &Filter,
+        acquired_nodes: &mut AcquiredNodes,
+        node: &Node,
+        values: &ComputedValues,
+        node_bbox: BoundingBox,
+    ) -> Result<cairo::Surface, RenderingError> {
+        let surface = match filters {
+            Filter::None => self.cr.get_target(),
+            Filter::List(filter_list) => {
+                if filter_list.is_applicable(node, acquired_nodes) {
+                    // The target surface has multiple references.
+                    // We need to copy it to a new surface to have a unique
+                    // reference to be able to safely access the pixel data.
+                    let child_surface = SharedImageSurface::copy_from_surface(
+                        &cairo::ImageSurface::try_from(self.cr.get_target()).unwrap(),
+                    )?;
+
+                    let img_surface = filter_list
+                        .iter()
+                        .try_fold(
+                            child_surface,
+                            |surface, filter| -> Result<_, RenderingError> {
+                                let FilterValue::URL(filter_uri) = filter;
+                                self.run_filter(
+                                    acquired_nodes,
+                                    &filter_uri,
+                                    node,
+                                    values,
+                                    surface,
+                                    node_bbox,
+                                )
+                            },
+                        )?
+                        .into_image_surface()?;
+                    // turn ImageSurface into a Surface
+                    (*img_surface).clone()
+                } else {
+                    self.cr.get_target()
+                }
+            }
+        };
+        Ok(surface)
+    }
+
     fn run_filter(
         &mut self,
         acquired_nodes: &mut AcquiredNodes,
diff --git a/rsvg_internals/src/filter.rs b/rsvg_internals/src/filter.rs
index eb44766b..3547d25e 100644
--- a/rsvg_internals/src/filter.rs
+++ b/rsvg_internals/src/filter.rs
@@ -255,13 +255,6 @@ impl Parse for FilterValueList {
     fn parse<'i>(parser: &mut Parser<'i, '_>) -> Result<Self, crate::error::ParseError<'i>> {
         let mut result = FilterValueList::default();
 
-        if parser
-            .try_parse(|p| p.expect_ident_matching("none"))
-            .is_ok()
-        {
-            return Ok(result);
-        }
-
         loop {
             let state = parser.state();
 
@@ -288,11 +281,6 @@ mod tests {
 
     #[test]
     fn parses_filter_value_list() {
-        assert_eq!(
-            FilterValueList::parse_str("none"),
-            Ok(FilterValueList::default())
-        );
-
         let f1 = Fragment::new(Some("foo.svg".to_string()), "bar".to_string());
         let f2 = Fragment::new(Some("test.svg".to_string()), "baz".to_string());
         assert_eq!(
@@ -306,6 +294,7 @@ mod tests {
 
     #[test]
     fn detects_invalid_filter_value_list() {
+        assert!(FilterValueList::parse_str("none").is_err());
         assert!(FilterValueList::parse_str("").is_err());
         assert!(FilterValueList::parse_str("fail").is_err());
         assert!(FilterValueList::parse_str("url(#test) none").is_err());
diff --git a/rsvg_internals/src/property_defs.rs b/rsvg_internals/src/property_defs.rs
index 9eab1d4a..ee1b8f2a 100644
--- a/rsvg_internals/src/property_defs.rs
+++ b/rsvg_internals/src/property_defs.rs
@@ -247,13 +247,32 @@ make_property!(
     "evenodd" => EvenOdd,
 );
 
+#[derive(Debug, Clone, PartialEq)]
+pub enum Filter {
+    None,
+    List(FilterValueList),
+}
 // https://www.w3.org/TR/SVG/filters.html#FilterProperty
 make_property!(
     ComputedValues,
     Filter,
-    default: FilterValueList::default(),
+    default: Filter::None,
     inherits_automatically: false,
-    newtype_parse: FilterValueList,
+    parse_impl: {
+        impl Parse for Filter {
+            fn parse<'i>(parser: &mut Parser<'i, '_>) -> Result<Self, crate::error::ParseError<'i>> {
+
+                if parser
+                    .try_parse(|p| p.expect_ident_matching("none"))
+                    .is_ok()
+                {
+                    return Ok(Filter::None);
+                }
+
+                Ok(Filter::List(FilterValueList::parse(parser)?))
+            }
+        }
+    }
 );
 
 // https://www.w3.org/TR/SVG/filters.html#FloodColorProperty
diff --git a/rsvg_internals/src/property_macros.rs b/rsvg_internals/src/property_macros.rs
index 6e212d3f..abcd7b3e 100644
--- a/rsvg_internals/src/property_macros.rs
+++ b/rsvg_internals/src/property_macros.rs
@@ -167,6 +167,18 @@ macro_rules! make_property {
         impl_property!($computed_values_type, $name, $inherits_automatically);
     };
 
+    ($computed_values_type: ty,
+     $name: ident,
+     default: $default: expr,
+     inherits_automatically: $inherits_automatically: expr,
+     parse_impl: { $parse: item }
+    ) => {
+        impl_default!($name, $default);
+        impl_property!($computed_values_type, $name, $inherits_automatically);
+
+        $parse
+    };
+
     // pending - only BaselineShift
     ($computed_values_type: ty,
      $name: ident,