[PATCH v2 0/2] MR9056: msxml3/element: Implement removeAttributeNode function.
Implement the function `IXMLDOMElement_removeAttributeNode`. Essentially pass the hard work to `IXMLDOMNamedNodeMap_removeNamedItem`. Use the fact, that attribute names are unique in elements. One case that isn't checked is wheter or not the value of the provided attribute match with the one stored in the dom-element. This MR implements according to the tests introduced in https://gitlab.winehq.org/wine/wine/-/merge_requests/8928 -- v2: msxml3/element: do attribute removal ourselfs https://gitlab.winehq.org/wine/wine/-/merge_requests/9056
From: Reinhold Gschweicher <pyro4hell+winehq(a)gmail.com> Implement the function `IXMLDOMElement_removeAttributeNode`. Essentially pass the hard work to `IXMLDOMNamedNodeMap_removeNamedItem`. Use the fact, that attribute names are unique in elements. One case that isn't checked is wheter or not the value of the provided attribute match with the one stored in the dom-element. --- dlls/msxml3/element.c | 31 +++++++++++++++++++++++++++++-- dlls/msxml3/tests/domdoc.c | 4 +--- 2 files changed, 30 insertions(+), 5 deletions(-) diff --git a/dlls/msxml3/element.c b/dlls/msxml3/element.c index 3e3c2d06169..41ba2493709 100644 --- a/dlls/msxml3/element.c +++ b/dlls/msxml3/element.c @@ -1491,8 +1491,35 @@ static HRESULT WINAPI domelem_removeAttributeNode( IXMLDOMAttribute** attributeNode) { domelem *This = impl_from_IXMLDOMElement( iface ); - FIXME("(%p)->(%p %p)\n", This, domAttribute, attributeNode); - return E_NOTIMPL; + DOMNodeType type; + HRESULT hr; + BSTR attr_name; + IXMLDOMNamedNodeMap* attr_map; + + TRACE("(%p)->(%p %p)\n", This, domAttribute, attributeNode); + + if (!domAttribute) { + return E_INVALIDARG; + } + + /* check given node type to be an attribute */ + hr = IXMLDOMNode_get_nodeType((IXMLDOMNode *)domAttribute, &type); + if (hr != S_OK) return hr; + if (type != NODE_ATTRIBUTE) { + return E_INVALIDARG; + } + + /* get attribute node name and reuse removeNamedItem function */ + hr = IXMLDOMAttribute_get_nodeName(domAttribute, &attr_name); + if (hr != S_OK) return hr; + hr = domelem_get_attributes(iface, &attr_map); + if (hr != S_OK) return hr; + hr = IXMLDOMNamedNodeMap_removeNamedItem(attr_map, attr_name, (IXMLDOMNode **)attributeNode); + IXMLDOMNamedNodeMap_Release(attr_map); + /* removeNameItem returns S_FALSE if not found, + * removeAttributeNode is expected to return E_INVALIDARG */ + if (hr == S_FALSE) return E_INVALIDARG; + return hr; } static HRESULT WINAPI domelem_getElementsByTagName( diff --git a/dlls/msxml3/tests/domdoc.c b/dlls/msxml3/tests/domdoc.c index dcda4821125..4e795422bf2 100644 --- a/dlls/msxml3/tests/domdoc.c +++ b/dlls/msxml3/tests/domdoc.c @@ -2331,10 +2331,8 @@ static void test_domnode( void ) ok(attr != NULL, "getAttributeNode returned NULL\n"); attr_out = NULL; hr = IXMLDOMElement_removeAttributeNode(element, attr, &attr_out ); -todo_wine { ok(hr == S_OK, "Unexpected hr %#lx.\n", hr); ok(attr_out != NULL, "removeAttributeNode expected to set attr_out, but got NULL pointer\n"); -} if (attr_out) { /* remove the same attribute again returns invalid arg */ @@ -2353,7 +2351,7 @@ todo_wine { attr_out = (IXMLDOMAttribute*)0xdeadbeef; hr = IXMLDOMElement_removeAttributeNode( element, NULL, &attr_out ); - todo_wine ok(hr == E_INVALIDARG, "removeAttributeNode removed a NULL pointer hr: %#lx.\n", hr); + ok(hr == E_INVALIDARG, "removeAttributeNode removed a NULL pointer hr: %#lx.\n", hr); ok(attr_out == (IXMLDOMAttribute*)0xdeadbeef, "removeAttributeNode expected to not touch attr_out in error case, got (%p)\n", attr_out); hr = IXMLDOMElement_get_attributes( element, &map ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9056
From: Reinhold Gschweicher <pyro4hell+winehq(a)gmail.com> Check if the given attribute has the element as parent. If so remove it just like `domelem_remove_qualified_item` does. --- dlls/msxml3/element.c | 35 +++++++++++++++-------------------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/dlls/msxml3/element.c b/dlls/msxml3/element.c index 41ba2493709..c311e327212 100644 --- a/dlls/msxml3/element.c +++ b/dlls/msxml3/element.c @@ -1491,35 +1491,30 @@ static HRESULT WINAPI domelem_removeAttributeNode( IXMLDOMAttribute** attributeNode) { domelem *This = impl_from_IXMLDOMElement( iface ); - DOMNodeType type; - HRESULT hr; - BSTR attr_name; - IXMLDOMNamedNodeMap* attr_map; + xmlnode *attr_node; TRACE("(%p)->(%p %p)\n", This, domAttribute, attributeNode); if (!domAttribute) { return E_INVALIDARG; } - - /* check given node type to be an attribute */ - hr = IXMLDOMNode_get_nodeType((IXMLDOMNode *)domAttribute, &type); - if (hr != S_OK) return hr; - if (type != NODE_ATTRIBUTE) { + attr_node = get_node_obj((IXMLDOMNode*)domAttribute); + if (This->node.node != attr_node->node->parent) { return E_INVALIDARG; } - /* get attribute node name and reuse removeNamedItem function */ - hr = IXMLDOMAttribute_get_nodeName(domAttribute, &attr_name); - if (hr != S_OK) return hr; - hr = domelem_get_attributes(iface, &attr_map); - if (hr != S_OK) return hr; - hr = IXMLDOMNamedNodeMap_removeNamedItem(attr_map, attr_name, (IXMLDOMNode **)attributeNode); - IXMLDOMNamedNodeMap_Release(attr_map); - /* removeNameItem returns S_FALSE if not found, - * removeAttributeNode is expected to return E_INVALIDARG */ - if (hr == S_FALSE) return E_INVALIDARG; - return hr; + if (attributeNode) + { + xmlUnlinkNode(attr_node->node ); + xmldoc_add_orphan(attr_node->node->doc, attr_node->node); + *attributeNode = (IXMLDOMAttribute*)create_node(attr_node->node); + } + else { + if (xmlRemoveProp((xmlAttrPtr)attr_node->node) == -1) { + return E_INVALIDARG; + } + } + return S_OK; } static HRESULT WINAPI domelem_getElementsByTagName( -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9056
On Mon Sep 29 06:28:29 2025 +0000, Nikolay Sivov wrote:
What I mean is that removing a node likely implies that node is actually is linked already to this element, as an attribute. Every msxml3 node will have a corresponding libxml2 node linked to it. To access that you can use get_node_obj(msxmlnode)->node.node or something similar. My understanding is that such check will help with extra conditions that you have now. And yes, it's not the most straightforward thing to read, which mostly comes from an attempt to reconcile libxml2 and msxml differences. thanks for the hint, now checking against `get_node_obj(msxmlnode)->node.parent`
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9056#note_116958
Now checking if the given attribute has the element as parent. If so remove it just like the function `domelem_remove_qualified_item` does. I hope this code duplication is acceptable -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9056#note_116960
wrong formatting slip in ```suggestion:-1+0 if (attributeNode) { ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9056#note_116967
participants (2)
-
Reinhold Gschweicher -
Reinhold Gschweicher (＠neroburner)