Re: [PATCH] msxml3: Handle SAX tags and characters for mxwriter domdoc.

[Nikolay Sivov]

On 4/10/21 4:27 AM, Jefferson Carpenter wrote:
> Moved some code from mxwriter ISAXContentHandler implementation into
> free functions that can be swapped out for DOMDocument functions.
>
> For other mxwriter callback functions, immediately return E_NOTIMPL if
> the destination is a DOMDocument.
>
> Added test_mxwriter_domdoc_start_end_document to help check the
> implementation of DOMDocument locking.
>
> Added "BOOL locked" to XML documents and a private interface that
> allows this variable to be set and checked.  (This variable is not
> used thread safely.  `LONG refs` is thread safe, but `struct list
> orphans` is not.)
>
> thanks,
> Jefferson
> @@ -125,6 +125,7 @@ struct domdoc
>      IPersistStreamInit        IPersistStreamInit_iface;
>      IObjectWithSite           IObjectWithSite_iface;
>      IObjectSafety             IObjectSafety_iface;
> - IWineXMLDOMDocumentLock   IWineXMLDOMDocumentLock_iface;
> IConnectionPointContainer IConnectionPointContainer_iface;
> LONG ref;
> VARIANT_BOOL async;

Maybe that could work, yes.
> @@ -209,6 +210,7 @@ typedef struct _xmldoc_priv {
>      LONG refs;
>      struct list orphans;
>      domdoc_properties* properties;
> - BOOL locked;
>
> } xmldoc_priv;

Why does it have to be in _priv?
> diff --git a/dlls/msxml3/node.c b/dlls/msxml3/node.c
> index 67f322eb0e5..9cbb2305830 100644
> --- a/dlls/msxml3/node.c
> +++ b/dlls/msxml3/node.c
> @@ -457,6 +457,8 @@ HRESULT node_insert_before(xmlnode *This, IXMLDOMNode *new_child, const VARIANT
>      if(!new_child)
>          return E_INVALIDARG;
> - if (xmldoc_get_locked(This->node->doc)) return E_FAIL;
> - node_obj = get_node_obj(new_child);
>    if(!node_obj) return E_FAIL;
>
> @@ -569,6 +571,8 @@ HRESULT node_replace_child(xmlnode *This, IXMLDOMNode *newChild, IXMLDOMNode *ol
>      if(!newChild || !oldChild)
>          return E_INVALIDARG;
> - if (xmldoc_get_locked(This->node->doc)) return E_FAIL;
> - if(ret)
>        *ret = NULL;
>
> @@ -627,6 +631,8 @@ HRESULT node_remove_child(xmlnode *This, IXMLDOMNode* child, IXMLDOMNode** oldCh
>  if(!child) return E_INVALIDARG;
>
> - if (xmldoc_get_locked(This->node->doc)) return E_FAIL;
> - if(oldChild)
>        *oldChild = NULL;

Maybe, but there's many more methods that modify the tree.
> +static HRESULT writer_end_tag_domdoc(mxwriter *writer, const WCHAR *qname, int len) {
> - HRESULT hr;
> - IXMLDOMNode *parent_node;
> -
> - IWineXMLDOMDocumentLock_put_locked(writer->doc_lock, 0);
> -
> - hr = domdoc_maybe_write_chars(writer);
> - if (FAILED(hr)) return hr;
> -
> - IWineXMLDOMDocumentLock_put_locked(writer->doc_lock, 1);
> -
> - hr = IXMLDOMNode_get_parentNode(writer->current_node, &parent_node);
> - if (FAILED(hr)) return hr;
> -
> - IXMLDOMNode_Release(writer->current_node);
> - writer->current_node = parent_node;
> - return hr;
>
> +}

I don't understand this pattern. Idea I had was to lock when output is
set to a document, and unlock when everything is written. Is that wrong?
> +static HRESULT writer_start_tag_string_len_stream(mxwriter *writer, const WCHAR *qname, int len)
> +{
> - static const WCHAR ltW[] = {'<'};
> -
> - close_element_starttag(writer);
> - set_element_name(writer, qname ? qname : emptyW, qname ? len : 0);
> -
> - write_node_indent(writer);
> -
> - write_output_buffer(writer, ltW, 1);
> - write_output_buffer(writer, qname ? qname : emptyW, qname ? len : 0);
> - writer_inc_indent(writer);
> -
> - return S_OK;
>
> +}
> -
>
> +static HRESULT writer_start_tag_bstr_stream(mxwriter *writer, BSTR name)
> +{
> - return writer_start_tag_string_len_stream(writer, name, SysStringLen(name));
>
> +}
> -

Do you need both?