[PATCH v3 0/3] 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 -- v3: fix formatting of if clause 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
From: Reinhold Gschweicher <pyro4hell+winehq(a)gmail.com> --- dlls/msxml3/element.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/dlls/msxml3/element.c b/dlls/msxml3/element.c index c311e327212..c6c33296299 100644 --- a/dlls/msxml3/element.c +++ b/dlls/msxml3/element.c @@ -1503,8 +1503,7 @@ static HRESULT WINAPI domelem_removeAttributeNode( return E_INVALIDARG; } - if (attributeNode) - { + if (attributeNode) { xmlUnlinkNode(attr_node->node ); xmldoc_add_orphan(attr_node->node->doc, attr_node->node); *attributeNode = (IXMLDOMAttribute*)create_node(attr_node->node); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9056
Squash all your patches and more (new) tests probably -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9056#note_117213
Nikolay Sivov (@nsivov) commented about dlls/msxml3/element.c:
+ + TRACE("(%p)->(%p %p)\n", This, domAttribute, attributeNode); + + if (!domAttribute) { + return E_INVALIDARG; + } + attr_node = get_node_obj((IXMLDOMNode*)domAttribute); + if (This->node.node != attr_node->node->parent) { + return E_INVALIDARG; + } + + if (attributeNode) { + xmlUnlinkNode(attr_node->node ); + xmldoc_add_orphan(attr_node->node->doc, attr_node->node); + *attributeNode = (IXMLDOMAttribute*)create_node(attr_node->node); + } I don't think we have a test for null out argument, for when attribute still exists.
Please make some cosmetic changes, so we don't have to reiterate just on that: - for single line blocks, there is no need for braces; - when there are braces, let's have opening one on a new line. As mentioned by previous comment, please squash your changes into a single commit, then do "git push --force" to the same branch to overwrite. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9056#note_117413
On Thu Oct 2 12:49:29 2025 +0000, Vijay Kiran Kamuju wrote:
Squash all your patches and more (new) tests probably will do. I normally do squash at the end of the MR, when all is sorted out. So I can revert a useless change I did or similar. Will squash-push in future. Thanks for the hint!
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9056#note_117421
I don't think we have a test for null out argument, for when attribute still exists.
should I add a new test here or in a separate MR? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9056#note_117422
participants (4)
-
Nikolay Sivov (＠nsivov) -
Reinhold Gschweicher
-
Reinhold Gschweicher (＠neroburner)
-
Vijay Kiran Kamuju (＠infyquest)