[librsvg: 2/22] Store the node ids in Svg, not in Defs

[Federico Mena Quintero <federico src gnome org>]

commit b833c1a99289fd4290530cbf04e95d0098f2768b
Author: Federico Mena Quintero <federico gnome org>
Date:   Mon Jan 7 15:28:18 2019 -0600

    Store the node ids in Svg, not in Defs
    
    With this, Defs becomes just a holding place for externally-loaded SVG
    files.  Hopefully we can turn it into a holding place for all
    externally-loaded resources, i.e. all the files that were loaded as a
    result of loading an SVG from the public API.

 rsvg_internals/src/create_node.rs |  8 ++++----
 rsvg_internals/src/defs.rs        | 23 ++++-------------------
 rsvg_internals/src/drawing_ctx.rs |  2 +-
 rsvg_internals/src/handle.rs      | 36 +++++++++++-------------------------
 rsvg_internals/src/svg.rs         | 24 +++++++++++++++++++++---
 rsvg_internals/src/xml.rs         |  9 ++++++---
 6 files changed, 47 insertions(+), 55 deletions(-)
---
diff --git a/rsvg_internals/src/create_node.rs b/rsvg_internals/src/create_node.rs
index 086d0973..3e6a8b0e 100644
--- a/rsvg_internals/src/create_node.rs
+++ b/rsvg_internals/src/create_node.rs
@@ -2,7 +2,6 @@ use std::collections::HashMap;
 
 use attributes::Attribute;
 use clip_path::NodeClipPath;
