On Wed Apr 17 19:23:20 2024 +0000, Gabriel Ivăncescu wrote:
I pushed new version with a small fix for arguments. I also rewrote the Typed Arrays and jscript proxy patches to make use of this. It's on my wine's branch `jscript-proxies`. The patches of note are cb615b32c4cd8cd9afed43bf1882df9139e6d365 and be63c5738979878a154b65e142983c9802feef4f. Note that they aren't split yet, since I keep rewriting them for now, it would make it too much wasted effort. But I don't think a full review of them is necessary, just the parts handling the props, right? Some things to note:
- I got rid of PropDefineOverride and the "this" pointer for PropInvoke.
- With respect to props, there are no special cases anymore for proxies,
it's all handled in the props methods in the vtbl (from this MR).
- Only one new method had to be added, `prop_fixup`.
I hope this shows that the current MR's approach is solid enough for proxies later. A couple of things to note while reviewing, if you're wondering why it looks "hackish":
- async props in localStorage etc are weird as hell. Defining an
accessor bypasses the override, and stores a normal js prop, and getOwnPropertyDescriptor also retrieves the normal js prop "underneath" the one set by setItem, if available. But, that only applies to getOwnPropertyDescriptor. Retrieving the prop via normal getter looks at getItem first. Look at the "storage" tests starting in dom.js after line 577 (on my branch).
- Other things making use of the "fixup" are the doc/window with props
being from element lookups (or global props for window). But note that these are different from localStorage because they cannot *add* a new one, while localStorage can (e.g. localStorage.foobar = "1234" defining a new prop, but that can't happen on doc or window with the element lookup, it defines an actual jscript prop in those cases). They also don't override an existing js prop at all, so they only make use of the "fixup" but not the "override".
The interface for functions looks suspicious and I think that's a result of special-casing them in a wrong place. If they are indeed JS functions, `dispex.c` should be able to just call it like any other function (and `proxy_disp_call` should not be needed). Then something like `ProxyFunction_call` could just call a `IJSDispatchHost` method of the proper object if it implements the required interface.
This `prop_fixup` sounds solvable by just using different override strategy in relevant code paths. In one case we may interpret such case differently than in the other, I'm sure there is a cleaner way for that.
Did you consider allowing enums without callbacks to jscript? Ie. `IJSDispatchHost` enum could have an ID parameter and return the next one like `IDispatchEx` does? jscript could still additionally skip over properties if needed.