[librsvg: 2/10] Cleanup the code that looks up node in the document

[Federico Mena Quintero <federico src gnome org>]

commit 7bac50d10c24ef7d411d10ac0ddcca5b203332ee
Author: Paolo Borelli <pborelli gnome org>
Date:   Sat Dec 12 11:20:41 2020 +0100

    Cleanup the code that looks up node in the document

 src/css.rs      | 17 ++++------------
 src/document.rs | 61 +++++++++++++++++++++++----------------------------------
 src/handle.rs   | 52 +++++++++++++++++++++---------------------------
 3 files changed, 50 insertions(+), 80 deletions(-)
---
diff --git a/src/css.rs b/src/css.rs
index 2e508035..11e2d65b 100644
--- a/src/css.rs
+++ b/src/css.rs
@@ -801,7 +801,6 @@ mod tests {
 
     use crate::document::Document;
     use crate::handle::LoadOptions;
-    use crate::url_resolver::Fragment;
 
     fn load_document(input: &'static [u8]) -> Document {
         let bytes = glib::Bytes::from_static(input);
@@ -827,18 +826,10 @@ mod tests {
 "#,
         );
 
-        let a = document
-            .lookup(&Fragment::new(None, "a".to_string()))
-            .unwrap();
-        let b = document
-            .lookup(&Fragment::new(None, "b".to_string()))
-            .unwrap();
-        let c = document
-            .lookup(&Fragment::new(None, "c".to_string()))
-            .unwrap();
-        let d = document
-            .lookup(&Fragment::new(None, "d".to_string()))
-            .unwrap();
+        let a = document.lookup_node(None, "a").unwrap();
+        let b = document.lookup_node(None, "b").unwrap();
+        let c = document.lookup_node(None, "c").unwrap();
+        let d = document.lookup_node(None, "d").unwrap();
 
         // Node types
         assert!(is_element_of_type!(a, Svg));
diff --git a/src/document.rs b/src/document.rs
index fff43d3d..55a0a312 100644
--- a/src/document.rs
+++ b/src/document.rs
@@ -73,26 +73,18 @@ impl Document {
         self.tree.clone()
     }
 
-    /// Looks up an element node by its URL.
-    ///
-    /// This is also used to find elements in referenced resources, as in
-    /// `xlink:href="subresource.svg#element_name".
-    pub fn lookup(&self, fragment: &Fragment) -> Result<Node, LoadingError> {
-        if fragment.uri().is_some() {
-            self.externs
+    /// Looks up a node in this document or one of its resources by its `id` attribute.
+    pub fn lookup_node(&self, url: Option<&str>, id: &str) -> Option<Node> {
+        match url {
+            Some(u) => self
+                .externs
                 .borrow_mut()
-                .lookup(&self.load_options, fragment)
-        } else {
-            self.lookup_node_by_id(fragment.fragment())
-                .ok_or(LoadingError::BadUrl)
+                .lookup(&self.load_options, u, id)
+                .ok(),
+            _ => self.ids.get(id).map(|n| (*n).clone()),
         }
     }
 
-    /// Looks up a node only in this document fragment by its `id` attribute.
-    pub fn lookup_node_by_id(&self, id: &str) -> Option<Node> {
-        self.ids.get(id).map(|n| (*n).clone())
-    }
-
     /// Loads an image by URL, or returns a pre-loaded one.
     pub fn lookup_image(&self, href: &str) -> Result<SharedImageSurface, LoadingError> {
         let aurl = self
@@ -127,17 +119,11 @@ impl Resources {
     pub fn lookup(
         &mut self,
         load_options: &LoadOptions,
-        fragment: &Fragment,
+        url: &str,
+        id: &str,
     ) -> Result<Node, LoadingError> {
-        if let Some(ref href) = fragment.uri() {
-            self.get_extern_document(load_options, href)
-                .and_then(|doc| {
-                    doc.lookup_node_by_id(fragment.fragment())
-                        .ok_or(LoadingError::BadUrl)
-                })
-        } else {
-            unreachable!();
-        }
+        self.get_extern_document(load_options, url)
+            .and_then(|doc| doc.lookup_node(None, id).ok_or(LoadingError::BadUrl))
     }
 
     fn get_extern_document(
@@ -325,17 +311,18 @@ impl<'i> AcquiredNodes<'i> {
             return Err(AcquireError::MaxReferencesExceeded);
         }
 
-        let node = self.document.lookup(fragment).map_err(|_| {
-            // FIXME: callers shouldn't have to know that get_node() can initiate a file load.
-            // Maybe we should have the following stages:
-            //   - load main SVG XML
-            //
-            //   - load secondary SVG XML and other files like images; all document::Resources and
-            //     document::Images loaded
-            //
-            //   - Now that all files are loaded, resolve URL references
-            AcquireError::LinkNotFound(fragment.clone())
-        })?;
+        // FIXME: callers shouldn't have to know that get_node() can initiate a file load.
+        // Maybe we should have the following stages:
+        //   - load main SVG XML
+        //
+        //   - load secondary SVG XML and other files like images; all document::Resources and
+        //     document::Images loaded
+        //
+        //   - Now that all files are loaded, resolve URL references
+        let node = self
+            .document
+            .lookup_node(fragment.uri(), fragment.fragment())
+            .ok_or_else(|| AcquireError::LinkNotFound(fragment.clone()))?;
 
         if !node.is_element() {
             return Err(AcquireError::InvalidLinkType(fragment.clone()));
diff --git a/src/handle.rs b/src/handle.rs
index 1bfc90c4..b20e700c 100644
--- a/src/handle.rs
+++ b/src/handle.rs
@@ -11,7 +11,7 @@ use crate::error::{DefsLookupErrorKind, LoadingError, RenderingError};
 use crate::node::{CascadedValues, Node, NodeBorrow};
 use crate::rect::Rect;
 use crate::structure::IntrinsicDimensions;
-use crate::url_resolver::{AllowedUrl, Href, UrlResolver};
+use crate::url_resolver::{AllowedUrl, Fragment, UrlResolver};
 
 /// Loading options for SVG documents.
 #[derive(Clone)]
@@ -192,36 +192,28 @@ impl Handle {
     }
 
     fn lookup_node(&self, id: &str) -> Result<Node, DefsLookupErrorKind> {
-        match Href::parse(&id).map_err(DefsLookupErrorKind::HrefError)? {
-            Href::PlainUrl(_) => Err(DefsLookupErrorKind::CannotLookupExternalReferences),
-            Href::WithFragment(fragment) => {
-                if let Some(uri) = fragment.uri() {
-                    // The public APIs to get geometries of individual elements, or to render
-                    // them, should only allow referencing elements within the main handle's
-                    // SVG file; that is, only plain "#foo" fragment IDs are allowed here.
-                    // Otherwise, a calling program could request "another-file#foo" and cause
-                    // another-file to be loaded, even if it is not part of the set of
-                    // resources that the main SVG actually references.  In the future we may
-                    // relax this requirement to allow lookups within that set, but not to
-                    // other random files.
-
-                    let msg = format!(
-                        "the public API is not allowed to look up external references: {}#{}",
-                        uri,
-                        fragment.fragment()
-                    );
-
-                    rsvg_log!("{}", msg);
-
-                    return Err(DefsLookupErrorKind::CannotLookupExternalReferences);
-                }
-
-                match self.document.lookup_node_by_id(fragment.fragment()) {
-                    Some(n) => Ok(n),
-                    None => Err(DefsLookupErrorKind::NotFound),
-                }
-            }
+        let fragment = Fragment::parse(&id).map_err(DefsLookupErrorKind::HrefError)?;
+
+        // The public APIs to get geometries of individual elements, or to render
+        // them, should only allow referencing elements within the main handle's
+        // SVG file; that is, only plain "#foo" fragment IDs are allowed here.
+        // Otherwise, a calling program could request "another-file#foo" and cause
+        // another-file to be loaded, even if it is not part of the set of
+        // resources that the main SVG actually references.  In the future we may
+        // relax this requirement to allow lookups within that set, but not to
+        // other random files.
+        if fragment.uri().is_some() {
+            rsvg_log!(
+                "the public API is not allowed to look up external references: {}",
+                fragment
+            );
+
+            return Err(DefsLookupErrorKind::CannotLookupExternalReferences);
         }
+
+        self.document
+            .lookup_node(None, fragment.fragment())
+            .ok_or(DefsLookupErrorKind::NotFound)
     }
 
     pub fn render_document(