On 21/07/2022 15:38, Jacek Caban (@jacek) wrote:
On Thu Jul 21 12:38:56 2022 +0000, Gabriel Ivăncescu wrote:
Merge request !487 https://gitlab.winehq.org/wine/wine/-/merge_requests/487 was reviewed by Jacek Caban https://gitlab.winehq.org/jacek
Jacek Caban https://gitlab.winehq.org/jacek started a new discussion on dlls/mshtml/htmlevent.c https://gitlab.winehq.org/wine/wine/-/merge_requests/487#note_4653:
2536
+unsigned size;
2537
+compat_mode_t compat_mode;
2538
+} types_table[] = {
Having a constructor here should be enough, other fields seem redundant. You could, for example, move allocation to constructors. Constructors would then call a common allocation helper, passing required info.
Initially I had all ctors just define everything themselves and allocate it, but that wasn't much of an improvement (it was too much duplicated code). I wanted to squash as much as possible into the table to keep it smaller, but a helper might work if you prefer, I'll go with it.
Jacek Caban https://gitlab.winehq.org/jacek started a new discussion on dlls/mshtml/htmlevent.c https://gitlab.winehq.org/wine/wine/-/merge_requests/487#note_4654:
320
321
+static void *DOMMouseEvent_query_interface(DOMEvent*,REFIID);
322
+static inline DOMMouseEvent *unsafe_DOMMouseEvent_from_DOMEvent(DOMEvent *event)
Do we really need that? QueryInterface() should be just fine, AFAICT.
Yeah, QI works... but it adds a ref which has to be released. I wanted to keep the call sites as simple as possible like they are now, just one line, since they're used in a *lot* of places. I guess I could use a helper (which would also check for NULL pointer first), and then Release it before returning, but IMO that's ugly. Are you sure you want to complicate the call sites instead of having this "unsafe" helper?
Handling of things like compat mode in the array looks very ad-hoc. With separated constructors, it will be a matter of conditionally forwarding the call to other constructor.
What's so "ugly" about a Release call?
I treat the array/table as "data" for each event type, so to me it makes sense to keep data about it there, for example, in what minimum compat mode it is available, its IID etc.
Currently, if it doesn't meet the compat mode requirements, it simply skips it. The constructor can't skip because once it's called, it either returns the constructed event of that type, or failure (no memory).
I guess, however, a way would be to just create a normal event in the constructor (instead of skipping it) if the compat mode is not enough, which is the same thing in practice. Is that what you meant?
Though I don't really see why it's better than keeping it in the table, especially since compat mode is last field to avoid specifying it at all except where needed, but I don't really mind either way.
As for Release call, I was talking about putting it in a helper to avoid Release-ing it everywhere it's used, which would be ugly. That was in context of not adding a Release everywhere, but if you think that's not an issue then I'll go with it.