Merge branch 'more-docs' into 'main'

Some internals documentation, and refactor the XML parser a bit

See merge request GNOME/librsvg!738

5 files changed, 166 insertions, 115 deletions

diff --git a/src/api.rs b/src/api.rs
index ee895310..9079278c 100644
--- a/src/api.rs
+++ b/src/api.rs
@@ -13,6 +13,7 @@ pub use crate::{
 use url::Url;
 
 use std::path::Path;
+use std::sync::Arc;
 
 use gio::prelude::*; // Re-exposes glib's prelude as well
 use gio::Cancellable;
@@ -218,7 +219,7 @@ impl Loader {
         Ok(SvgHandle {
             handle: Handle::from_stream(
                 self.session.clone(),
-                &load_options,
+                Arc::new(load_options),
                 stream.as_ref(),
                 cancellable.map(|c| c.as_ref()),
             )?,
diff --git a/src/css.rs b/src/css.rs
index d7bd4817..10ac8243 100644
--- a/src/css.rs
+++ b/src/css.rs
@@ -94,7 +94,7 @@ use crate::io::{self, BinaryData};
 use crate::node::{Node, NodeBorrow, NodeCascade};
 use crate::properties::{parse_value, ComputedValues, ParseAs, ParsedProperty};
 use crate::session::Session;
-use crate::url_resolver::UrlResolver;
+use crate::url_resolver::{AllowedUrl, UrlResolver};
 
 /// A parsed CSS declaration
 ///
@@ -507,10 +507,9 @@ impl SelectorImpl for Selector {
     type PseudoElement = PseudoElement;
 }
 
-/// Wraps an `Node` with a locally-defined type, so we can implement
-/// a foreign trait on it.
+/// Newtype wrapper around `Node` so we can implement [`selectors::Element`] for it.
 ///
-/// `Node` is an alias for `rctree::Node`, so we can't implement
+/// `Node` is an alias for [`rctree::Node`], so we can't implement
 /// `selectors::Element` directly on it.  We implement it on the
 /// `RsvgElement` wrapper instead.
 #[derive(Clone, PartialEq)]
@@ -750,7 +749,7 @@ pub enum Origin {
     Author,
 }
 
-/// A parsed CSS stylesheet
+/// A parsed CSS stylesheet.
 pub struct Stylesheet {
     origin: Origin,
     qualified_rules: Vec<QualifiedRule>,
@@ -758,11 +757,11 @@ pub struct Stylesheet {
 
 /// A match during the selector matching process
 ///
-/// This struct comes from `Stylesheet.get_matches()`, and represents
+/// This struct comes from [`Stylesheet::get_matches`], and represents
 /// that a certain node matched a CSS rule which has a selector with a
 /// certain `specificity`.  The stylesheet's `origin` is also given here.
 ///
-/// This type implements `Ord` so a list of `Match` can be sorted.
+/// This type implements [`Ord`] so a list of `Match` can be sorted.
 /// That implementation does ordering based on origin and specificity
 /// as per <https://www.w3.org/TR/CSS22/cascade.html#cascading-order>.
 struct Match<'a> {
@@ -795,40 +794,47 @@ impl<'a> PartialEq for Match<'a> {
 impl<'a> Eq for Match<'a> {}
 
 impl Stylesheet {
-    pub fn new(origin: Origin) -> Stylesheet {
+    fn empty(origin: Origin) -> Stylesheet {
         Stylesheet {
             origin,
             qualified_rules: Vec::new(),
         }
     }
 
+    /// Parses a new stylesheet from CSS data in a string.
+    ///
+    /// The `url_resolver_url` is required for `@import` rules, so that librsvg can determine if
+    /// the requested path is allowed.
     pub fn from_data(
         buf: &str,
         url_resolver: &UrlResolver,
         origin: Origin,
         session: Session,
     ) -> Result<Self, LoadingError> {
-        let mut stylesheet = Stylesheet::new(origin);
-        stylesheet.parse(buf, url_resolver, session)?;
+        let mut stylesheet = Stylesheet::empty(origin);
+        stylesheet.add_rules_from_string(buf, url_resolver, session)?;
         Ok(stylesheet)
     }
 
+    /// Parses a new stylesheet by loading CSS data from a URL.
     pub fn from_href(
-        href: &str,
-        url_resolver: &UrlResolver,
+        aurl: &AllowedUrl,
         origin: Origin,
         session: Session,
     ) -> Result<Self, LoadingError> {
-        let mut stylesheet = Stylesheet::new(origin);
-        stylesheet.load(href, url_resolver, session)?;
+        let mut stylesheet = Stylesheet::empty(origin);
+        stylesheet.load(aurl, session)?;
         Ok(stylesheet)
     }
 
-    /// Parses a CSS stylesheet from a string
+    /// Parses the CSS rules in `buf` and appends them to the stylesheet.
     ///
-    /// The `base_url` is required for `@import` rules, so that librsvg
-    /// can determine if the requested path is allowed.
-    pub fn parse(
+    /// The `url_resolver_url` is required for `@import` rules, so that librsvg can determine if
+    /// the requested path is allowed.
+    ///
+    /// If there is an `@import` rule, its rules will be recursively added into the
+    /// stylesheet, in the order in which they appear.
+    fn add_rules_from_string(
         &mut self,
         buf: &str,
         url_resolver: &UrlResolver,
@@ -849,10 +855,17 @@ impl Stylesheet {
                 }
             })
             .for_each(|rule| match rule {
-                Rule::AtRule(AtRule::Import(url)) => {
-                    // ignore invalid imports
-                    let _ = self.load(&url, url_resolver, session.clone());
-                }
+                Rule::AtRule(AtRule::Import(url)) => match url_resolver.resolve_href(&url) {
+                    Ok(aurl) => {
+                        // ignore invalid imports
+                        let _ = self.load(&aurl, session.clone());
+                    }
+
+                    Err(e) => {
+                        rsvg_log!(session, "Not loading stylesheet from \"{}\": {}", url, e);
+                    }
+                },
+
                 Rule::QualifiedRule(qr) => self.qualified_rules.push(qr),
             });
 
@@ -860,17 +873,8 @@ impl Stylesheet {
     }
 
     /// Parses a stylesheet referenced by an URL
-    fn load(
-        &mut self,
-        href: &str,
-        url_resolver: &UrlResolver,
-        session: Session,
-    ) -> Result<(), LoadingError> {
-        let aurl = url_resolver
-            .resolve_href(href)
-            .map_err(|_| LoadingError::BadUrl)?;
-
-        io::acquire_data(&aurl, None)
+    fn load(&mut self, aurl: &AllowedUrl, session: Session) -> Result<(), LoadingError> {
+        io::acquire_data(aurl, None)
             .map_err(LoadingError::from)
             .and_then(|data| {
                 let BinaryData {
@@ -895,7 +899,10 @@ impl Stylesheet {
                     LoadingError::BadCss
                 })
             })
-            .and_then(|utf8| self.parse(&utf8, &UrlResolver::new(Some((*aurl).clone())), session))
+            .and_then(|utf8| {
+                let url = (**aurl).clone();
+                self.add_rules_from_string(&utf8, &UrlResolver::new(Some(url)), session)
+            })
     }
 
     /// Appends the style declarations that match a specified node to a given vector
diff --git a/src/document.rs b/src/document.rs
index 21252e40..d0270147 100644
--- a/src/document.rs
+++ b/src/document.rs
@@ -11,6 +11,7 @@ use std::fmt;
 use std::include_str;
 use std::rc::Rc;
 use std::str::FromStr;
+use std::sync::Arc;
 
 use crate::css::{self, Origin, Stylesheet};
 use crate::error::{AcquireError, AllowedUrlError, LoadingError, NodeIdError};
@@ -90,9 +91,9 @@ pub struct Document {
     images: RefCell<Images>,
 
     /// Used to load referenced resources.
-    load_options: LoadOptions,
+    load_options: Arc<LoadOptions>,
 
-    /// Stylesheets defined in the document
+    /// Stylesheets defined in the document.
     stylesheets: Vec<Stylesheet>,
 }
 
@@ -100,13 +101,13 @@ impl Document {
     /// Constructs a `Document` by loading it from a stream.
     pub fn load_from_stream(
         session: Session,
-        load_options: &LoadOptions,
+        load_options: Arc<LoadOptions>,
         stream: &gio::InputStream,
         cancellable: Option<&gio::Cancellable>,
     ) -> Result<Document, LoadingError> {
         xml_load_from_possibly_compressed_stream(
-            DocumentBuilder::new(session, load_options),
-            load_options.unlimited_size,
+            DocumentBuilder::new(session, load_options.clone()),
+            load_options,
             stream,
             cancellable,
         )
@@ -122,7 +123,7 @@ impl Document {
 
         Document::load_from_stream(
             Session::new_for_test_suite(),
-            &LoadOptions::new(UrlResolver::new(None)),
+            Arc::new(LoadOptions::new(UrlResolver::new(None))),
             &stream.upcast(),
             None::<&gio::Cancellable>,
         )
@@ -220,7 +221,7 @@ impl Resources {
                     .and_then(|stream| {
                         Document::load_from_stream(
                             session.clone(),
-                            &load_options.copy_with_base_url(aurl),
+                            Arc::new(load_options.copy_with_base_url(aurl)),
                             &stream,
                             None,
                         )
@@ -492,19 +493,39 @@ impl NodeStack {
     }
 }
 
+/// Used to build a tree of SVG nodes while an XML document is being read.
+///
+/// This struct holds the document-related state while loading an SVG document from XML:
+/// the loading options, the partially-built tree of nodes, the CSS stylesheets that
+/// appear while loading the document.
+///
+/// The XML loader asks a `DocumentBuilder` to
+/// [`append_element`][DocumentBuilder::append_element],
+/// [`append_characters`][DocumentBuilder::append_characters], etc.  When all the XML has
+/// been consumed, the caller can use [`build`][DocumentBuilder::build] to get a
+/// fully-loaded [`Document`].
 pub struct DocumentBuilder {
+    /// Metadata for the document's lifetime.
     session: Session,
-    load_options: LoadOptions,
+
+    /// Loading options; mainly the URL resolver.
+    load_options: Arc<LoadOptions>,
+
+    /// Root node of the tree.
     tree: Option<Node>,
+
+    /// Mapping from `id` attributes to nodes.
     ids: HashMap<String, Node>,
+
+    /// Stylesheets defined in the document.
     stylesheets: Vec<Stylesheet>,
 }
 
 impl DocumentBuilder {
-    pub fn new(session: Session, load_options: &LoadOptions) -> DocumentBuilder {
+    pub fn new(session: Session, load_options: Arc<LoadOptions>) -> DocumentBuilder {
         DocumentBuilder {
             session,
-            load_options: load_options.clone(),
+            load_options,
             tree: None,
             ids: HashMap::new(),
             stylesheets: Vec::new(),
@@ -515,33 +536,24 @@ impl DocumentBuilder {
         &self.session
     }
 
-    pub fn append_stylesheet_from_xml_processing_instruction(
-        &mut self,
-        alternate: Option<String>,
-        type_: Option<String>,
-        href: &str,
-    ) -> Result<(), LoadingError> {
-        if type_.as_deref() != Some("text/css")
-            || (alternate.is_some() && alternate.as_deref() != Some("no"))
-        {
-            return Err(LoadingError::Other(String::from(
-                "invalid parameters in XML processing instruction for stylesheet",
-            )));
-        }
-
-        // FIXME: handle CSS errors
-        if let Ok(stylesheet) = Stylesheet::from_href(
-            href,
-            &self.load_options.url_resolver,
-            Origin::Author,
-            self.session.clone(),
-        ) {
-            self.stylesheets.push(stylesheet);
-        }
-
-        Ok(())
+    /// Adds a stylesheet in order to the document.
+    ///
+    /// Stylesheets will later be matched in the order in which they were added.
+    pub fn append_stylesheet(&mut self, stylesheet: Stylesheet) {
+        self.stylesheets.push(stylesheet);
     }
 
+    /// Creates an element of the specified `name` as a child of `parent`.
+    ///
+    /// This is the main function to create new SVG elements while parsing XML.
+    ///
+    /// `name` is the XML element's name, for example `rect`.
+    ///
+    /// `attrs` has the XML element's attributes, e.g. cx/cy/r for `<circle cx="0" cy="0"
+    /// r="5">`.
+    ///
+    /// If `parent` is `None` it means that we are creating the root node in the tree of
+    /// elements.  The code will later validate that this is indeed an `<svg>` element.
     pub fn append_element(
         &mut self,
         name: &QualName,
@@ -568,38 +580,24 @@ impl DocumentBuilder {
         node
     }
 
-    pub fn append_stylesheet_from_text(&mut self, text: &str) {
-        // FIXME: handle CSS errors
-        if let Ok(stylesheet) = Stylesheet::from_data(
-            text,
-            &self.load_options.url_resolver,
-            Origin::Author,
-            self.session.clone(),
-        ) {
-            self.stylesheets.push(stylesheet);
-        }
-    }
-
+    /// Creates a node for an XML text element as a child of `parent`.
     pub fn append_characters(&mut self, text: &str, parent: &mut Node) {
         if !text.is_empty() {
-            self.append_chars_to_parent(text, parent);
+            // When the last child is a Chars node we can coalesce
+            // the text and avoid screwing up the Pango layouts
+            if let Some(child) = parent.last_child().filter(|c| c.is_chars()) {
+                child.borrow_chars().append(text);
+            } else {
+                parent.append(Node::new(NodeData::new_chars(text)));
+            };
         }
     }
 
-    fn append_chars_to_parent(&mut self, text: &str, parent: &mut Node) {
-        // When the last child is a Chars node we can coalesce
-        // the text and avoid screwing up the Pango layouts
-        if let Some(child) = parent.last_child().filter(|c| c.is_chars()) {
-            child.borrow_chars().append(text);
-        } else {
-            parent.append(Node::new(NodeData::new_chars(text)));
-        };
-    }
-
     pub fn resolve_href(&self, href: &str) -> Result<AllowedUrl, AllowedUrlError> {
         self.load_options.url_resolver.resolve_href(href)
     }
 
+    /// Does the final validation on the `Document` being read, and returns it.
     pub fn build(self) -> Result<Document, LoadingError> {
         let DocumentBuilder {
             load_options,
diff --git a/src/handle.rs b/src/handle.rs
index 62cd9a1e..3ef89c20 100644
--- a/src/handle.rs
+++ b/src/handle.rs
@@ -2,6 +2,8 @@
 //!
 //! This module provides the primitives on which the public APIs are implemented.
 
+use std::sync::Arc;
+
 use crate::accept_language::UserLanguage;
 use crate::bbox::BoundingBox;
 use crate::css::{Origin, Stylesheet};
@@ -88,7 +90,7 @@ impl Handle {
     /// Loads an SVG document into a `Handle`.
     pub fn from_stream(
         session: Session,
-        load_options: &LoadOptions,
+        load_options: Arc<LoadOptions>,
         stream: &gio::InputStream,
         cancellable: Option<&gio::Cancellable>,
     ) -> Result<Handle, LoadingError> {
@@ -371,8 +373,12 @@ impl Handle {
     }
 
     pub fn set_stylesheet(&mut self, css: &str) -> Result<(), LoadingError> {
-        let mut stylesheet = Stylesheet::new(Origin::User);
-        stylesheet.parse(css, &UrlResolver::new(None), self.session.clone())?;
+        let stylesheet = Stylesheet::from_data(
+            css,
+            &UrlResolver::new(None),
+            Origin::User,
+            self.session.clone(),
+        )?;
         self.document.cascade(&[stylesheet], &self.session);
         Ok(())
     }
diff --git a/src/xml/mod.rs b/src/xml/mod.rs
index b06e3fb3..a9ccffac 100644
--- a/src/xml/mod.rs
+++ b/src/xml/mod.rs
@@ -16,11 +16,14 @@ use std::collections::HashMap;
 use std::rc::{Rc, Weak};
 use std::str;
 use std::string::ToString;
+use std::sync::Arc;
 use xml5ever::tendril::format_tendril;
 use xml5ever::tokenizer::{TagKind, Token, TokenSink, XmlTokenizer, XmlTokenizerOpts};
 
+use crate::css::{Origin, Stylesheet};
 use crate::document::{Document, DocumentBuilder};
 use crate::error::{ImplementationLimit, LoadingError};
+use crate::handle::LoadOptions;
 use crate::io::{self, IoError};
 use crate::limits::MAX_LOADED_ELEMENTS;
 use crate::node::{Node, NodeBorrow};
@@ -112,7 +115,7 @@ struct XmlStateInner {
 pub struct XmlState {
     inner: RefCell<XmlStateInner>,
 
-    unlimited_size: bool,
+    load_options: Arc<LoadOptions>,
 }
 
 /// Errors returned from XmlState::acquire()
@@ -135,7 +138,7 @@ impl XmlStateInner {
 }
 
 impl XmlState {
-    fn new(document_builder: DocumentBuilder, unlimited_size: bool) -> XmlState {
+    fn new(document_builder: DocumentBuilder, load_options: Arc<LoadOptions>) -> XmlState {
         XmlState {
             inner: RefCell::new(XmlStateInner {
                 weak: None,
@@ -146,7 +149,7 @@ impl XmlState {
                 entities: HashMap::new(),
             }),
 
-            unlimited_size,
+            load_options,
         }
     }
 
@@ -270,19 +273,39 @@ impl XmlState {
 
             let session = inner.document_builder.as_mut().unwrap().session().clone();
 
+            if type_.as_deref() != Some("text/css")
+                || (alternate.is_some() && alternate.as_deref() != Some("no"))
+            {
+                rsvg_log!(
+                    session,
+                    "invalid parameters in XML processing instruction for stylesheet",
+                );
+                return;
+            }
+
             if let Some(href) = href {
-                // FIXME: https://www.w3.org/TR/xml-stylesheet/ does not seem to specify
-                // what to do if the stylesheet cannot be loaded, so here we ignore the error.
-                if inner
-                    .document_builder
-                    .as_mut()
-                    .unwrap()
-                    .append_stylesheet_from_xml_processing_instruction(alternate, type_, &href)
-                    .is_err()
-                {
+                if let Ok(aurl) = self.load_options.url_resolver.resolve_href(&href) {
+                    if let Ok(stylesheet) =
+                        Stylesheet::from_href(&aurl, Origin::Author, session.clone())
+                    {
+                        inner
+                            .document_builder
+                            .as_mut()
+                            .unwrap()
+                            .append_stylesheet(stylesheet);
+                    } else {
+                        // FIXME: https://www.w3.org/TR/xml-stylesheet/ does not seem to specify
+                        // what to do if the stylesheet cannot be loaded, so here we ignore the error.
+                        rsvg_log!(
+                            session,
+                            "could not create stylesheet from {} in XML processing instruction",
+                            href
+                        );
+                    }
+                } else {
                     rsvg_log!(
                         session,
-                        "invalid xml-stylesheet {} in XML processing instruction",
+                        "{} not allowed for xml-stylesheet in XML processing instruction",
                         href
                     );
                 }
@@ -384,7 +407,18 @@ impl XmlState {
                 .collect::<String>();
 
             let builder = inner.document_builder.as_mut().unwrap();
-            builder.append_stylesheet_from_text(&stylesheet_text);
+            let session = builder.session().clone();
+
+            if let Ok(stylesheet) = Stylesheet::from_data(
+                &stylesheet_text,
+                &self.load_options.url_resolver,
+                Origin::Author,
+                session.clone(),
+            ) {
+                builder.append_stylesheet(stylesheet);
+            } else {
+                rsvg_log!(session, "invalid inline stylesheet");
+            }
         }
     }
 
@@ -601,9 +635,14 @@ impl XmlState {
             .unwrap()
             .upgrade()
             .unwrap();
-        Xml2Parser::from_stream(strong, self.unlimited_size, stream, cancellable)
-            .and_then(|parser| parser.parse())
-            .and_then(|_: ()| self.check_last_error())
+        Xml2Parser::from_stream(
+            strong,
+            self.load_options.unlimited_size,
+            stream,
+            cancellable,
+        )
+        .and_then(|parser| parser.parse())
+        .and_then(|_: ()| self.check_last_error())
     }
 
     fn unsupported_xinclude_start_element(&self, _name: &QualName) -> Context {
@@ -703,11 +742,11 @@ fn parse_xml_stylesheet_processing_instruction(data: &str) -> Result<Vec<(String
 
 pub fn xml_load_from_possibly_compressed_stream(
     document_builder: DocumentBuilder,
-    unlimited_size: bool,
+    load_options: Arc<LoadOptions>,
     stream: &gio::InputStream,
     cancellable: Option<&gio::Cancellable>,
 ) -> Result<Document, LoadingError> {
-    let state = Rc::new(XmlState::new(document_builder, unlimited_size));
+    let state = Rc::new(XmlState::new(document_builder, load_options));
 
     state.inner.borrow_mut().weak = Some(Rc::downgrade(&state));