Created attachment 129078 [details] [review]
cr-sel-eng.diff

* Change libxml-based interface to the selection engine to a generic one.
* Reinstate libxml-based API on top of the generic interface.
* Update tests to use libxml-based API.
* Re-enable gtk-doc support and make sure "make distcheck" works.

This breaks API and would probably have to be called 0.7.

Hello Robert,

Thank you very much for this patch, and sorry for my late reply.

Please find below my comments. If you want, I can post them to the libcroco mailing list if you feel like it would be easier to discuss there.

For now, I am sticking them here.

Thanks.

~=~

> @@ -111,10 +103,11 @@ static xmlNode *get_next_parent_element_node (xmlNode * a_node);
>  
>  static gboolean
>  lang_pseudo_class_handler (CRSelEng * a_this,
> -                           CRAdditionalSel * a_sel, xmlNode * a_node)
> +                           CRAdditionalSel * a_sel, CRNode const * a_node)
>  {
> -        xmlNode *node = a_node;
> -        xmlChar *val = NULL;
> +        CRNode const *node = a_node;
> +        CRNode *child;

Please initialize variable at declaration time. At least for the sake of
consistency with the rest of the code. The rationale has to with being
defensive against our own careless mistakes that can arise at any time.

> +        child = NULL;

Then here you wouldn't need to initialize it as already done at
declaration time.

> @@ -144,19 +140,30 @@ lang_pseudo_class_handler (CRSelEng * a_this,
>                          result = TRUE;
>                  }
>                  if (val) {
> -                        xmlFree (val);
> +                        g_free (val);

I am not sure using g_free here is a good idea, because under the hood, val is allocated by xmlGetProp. So xmlFree (val) is what is due here. g_free(), depending on the Glib allocator that is used could possibly not work as you would expect, from an libxml standpoint. Maybe we should make sure that node->iface->get_attribute (node, "lang") returns something that can be _always_ safely freed by g_free ? Or rather provide a method to release the result of node->iface->get_attribute like cr_node_free_attribute ?

> +                /* Taking care manually not to release a_node. */
> +                if (child && child != a_node) {
> +                        child->iface->release (child);
> +                        child = NULL;
> +                }
> +                child = (CRNode *) node;
> +        }
> +        /* Taking care manually not to release a_node. */
> +        if (child && child != a_node) {
> +                child->iface->release (child);
> +                child = NULL;
> +   

Why releasing the nodes we've just walked ? Is
node->iface->get_parent_node allocating new nodes ?
Also, I don't understand why we should be releaseing the nodes _here_. Shouldn't the nodes be organized in a "Document" like hierarchy so that we are done, destroying the document would then destroy all the nodes at once ?

> @@ -336,42 +338,32 @@ attr_add_sel_matches_node (CRAdditionalSel * a_add_sel, xmlNode * a_node)

[...]

> +                                if (value == NULL) {
> +                                        return FALSE;
> +                                } else if (value
> +                                           && strcmp (value,
> +                                                      cur_sel->value->stryng->str)) {
> +                                        g_free (value);

There is no need to check if value is non NULL, in the else if branch.

> -        for (cur_node = a_node; cur_sel; cur_sel = cur_sel->prev) {
> +        /* We are taking care manually not to release cur_node == a_node. */
> +        for (cur_node = (CRNode *) a_node; cur_sel; cur_sel = cur_sel->prev) {

How does the comment relate to the code of the loop ?

> -                        xmlNode *n = NULL;
> +                        CRNode *n = NULL;
> +                        CRNode *child;
[...]
> +                        child = NULL;

Same comments as earlier regarding variable initialization at
declaration time.

> +struct _CRNode {
> +        CRNodeIface const *iface;
> +};
> +
> +struct _CRNodeIface {
> +       /* Names based on DOM. */
> +       CRNode *        (*get_parent_node)      (CRNode const *);
> +       CRNode *        (*get_first_child)      (CRNode const *);
> +       CRNode *        (*get_next_sibling)     (CRNode const *);
> +       CRNode *        (*get_previous_sibling) (CRNode const *);
> +       char const *    (*get_node_name)        (CRNode const *);
> +       char *          (*get_attribute)        (CRNode const *, char const *);
> +       /* Additional methods for DOM emulation / CSS matching. */
> +       guint           (*get_children_count)   (CRNode const *);
> +       guint           (*get_index)            (CRNode const *);
> +       void            (*release)              (CRNode *);
> +};

For encapsulation purposes, and to shield us against future ABI
breakage, I'd put the definition of _CRNode and _CRNodeIface in a .c
file, keeping only typedef struct _CRNode CRNode; in the header file.

I'd provide methods to create a node, e.g cr_node_*_new().

I'd provide an init method that'd be called by constructors of nodes inheriting CRNode: cr_node_init (CRNode *a_node);

I'd provide methods to access the iface members, e.g:
CRNode* cr_node_get_parent (CRNode *a_node);
CRNode* cr_node_get_first_child (CRNode *a_node);
...

I'd provide methods to set the members of the iface, eg:
cr_node_set_get_parent_handler (CRNode *, GetParentFuncPtr);

These methods would be called by the constructors of the nodes inheriting CRNode to set the behaviour of cr_node_get_parent, cr_node_get_first_child etc.

That way, adding a new iface member or re-organising the layout of the
iface structure wouldn't incur any risk of ABI breakage.


> Index: src/cr-xml-sel-eng.h

In this header file, I would expect to have only one method declared:
CRNode* cr_new_xml_node (/*xml node ?*/);

Methods cr_sel_eng_register_pseudo_class_sel_handler to cr_sel_eng_get_pseudo_class_selector_handler should be moved into cr-sel-eng.h and be de-commented.

Methods cr_xml_sel_eng_matches_node to cr_xml_sel_eng_get_matched_style shouldn't exist in this form, at least not publicly. What should be public is cr_sel_eng_matches_node ... cr_sel_eng_get_matched_style. 
Does this make sense ?

> +
> +static CRNode *
> +get_parent_node (CRNode const *node)
> +{
> +        CRXmlNode       *self = (CRXmlNode *) node;
> +        xmlNode         *cur_node;
> +
> +        g_return_val_if_fail (self, NULL);
> +        g_return_val_if_fail (self->node, NULL);
> +
> +        CR_XML_LOG (self->node);
> +
> +        cur_node = self->node->parent;
> +        while (cur_node && cur_node->type != XML_ELEMENT_NODE) {
> +                cur_node = cur_node->parent;
> +        }
> +
> +        return cr_xml_node_new (cur_node);
> +}

Hmmh, this doesn't look right to me.
get_parent_node should not _create_ a new node. Really. The same thing applies to all the node siblings and children getters that comes in the same file.

The document which nodes are instances of CRNode should exist _before_ the selection engine tries to walk it to interpret the selector expressions present in the style sheet. That's an implicit semantic of CSS I think.

So, we could say that CRNode is a typedef of void*, and in the case of XML documents, a CRNode* would be just an xmlNode*. That would oblige us to make the CRSeleng carry the vtable of functions that is used to walk the tree CRNode. I think that could work. 

This approach would keep the semantics of Documents/CSS, and would have amongst his positive side effect that no CRNode memory management would be needed in the CRSeleng during the selectors expressions evalutation.

Any thought about this ?

~=~

Thanks for your comments Dodji. I agree it would be a good idea to seal the CRNode struct, although ABI will still be broken if the struct changes size. Would it make sense to add some padding to the Iface too?

Confirming bug, as this is a feature we would definitely like to add.

(In reply to comment #3)
> Thanks for your comments Dodji. I agree it would be a good idea to seal the
> CRNode struct, although ABI will still be broken if the struct changes size.
> Would it make sense to add some padding to the Iface too?

Sorry, but with the changes I proposed, normally, nobody should ever be dereferencing any instance CRNode*. Is that correct ?

If it is correct, then I don't think adding a new member to CRNode, would break the API, as "public" code would only see pointers to CRNode, not instances of it.

Does this make sense ?

Thomas, Dodji,
Any chance to adapt this patch and integrate in libcroco? This would clearly help get away with a bunch of embedded copies of libcroco, eg. in inkscape, avoiding divergences and helping maintainability.
See for example http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/annotate/head%3A/src/libcroco/README

libcroco is not under development anymore. Its codebase has been archived.

Closing this report as WONTFIX as part of Bugzilla Housekeeping to reflect
reality. Please feel free to reopen this ticket (or rather transfer the project
to GNOME Gitlab, as GNOME Bugzilla is being shut down) if anyone takes the
responsibility for active development again.