On Mon Apr 22 16:02:21 2024 +0000, Gabriel Ivăncescu wrote:
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. But `proxy_disp_call` is not used for the functions, it's basically the analog for the builtin_info->call except for proxies. In fact, that's exactly where it is called from, the "call" method of JSCLASS_PROXY objects. It's for calling DISPID_VALUE mainly on objects (and constructors that aren't actual functions for instance, which is the majority of them). It is additionally used for calling PROP_PROXY props—these are not the functions, but rather calling a prop like for instance sessionStorage's props and so on. You know, like our builtins on jscript that aren't functions nor accessors. Those can be invoked too, although it will likely error out. In such case, the error is given by mshtml, which I found more extensible and cleaner so jscript doesn't have to worry about it (I don't know yet if some objects return something else, either). Plus, it's just one line of code re-using the same helper that's needed anyway… I could hardcode the error in jscript if you prefer it, but I'm not sure that buys us anything? `proxy_disp_call` is already used by the builtin_info->call anyway. If you want an example of an object that's **not** a function that still needs to be invoked: anchor element's DISPID_VALUE.
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. Sorry, I don't quite understand what you mean here. I use different cases because of how native works, to pass existing and new tests. `prop_fixup` is basically similar to the PROP_PROTREF fixup (in fact, the PROTREF fixup is implemented with it too), except it's for props that may pop into existence or pop out of existence at any point. Can you be more specific about the other strategy please? And are you sure it's going to work in all cases? EDIT: Regardless, I do believe that since it's a new method added only for proxies, this MR should be fine as it is, right? I mean the remaining methods in this MR do seem fine unless I'm missing something...?
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. Yeah, I guess that can work too, the only thing jscript is doing right now is to just populate the props (with find_prop_name), similar to fill_props. It's probably unrelated to this MR though, I don't see how it affects it.
Scratch the reference to the implementation, let me put it differently: why do you need a separate interface for functions? They are always called on "this" object, so why not use `IJSDisatchHost` of "this" object? Its implementation may get a function descriptor from the ID. The only additional thing from function descriptor that you need earlier is IID, to make sure that the object implements required interface. You could just make that a part of function description. I think we already discussed this part years ago. Things like `get_dispex_for_hook` should not be needed in the first place.
`prop_fixup` is an implementation detail (not a pretty one) that shouldn't be a part of the interface in its current form. When you need to check if the prototype is still valid, you may just query that value's description, for example.