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?