[librsvg] IRI: store a Fragment, not just a plain String

[Federico Mena Quintero <federico src gnome org>]

commit 9969002bb482a9cf8295ec01975061a2d1630547
Author: Federico Mena Quintero <federico gnome org>
Date:   Thu Nov 29 17:07:08 2018 -0600

    IRI: store a Fragment, not just a plain String
    
    All the places that need a <funciri>, i.e. the IRI type, are style
    properties that reference an SVG element - therefore a Fragment.

 rsvg_internals/src/defs.rs        | 21 +++++++++++++++---
 rsvg_internals/src/drawing_ctx.rs |  8 +++----
 rsvg_internals/src/iri.rs         | 45 +++++++++++++++++++++++++--------------
 rsvg_internals/src/marker.rs      | 15 ++++---------
 4 files changed, 55 insertions(+), 34 deletions(-)
---
diff --git a/rsvg_internals/src/defs.rs b/rsvg_internals/src/defs.rs
index 049c64a8..ee5632c0 100644
--- a/rsvg_internals/src/defs.rs
+++ b/rsvg_internals/src/defs.rs
@@ -1,6 +1,7 @@
 use libc;
 use std::collections::hash_map::Entry;
 use std::collections::HashMap;
+use std::fmt;
 use std::ptr;
 use std::rc::Rc;
 
@@ -81,10 +82,13 @@ pub enum Href {
 }
 
 /// Optional URI, mandatory fragment id
-#[derive(Debug, PartialEq)]
+#[derive(Debug, PartialEq, Clone)]
 pub struct Fragment(Option<String>, String);
 
 impl Fragment {
+    // Outside of testing, we don't want code creating Fragments by hand;
+    // they should get them from Href.
+    #[cfg(test)]
     pub fn new(uri: Option<String>, fragment: String) -> Fragment {
         Fragment(uri, fragment)
     }
@@ -98,6 +102,17 @@ impl Fragment {
     }
 }
 
