On Tue Apr 16 15:10:32 2024 +0000, Jacek Caban wrote:
Well this MR technically isn't about that, is it?
I think it should, see my earlier comments.
And having something like this is clearly the most "extensible" since
it allows overriding every prop's behavior. There are many solutions, we're looking for the one that fits our needs best. I can't review that aspect in its current form and I don't share your confidence about it.
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".