[PATCH 5/6] mshtml: Use an event mask and list in XMLHttpReqEventListener.

[Gabriel Ivăncescu]

On 24/05/2022 18:35, Jacek Caban wrote:
> Hi Gabriel,
> 
> On 5/23/22 19:41, Gabriel Ivăncescu wrote:
>> On 23/05/2022 19:09, Jacek Caban wrote:
>>> Hi Gabriel,
>>>
>>> On 5/23/22 17:22, Gabriel Ivăncescu wrote:
>>>> Instead of hardcoding each event, since there will be plenty more.
>>>
>>>
>>> What's an example of those missing ones?
>>>
>>
>> It's not in this patch series, but IE10+ modes will additionally need:
>>
>> abort, error, loadstart, loadend, progress
>>
>>>> Signed-off-by: Gabriel Ivăncescu<gabrielopcode at gmail.com>
>>>> ---
>>>>   dlls/mshtml/xmlhttprequest.c | 47 
>>>> +++++++++++++++++++-----------------
>>>>   1 file changed, 25 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/dlls/mshtml/xmlhttprequest.c 
>>>> b/dlls/mshtml/xmlhttprequest.c
>>>> index b9f6f1f..baef36e 100644
>>>> --- a/dlls/mshtml/xmlhttprequest.c
>>>> +++ b/dlls/mshtml/xmlhttprequest.c
>>>> @@ -94,12 +94,23 @@ static HRESULT return_nscstr(nsresult nsres, 
>>>> nsACString *nscstr, BSTR *p)
>>>>       return S_OK;
>>>>   }
>>>> +#define EVENTS_LIST \
>>>> +    X(readystatechange) \
>>>> +    X(load)
>>>> +
>>>> +#undef X
>>>> +#define X(event) EVENT_##event,
>>>> +enum { EVENTS_LIST };
>>>> +#undef X
>>>> +#define X(event) L"" #event,
>>>> +static const WCHAR *events[] = { EVENTS_LIST };
>>>> +#undef X
>>>
>>>
>>> You could avoid all those preprocessor directives by simply using an 
>>> array of structs.
>>>
>>
>> The problem is the first one is an enum, while second one is actual 
>> data, so I can't use a struct to unify this. I wanted to keep it 
>> easily in sync, but I can of course separate them without preprocessor 
>> if you prefer.
> 
> 
> It doesn't sound too bad and you may always use something like C_ASSERT 
> to make sure we don't forget to update one of its parts.
> 
> 
> Generally, this could use more generic event code. Reasons for 
> XMLHttpReqEventListener existance are mostly historical, we could for 
> example have a more generic version of Gecko event listener that could 
> be also used instead of htmlevent_listener. I left it separated while 
> changing the code for IE9+ events mostly because it was simple enough. 
> If this is going to need more duplication in the future, I think we 
> should revisit that. That said, I'm generally fine with the approach for 
> now, but I wouldn't plan on extending that much further in this direction.
> 
> 
> Jacek
> 

Hi Jacek,

Well with the event mask there will be almost no duplication at all 
(except the switch() case in the bind function, but that could probably 
be looked up in a table too, so all the event data is in the table). The 
point of the mask is to iterate through it, after all, and avoid any 
duplication there.

This patch is a good example of that; adding additional events requires 
no extra code except extending the list (macro now) or table (revised) 
with the new event. Is that not simple enough?

I don't know what else "extending much further" in this direction means 
though—do you have some examples of the more generic version of Gecko 
event listener you mentioned? Or how would it look like? I honestly 
don't know how else this could be handled, since I learned the code as I 
worked on it. :-)

IMO I think the mask with table is as simple as it gets when it comes to 
extending it: just add new entries to the table.

Thanks,
Gabriel