On Mon Aug 4 17:40:18 2025 +0000, Jacek Caban wrote:
this uses the existing infrastructure to keep track of the attributes
in a collection (and so, associate DISPIDs for them, etc), since it also matches native (same DISPID broken behavior when removing attrs) and we already have the code for it. While I don't think we need full compatibility with legacy-in-compat attributes right now, we do know that the collection exposes different sets of attributes depending on how it's queried. That means we'd need two separate sets to handle it. Your patch replaces the existing set instead. IMO, the straightforward solution would be to use Gecko for the compat set and leave the existing architecture for the legacy mess. Overall, the patch feels like a mix of various approaches. For example, the name `find_attr_in_list_by_nsattr` doesn’t make sense, it refers to a “list” from the “elem”, but `elem` is an unused argument now. More importantly, the refactoring doesn't add clear value at this stage and just makes things more complicated than they need to be. I still don't see why we can't start with just thin wrapper for the collection, so I gave it a try myself. What do you think about something like this? https://gitlab.winehq.org/jacek/wine/-/commits/html-attr
Sorry about the name, I forgot to rename it from my previous attempt which also looked it up in the list (for legacy attributes mixed in the collection).
The reason I went with my approach instead of a thin wrapper is mostly because it re-uses the existing infrastructure, so that whenever we implement new methods (as your branch does, btw), we won't have to add `if(This->dom_attr)` or checking for IE9 code paths everywhere and deal with all that entails. It's also subtly wrong as in you're not actually testing the COM methods' behavior itself.
For instance, it seems you're using new interface methods to separate legacy/IE9 nodes, but are you sure it's how it's supposed to work? getAttributeNode has changed behavior between interfaces, true, but that also applies to legacy attribute nodes, as well as IE9 nodes. IHTMLElement4's getAttributeNode enumerates "unspecified" builtins in both modes (though it's a FIXME even in my patches for IE9, that's fine), so does IHTMLElement6's not enumerate them in both modes.
In other words, your branch will fail the dom.c attr node tests I added (which checks behavior of the different methods). Sure, that's a simple fix for legacy attrs, but at that point we're duplicating code paths again. And if we ever implement IHTMLElement4's getAttributeNode for IE9 attrs too, it will also require a different code path yet again. (contrast this with my patch which just uses a flag "specified_only")
Another example is test_attr_collection for example in dom.c, if you run a limited version of it in IE9 mode (like in my patches) it will fail some tests since it uses the "older" interface methods, which in your case assume it's legacy collection/nodes, even though they're not.
What I'm trying to say is I don't see where native draws such a "big" or hard line between the COM methods?
That's why I'm not sure if it's a good idea to "separate" implementations based on the interface method. Note that it's also probably the reason it doesn't look like your branch has too many code paths in every method, because you've separated them by the COM method, so instead of having 2 code paths in every method, you now have a separate code path in each method. But that's not correct, in native it seems those COM methods affect both type of attr nodes (legacy/IE9) equally.
So while I think it may seem simpler because of the separation, it's kind of obviously wrong compared to native. And the problem is, it can't be used as a "starting block" either because it will have to be rewritten if that will be a problem at some point.
Lastly, the other thing I'm worried about is because of how you separated the impls, it will be very hard to debug if an app depends on native's behavior at any of those points. In my patches it was easy to test (and in one code path) and I just added FIXMEs where applicable, which will make it obvious (there's 3 of them actually: placing legacy attrs into IE9 collection, enumerating non-specified builtins, and obtaining non-specified builtins, in IE9 modes). You can also mix and match attribute nodes in native in a collection (either legacy or IE9 collection)—and while it's obviously a FIXME even in my patches, at least it does give an idea that native doesn't draw such a hard line between them.
I don't mind a simpler implementation don't get me wrong, but the problem with separate method impls like in your branch is that I can't see how to expand on it to make it more correct in the future. Wouldn't that be a problem?