By making it simpler do you mean removing tests?
- In that case, which subset of the conclusions above would you like to remove?
- Otherwise, are you saying that the declarative approach (`manifest_res_spec` arrays) is unreadable compared to imperative / procedural one?
I don't know, my comment is mostly based on the time it took me to read the new tests, trying to understand what they were doing, and the apparent simplicity of the fix, which basically just takes the first RT_MANIFEST resource.
The fix could be different or even more simpler if we only assume that "the first manifest resource is used."
Quoting the test descriptions above:
- If multiple manifest resources have the same ID but different languages, their precedence is resolved in a specific order (Language Netural, UI language, UI language with sublanguage netural, US English, *then* the first LCID).
- The original wine-staging patch only recognizes manifest resource IDs in the range `MINIMUM_RESERVED_MANIFEST_RESOURCE_ID - MAXIMUM_RESERVED_MANIFEST_RESOURCE_ID` inclusive, whereas Windows accepts arbitrary non-zero resource IDs in the range `0x0001 - 0xffff` inclusive. The tests demonstrate that the original staging patch isn't sufficient.
- Arbitrary non-zero LCIDs are also accepted.
- Named (as opposed to numbered) resources are ignored.
Let's try omitting each one of above.
- Omitting (1), we could have hard-coded a neutral locale for `info.Language`. - Omitting (2), we could have just re-used the staging patch. - Omitting (3), we could have hard-coded the list of accepted locales. - Omitting (4), we could have ommited the line `if (!resdir->NumberOfIdEntries) return STATUS_RESOURCE_NAME_NOT_FOUND;` and `entry = entry_base + resdir->NumberOfNamedEntries;` in the implementation part as well.
I think that all the detail about resource ordering seems to be a LdrFindResourceDirectory_U behavior, rather than something related to activation contexts.
Well, so far I assumed that leftover `todo_wine`s are harmless. Is there any reason you'd like them removed?
No, but given the number of tests I thought at first that it was a new corner case that was left unfixed. But it's actually unrelated, and imho also a sign that the tests could be made simpler.
Okay, I'll just remove the tests for the English locales. That'll remove the leftover todo_wine.