[PATCH 0/1] 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 -- 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
Nikolay Sivov (@nsivov) commented about dlls/msxml3/element.c:
+ 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; + } I suspect it should be stricter and check that attribute node is actually has "this" as a parent (currently we can't easily check that using API, so it likely should be done and xmlNode level). This also should fix your S_FALSE -> E_INVALIDARG fixup.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9056#note_116863
On Fri Sep 26 08:10:03 2025 +0000, Nikolay Sivov wrote:
I suspect it should be stricter and check that attribute node is actually has "this" as a parent (currently we can't easily check that using API, so it likely should be done and xmlNode level). This also should fix your S_FALSE -> E_INVALIDARG fixup. as this is my first real code contribution to Wine I'm a bit lost on how to move this MR forward. Could you provide me some guidance how to tackle this?
Should I add a `IXMLDOMNamedNodeMap_removeItemNode` function and call that instead of `IXMLDOMNamedNodeMap_removeNamedItem`? As alternative: for debugging I had a "for each attribute print attribute name" loop. I could re-add that and check name+value to match the attribute and somehow reimplement the "remove Attribute" code (although I failed to do that already, so I went to use `IXMLDOMNamedNodeMap_removeNamedItem` as a workaround) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9056#note_116882
On Fri Sep 26 14:16:34 2025 +0000, Reinhold Gschweicher wrote:
as this is my first real code contribution to Wine I'm a bit lost on how to move this MR forward. Could you provide me some guidance how to tackle this? Should I add a `IXMLDOMNamedNodeMap_removeItemNode` function and call that instead of `IXMLDOMNamedNodeMap_removeNamedItem`? As alternative: for debugging I had a "for each attribute print attribute name" loop. I could re-add that and check name+value to match the attribute and somehow reimplement the "remove Attribute" code (although I failed to do that already, so I went to use `IXMLDOMNamedNodeMap_removeNamedItem` as a workaround) 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.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9056#note_116884
participants (3)
-
Nikolay Sivov (＠nsivov) -
Reinhold Gschweicher
-
Reinhold Gschweicher (＠neroburner)