-use defs::Defs;
 use filters::{
     blend::Blend,
     color_matrix::ColorMatrix,
@@ -271,7 +270,7 @@ pub fn create_node_and_register_id(
     name: &str,
     parent: Option<&RsvgNode>,
     pbag: &PropertyBag,
-    defs: &mut Defs,
+    ids: &mut HashMap<String, RsvgNode>,
 ) -> RsvgNode {
     let mut id = None;
     let mut class = None;
@@ -298,8 +297,9 @@ pub fn create_node_and_register_id(
 
     let node = create_fn(id, class, parent);
 
-    if id.is_some() {
-        defs.insert(id.unwrap(), &node);
+    if let Some(id) = id {
+        // This is so we don't overwrite an existing id
+        ids.entry(id.to_string()).or_insert(node.clone());
     }
 
     node
diff --git a/rsvg_internals/src/defs.rs b/rsvg_internals/src/defs.rs
index 91591297..3f4271d2 100644
--- a/rsvg_internals/src/defs.rs
+++ b/rsvg_internals/src/defs.rs
@@ -10,41 +10,26 @@ use node::Node;
 use parsers::ParseError;
 
 pub struct Defs {
-    nodes: HashMap<String, Rc<Node>>,
     externs: HashMap<AllowedUrl, *const RsvgHandle>,
 }
 
 impl Defs {
     pub fn new() -> Defs {
         Defs {
-            nodes: Default::default(),
             externs: Default::default(),
         }
     }
 
-    pub fn insert(&mut self, id: &str, node: &Rc<Node>) {
-        self.nodes.entry(id.to_string()).or_insert(node.clone());
-    }
-
-    pub fn lookup_fragment_id(&self, id: &str) -> Option<Rc<Node>> {
-        self.nodes.get(id).map(|n| (*n).clone())
-    }
-
-    /// Returns a node referenced by a fragment ID
-    ///
-    /// If the `Fragment`'s URL is `None`, then the fragment ID refers
-    /// to the RSVG handle in question.  Otherwise, it will refer to
-    /// an externally-loaded SVG file that is referenced by the
-    /// current one.  If the element's id is not found, returns
-    /// `None`.
+    /// Returns a node referenced by a fragment ID, from an
+    /// externally-loaded SVG file.
     pub fn lookup(&mut self, handle: *const RsvgHandle, fragment: &Fragment) -> Option<Rc<Node>> {
         if let Some(ref href) = fragment.uri() {
             match self.get_extern_handle(handle, href) {
-                Ok(extern_handle) => handle::lookup_fragment_id(extern_handle, fragment),
+                Ok(extern_handle) => handle::lookup_fragment_id(extern_handle, fragment.fragment()),
                 Err(()) => None,
             }
         } else {
-            self.lookup_fragment_id(fragment.fragment())
+            unreachable!();
         }
     }
 
diff --git a/rsvg_internals/src/drawing_ctx.rs b/rsvg_internals/src/drawing_ctx.rs
index fe3223f3..9fded6c2 100644
--- a/rsvg_internals/src/drawing_ctx.rs
+++ b/rsvg_internals/src/drawing_ctx.rs
@@ -263,7 +263,7 @@ impl DrawingCtx {
     // acquire it again.  If you acquire a node "#foo" and don't release it before
     // trying to acquire "foo" again, you will obtain a %NULL the second time.
     pub fn get_acquired_node(&mut self, fragment: &Fragment) -> Option<AcquiredNode> {
-        if let Some(node) = self.svg.lookup_node(fragment) {
+        if let Some(node) = self.svg.lookup(fragment) {
             if !self.acquired_nodes_contains(&node) {
                 self.acquired_nodes.borrow_mut().push(node.clone());
                 let acq = AcquiredNode(self.acquired_nodes.clone(), node.clone());
diff --git a/rsvg_internals/src/handle.rs b/rsvg_internals/src/handle.rs
index 45145239..21724d99 100644
--- a/rsvg_internals/src/handle.rs
+++ b/rsvg_internals/src/handle.rs
@@ -18,7 +18,7 @@ use url::Url;
 
 use allowed_url::AllowedUrl;
 use css::{self, CssStyles};
-use defs::{Fragment, Href};
+use defs::Href;
 use dpi::Dpi;
 use drawing_ctx::{DrawingCtx, RsvgRectangle};
 use error::{set_gerror, DefsLookupErrorKind, LoadingError, RenderingError};
@@ -344,9 +344,7 @@ impl Handle {
         };
 
         let (node, is_root) = if let Some(id) = id {
-            let n = self
-                .defs_lookup(handle, id)
-                .map_err(RenderingError::InvalidId)?;
+            let n = self.lookup_node(id).map_err(RenderingError::InvalidId)?;
             let is_root = Rc::ptr_eq(&n, &root);
             (n, is_root)
         } else {
@@ -373,17 +371,11 @@ impl Handle {
         self.get_node_geometry(handle, &node)
     }
 
-    fn defs_lookup(
-        &mut self,
-        handle: *const RsvgHandle,
-        name: &str,
-    ) -> Result<RsvgNode, DefsLookupErrorKind> {
+    fn lookup_node(&mut self, id: &str) -> Result<RsvgNode, DefsLookupErrorKind> {
         let svg_ref = self.svg.borrow();
         let svg = svg_ref.as_ref().unwrap();
 
-        let mut defs = svg.defs.borrow_mut();
-
-        let href = Href::with_fragment(name).map_err(DefsLookupErrorKind::HrefError)?;
+        let href = Href::with_fragment(id).map_err(DefsLookupErrorKind::HrefError)?;
 
         match href {
             Href::WithFragment(fragment) => {
@@ -409,7 +401,7 @@ impl Handle {
                     return Err(DefsLookupErrorKind::CannotLookupExternalReferences);
                 }
 
-                match defs.lookup(handle, &fragment) {
+                match svg.lookup_node_by_id(fragment.fragment()) {
                     Some(n) => Ok(n),
                     None => Err(DefsLookupErrorKind::NotFound),
                 }
@@ -419,9 +411,9 @@ impl Handle {
         }
     }
 
-    fn has_sub(&mut self, handle: *const RsvgHandle, name: &str) -> bool {
+    fn has_sub(&mut self, name: &str) -> bool {
         // FIXME: return a proper error; only NotFound should map to false
-        self.defs_lookup(handle, name).is_ok()
+        self.lookup_node(name).is_ok()
     }
 
     fn render_cairo_sub(
@@ -442,10 +434,7 @@ impl Handle {
         }
 
         let node = if let Some(id) = id {
-            Some(
-                self.defs_lookup(handle, id)
-                    .map_err(RenderingError::InvalidId)?,
-            )
+            Some(self.lookup_node(id).map_err(RenderingError::InvalidId)?)
         } else {
             None
         };
@@ -571,16 +560,13 @@ extern "C" {
 }
 
 // Looks up a node by its id.
-//
-// Note that this ignores the Fragment's url; it only uses the fragment identifier.
-pub fn lookup_fragment_id(handle: *const RsvgHandle, fragment: &Fragment) -> Option<Rc<Node>> {
+pub fn lookup_fragment_id(handle: *const RsvgHandle, id: &str) -> Option<Rc<Node>> {
     let rhandle = get_rust_handle(handle);
 
     let svg_ref = rhandle.svg.borrow();
     let svg = svg_ref.as_ref().unwrap();
-    let defs_ref = svg.defs.borrow();
 
-    defs_ref.lookup_fragment_id(fragment.fragment())
+    svg.lookup_node_by_id(id)
 }
 
 pub fn load_extern(handle: *const RsvgHandle, aurl: &AllowedUrl) -> Result<*const RsvgHandle, ()> {
@@ -1019,7 +1005,7 @@ pub unsafe extern "C" fn rsvg_handle_rust_has_sub(
     }
 
     let id: String = from_glib_none(id);
-    rhandle.has_sub(handle, &id).to_glib()
+    rhandle.has_sub(&id).to_glib()
 }
 
 #[no_mangle]
diff --git a/rsvg_internals/src/svg.rs b/rsvg_internals/src/svg.rs
index 9774f6a2..ca81c3fa 100644
--- a/rsvg_internals/src/svg.rs
+++ b/rsvg_internals/src/svg.rs
@@ -1,4 +1,5 @@
 use std::cell::RefCell;
+use std::collections::HashMap;
 
 use css::CssStyles;
 use defs::{Defs, Fragment};
@@ -20,20 +21,37 @@ pub struct Svg {
     // once, at loading time, and keep this immutable.
     pub defs: RefCell<Defs>,
 
+    ids: HashMap<String, RsvgNode>,
+
     pub css_styles: CssStyles,
 }
 
 impl Svg {
-    pub fn new(handle: *mut RsvgHandle, tree: Tree, defs: Defs, css_styles: CssStyles) -> Svg {
+    pub fn new(
+        handle: *mut RsvgHandle,
+        tree: Tree,
+        defs: Defs,
+        ids: HashMap<String, RsvgNode>,
+        css_styles: CssStyles,
+    ) -> Svg {
         Svg {
             handle,
             tree,
             defs: RefCell::new(defs),
+            ids,
             css_styles,
         }
     }
 
-    pub fn lookup_node(&self, fragment: &Fragment) -> Option<RsvgNode> {
-        self.defs.borrow_mut().lookup(self.handle, fragment)
+    pub fn lookup(&self, fragment: &Fragment) -> Option<RsvgNode> {
+        if fragment.uri().is_some() {
+            self.defs.borrow_mut().lookup(self.handle, fragment)
+        } else {
+            self.lookup_node_by_id(fragment.fragment())
+        }
+    }
+
+    pub fn lookup_node_by_id(&self, id: &str) -> Option<RsvgNode> {
+        self.ids.get(id).map(|n| (*n).clone())
     }
 }
diff --git a/rsvg_internals/src/xml.rs b/rsvg_internals/src/xml.rs
index af22bd4d..a28a4909 100644
--- a/rsvg_internals/src/xml.rs
+++ b/rsvg_internals/src/xml.rs
@@ -14,7 +14,7 @@ use css::{self, CssStyles};
 use defs::Defs;
 use error::LoadingError;
 use handle::{self, RsvgHandle};
-use node::{node_new, Node, NodeType};
+use node::{node_new, Node, NodeType, RsvgNode};
 use property_bag::PropertyBag;
 use structure::NodeSvg;
 use style::NodeStyle;
@@ -71,6 +71,7 @@ extern "C" {
 pub struct XmlState {
     tree: Option<Tree>,
     defs: Option<Defs>,
+    ids: Option<HashMap<String, RsvgNode>>,
     css_styles: Option<CssStyles>,
     context_stack: Vec<Context>,
     current_node: Option<Rc<Node>>,
@@ -97,6 +98,7 @@ impl XmlState {
         XmlState {
             tree: None,
             defs: Some(Defs::new()),
+            ids: Some(HashMap::new()),
             css_styles: Some(CssStyles::new()),
             context_stack: vec![Context::Start],
             current_node: None,
@@ -130,6 +132,7 @@ impl XmlState {
             self.handle,
             self.tree.take().unwrap(),
             self.defs.take().unwrap(),
+            self.ids.take().unwrap(),
             self.css_styles.take().unwrap(),
         )
     }
@@ -315,9 +318,9 @@ impl XmlState {
         name: &str,
         pbag: &PropertyBag,
     ) -> Rc<Node> {
-        let defs = self.defs.as_mut().unwrap();
+        let ids = self.ids.as_mut().unwrap();
 
-        let new_node = create_node_and_register_id(name, parent, pbag, defs);
+        let new_node = create_node_and_register_id(name, parent, pbag, ids);
 
         if let Some(parent) = parent {
             parent.add_child(&new_node);