Re: [PATCH v8 0/11] MR5444: jscript: Do not allocate dispex_prop_t for indexed props.

[Gabriel Ivăncescu]

On Wed Jun  5 21:57:54 2024 +0000, Jacek Caban wrote:
> > But it was still ambiguous as I explained.
> > If you mean the "zoom" example, I'd appreciate for some details. That
> > one should already use `IHTMLCSSStyleDeclaration` interface, not
> > `IHTMLStyle*` ones.
> > But since we pass the mshtml-internal tid (called iid in args) which
> > is opaque to jscript, I don't really see why this is better than passing
> > the `func_info_t` directly? Well IMO passing the pair is already more
> > complex and requires more lookups, more args, etc, but that's just me.
> > `get_dispex_from_this` is one thing, going through the interface take
> > cares of that. For that part, you could just replace the argument in my branch.
> > But we should also somehow make sure that the hook is safe to call. By
> > binding function info to the exact object instance, we guarantee that
> > it's safe to call `impl_from_*` from hooks. If you accept any function
> > info as an input, you need to somehow validate that it's safe to call
> > it. Something like moving QI earlier would only partially mitigate that.
> > I'm open to other approaches, but unless I'm missing something, there is
> > no such validation in your current branch.
> > It seems to me that things like 1ffeaa446613 come from some variation of
> > solution to that problem. Maybe it was needed in earlier version, but it
> > shouldn't be needed with "this" being resolved on jscript side. It's no
> > longer arbitrary when it's used for a `IJSDispatchHost` call. I would
> > suggest to try removing it.
> > Yeah, I forgot to validate it in my branch. In BuiltinFuncCall,
> > something like this would work:

```c
     if(func->hook) {
         hres =
IWineJSDispatchHost_QueryInterface(&This->IWineJSDispatchHost_iface,
tid_ids[func->tid], (void**)&unk);
         if(FAILED(hres))
             return hres;
         IUnknown_Release(unk);

         hres = func->hook(This, setter ? DISPATCH_PROPERTYPUT :
DISPATCH_PROPERTYGET, dp, ret, ei, caller);
         if(hres != S_FALSE)
             return hres;
     }
```

Alternatively, we can pass around `func_info_t` instead of dispid and
tid, but still look it up like in your branch. We can obtain the dispid
and tid from `func_info_t` so that's all that's needed. I'm not sure if
that's an improvement over my approach, but I think it's definitely a
simplification over yours. What do you think?


BTW for `zoom`, the assignment should fail as it does on native (as the
test does), are you sure it's supposed to be using
`IHTMLCSSStyleDeclaration` which doesn't fail? I mean the test will
simply start failing if it picks `IHTMLCSSStyleDeclaration` because the
assignment does not throw exception anymore (when it should be!).

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5444#note_72586