+impl fmt::Display for Fragment {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        write!(
+            f,
+            "{}#{}",
+            self.0.as_ref().map(String::as_str).unwrap_or(""),
+            self.1
+        )
+    }
+}
+
 /// Errors returned when creating an `Href` out of a string
 #[derive(Debug, PartialEq)]
 pub enum HrefError {
@@ -132,11 +147,11 @@ impl Href {
 
         match (uri, fragment) {
             (None, Some(f)) if f.len() == 0 => Err(HrefError::ParseError),
-            (None, Some(f)) => Ok(Href::WithFragment(Fragment::new(None, f.to_string()))),
+            (None, Some(f)) => Ok(Href::WithFragment(Fragment(None, f.to_string()))),
             (Some(u), _) if u.len() == 0 => Err(HrefError::ParseError),
             (Some(u), None) => Ok(Href::PlainUri(u.to_string())),
             (Some(_u), Some(f)) if f.len() == 0 => Err(HrefError::ParseError),
-            (Some(u), Some(f)) => Ok(Href::WithFragment(Fragment::new(
+            (Some(u), Some(f)) => Ok(Href::WithFragment(Fragment(
                 Some(u.to_string()),
                 f.to_string(),
             ))),
diff --git a/rsvg_internals/src/drawing_ctx.rs b/rsvg_internals/src/drawing_ctx.rs
index d35efa06..124475e9 100644
--- a/rsvg_internals/src/drawing_ctx.rs
+++ b/rsvg_internals/src/drawing_ctx.rs
@@ -403,7 +403,7 @@ impl<'a> DrawingCtx<'a> {
             let affine = original_cr.get_matrix();
 
             let (acquired_clip, clip_units) = {
-                if let Some(acquired) = self.get_acquired_href_of_type(clip_uri, NodeType::ClipPath)
+                if let Some(acquired) = self.get_acquired_node_of_type(clip_uri, NodeType::ClipPath)
                 {
                     let ClipPathUnits(units) = acquired
                         .get()
@@ -489,7 +489,7 @@ impl<'a> DrawingCtx<'a> {
 
                 if let Some(mask) = mask {
                     if let Some(acquired) =
-                        self.get_acquired_href_of_type(Some(mask), NodeType::Mask)
+                        self.get_acquired_node_of_type(Some(mask), NodeType::Mask)
                     {
                         let node = acquired.get();
 
@@ -524,14 +524,14 @@ impl<'a> DrawingCtx<'a> {
 
     fn run_filter(
         &mut self,
-        filter_uri: &str,
+        filter_uri: &Fragment,
         node: &RsvgNode,
         values: &ComputedValues,
         child_surface: &cairo::ImageSurface,
     ) -> Result<cairo::ImageSurface, RenderingError> {
         let output = self.surfaces_stack.pop().unwrap();
 
-        match self.get_acquired_href_of_type(Some(filter_uri), NodeType::Filter) {
+        match self.get_acquired_node_of_type(Some(filter_uri), NodeType::Filter) {
             Some(acquired) => {
                 let filter_node = acquired.get();
 
diff --git a/rsvg_internals/src/iri.rs b/rsvg_internals/src/iri.rs
index 03347171..ca0280bf 100644
--- a/rsvg_internals/src/iri.rs
+++ b/rsvg_internals/src/iri.rs
@@ -1,5 +1,6 @@
 use cssparser::Parser;
 
+use defs::{Fragment, Href};
 use parsers::Parse;
 use parsers::ParseError;
 
@@ -12,7 +13,7 @@ use parsers::ParseError;
 #[derive(Debug, Clone, PartialEq)]
 pub enum IRI {
     None,
-    Resource(String),
+    Resource(Fragment),
 }
 
 impl Default for IRI {
@@ -23,10 +24,10 @@ impl Default for IRI {
 
 impl IRI {
     /// Returns the contents of an `IRI::Resource`, or `None`
-    pub fn get(&self) -> Option<&str> {
+    pub fn get(&self) -> Option<&Fragment> {
         match *self {
             IRI::None => None,
-            IRI::Resource(ref s) => Some(s.as_ref()),
+            IRI::Resource(ref f) => Some(f),
         }
     }
 }
@@ -47,7 +48,14 @@ impl Parse for IRI {
                 .expect_exhausted()
                 .map_err(|_| ParseError::new("expected url"))?;
 
-            Ok(IRI::Resource(url.as_ref().to_owned()))
+            let href =
+                Href::with_fragment(&url).map_err(|_| ParseError::new("could not parse href"))?;
+
+            if let Href::WithFragment(fragment) = href {
+                Ok(IRI::Resource(fragment))
+            } else {
+                Err(ParseError::new("href requires a fragment identifier"))
+            }
         }
     }
 }
@@ -64,28 +72,33 @@ mod tests {
     #[test]
     fn parses_url() {
         assert_eq!(
-            IRI::parse_str("url(foo)", ()),
-            Ok(IRI::Resource("foo".to_string()))
+            IRI::parse_str("url(#bar)", ()),
+            Ok(IRI::Resource(Fragment::new(None, "bar".to_string())))
+        );
+
+        assert_eq!(
+            IRI::parse_str("url(foo#bar)", ()),
+            Ok(IRI::Resource(Fragment::new(
+                Some("foo".to_string()),
+                "bar".to_string()
+            )))
         );
 
         // be permissive if the closing ) is missing
         assert_eq!(
-            IRI::parse_str("url(", ()),
-            Ok(IRI::Resource("".to_string()))
+            IRI::parse_str("url(#bar", ()),
+            Ok(IRI::Resource(Fragment::new(None, "bar".to_string())))
         );
         assert_eq!(
-            IRI::parse_str("url(foo", ()),
-            Ok(IRI::Resource("foo".to_string()))
+            IRI::parse_str("url(foo#bar", ()),
+            Ok(IRI::Resource(Fragment::new(
+                Some("foo".to_string()),
+                "bar".to_string()
+            )))
         );
 
         assert!(IRI::parse_str("", ()).is_err());
         assert!(IRI::parse_str("foo", ()).is_err());
         assert!(IRI::parse_str("url(foo)bar", ()).is_err());
     }
-
-    #[test]
-    fn get() {
-        assert_eq!(IRI::None.get(), None);
-        assert_eq!(IRI::Resource(String::from("foo")).get(), Some("foo"));
-    }
 }
diff --git a/rsvg_internals/src/marker.rs b/rsvg_internals/src/marker.rs
index b1d594b9..b58c1b1a 100644
--- a/rsvg_internals/src/marker.rs
+++ b/rsvg_internals/src/marker.rs
@@ -6,6 +6,7 @@ use cssparser::{CowRcStr, Parser, Token};
 
 use aspect_ratio::*;
 use attributes::Attribute;
+use defs::Fragment;
 use drawing_ctx::DrawingCtx;
 use error::*;
 use float_eq_cairo::ApproxEqCairo;
@@ -634,14 +635,14 @@ enum MarkerType {
 
 fn emit_marker_by_name(
     draw_ctx: &mut DrawingCtx<'_>,
-    name: &str,
+    name: &Fragment,
     xpos: f64,
     ypos: f64,
     computed_angle: f64,
     line_width: f64,
     clipping: bool,
 ) -> Result<(), RenderingError> {
-    if let Some(acquired) = draw_ctx.get_acquired_href_of_type(Some(name), NodeType::Marker) {
+    if let Some(acquired) = draw_ctx.get_acquired_node_of_type(Some(name), NodeType::Marker) {
         let node = acquired.get();
 
         node.with_impl(|marker: &NodeMarker| {
@@ -721,15 +722,7 @@ pub fn render_markers_for_path_builder(
                 MarkerType::Middle => &values.marker_mid.0,
                 MarkerType::End => &values.marker_end.0,
             } {
-                emit_marker_by_name(
-                    draw_ctx,
-                    &marker,
-                    x,
-                    y,
-                    computed_angle,
-                    line_width,
-                    clipping,
-                )
+                emit_marker_by_name(draw_ctx, marker, x, y, computed_angle, line_width, clipping)
             } else {
                 Ok(())
             }