Re: msxml3: Add custom handling for DISPID_VALUE for node map

[Jacek Caban]

On 12/19/12 08:24, Nikolay Sivov wrote:
> Add custom handling for DISPID_VALUE for node map

If this has to be done for all IDispatchEx objects with DISPID_VALUE, it
may be worth considering changing dispex.c. Something like:

if(id == DISPID_VALUE && wFlags == DISPATCH_METHOD)
    wFlags |= DISPATCH_PROPERTYGET;

before ITypeInfo::Invoke call should do the trick and replace both this
patch and already committed node list patch.

Cheers,
Jacek

msxml3: Add custom handling for DISPID_VALUE for node map

[Nikolay Sivov]

On 12/19/2012 17:22, Jacek Caban wrote:
> On 12/19/12 08:24, Nikolay Sivov wrote:
> > Add custom handling for DISPID_VALUE for node map
> > If this has to be done for all IDispatchEx objects with DISPID_VALUE, it
> > may be worth considering changing dispex.c. Something like:
>
> if(id == DISPID_VALUE && wFlags == DISPATCH_METHOD)
>      wFlags |= DISPATCH_PROPERTYGET;
>
> before ITypeInfo::Invoke call should do the trick and replace both this
> patch and already committed node list patch.
> Sure, I thought about that too. First one was to fix an open bug, and to 
> do this for all interfaces I'll need tests for everything with 
> DISPID_VALUE, but you're right node map patch could wait for now.
>
> Cheers,
> Jacek

msxml3: Add custom handling for DISPID_VALUE for node map

[Nikolay Sivov]

On 12/19/2012 17:22, Jacek Caban wrote:
> On 12/19/12 08:24, Nikolay Sivov wrote:
> > Add custom handling for DISPID_VALUE for node map
> > If this has to be done for all IDispatchEx objects with DISPID_VALUE, it
> > may be worth considering changing dispex.c. Something like:
>
> if(id == DISPID_VALUE && wFlags == DISPATCH_METHOD)
>      wFlags |= DISPATCH_PROPERTYGET;
>
> before ITypeInfo::Invoke call should do the trick and replace both this
> patch and already committed node list patch.
> So I did more testing and it doesn't work like that for all DISPID_VALUE 
> methods, what a surprise:

http://www.winehq.org/pipermail/wine-patches/2012-December/120928.html

Do you think it's still better to centrally change it in dispex.c and 
wrongly accept DISPATCH_METHOD when we shouldn't?
> Cheers,
> Jacek

msxml3: Add custom handling for DISPID_VALUE for node map

[Jacek Caban]

Hi Nikolay,

On 12/20/12 11:29, Nikolay Sivov wrote:
> On 12/19/2012 17:22, Jacek Caban wrote:
> > On 12/19/12 08:24, Nikolay Sivov wrote:
> > > Add custom handling for DISPID_VALUE for node map
> > > If this has to be done for all IDispatchEx objects with DISPID_VALUE, it
> > > may be worth considering changing dispex.c. Something like:
> >
> > if(id == DISPID_VALUE && wFlags == DISPATCH_METHOD)
> >      wFlags |= DISPATCH_PROPERTYGET;
> >
> > before ITypeInfo::Invoke call should do the trick and replace both this
> > patch and already committed node list patch.
> > So I did more testing and it doesn't work like that for all
> > DISPID_VALUE methods, what a surprise:
>
> http://www.winehq.org/pipermail/wine-patches/2012-December/120928.html
>
> Do you think it's still better to centrally change it in dispex.c and
> wrongly accept DISPATCH_METHOD when we shouldn't?

I'm not really convinced by those tests, here is why:

+
+    hr = IXMLDOMParseError_QueryInterface(error, &IID_IDispatchEx, (void**)&dispex);
+    ok(hr == S_OK, "got 0x%08x\n", hr);
+
+    V_VT(&arg) = VT_I4;
+    V_I4(&arg) = 0;
+    dispparams.cArgs = 1;
+    dispparams.cNamedArgs = 0;
+    dispparams.rgdispidNamedArgs = NULL;
+    dispparams.rgvarg = &arg;
+
+    V_VT(&ret) = VT_EMPTY;
+    V_DISPATCH(&ret) = (void*)0x1;
+    hr = IDispatchEx_Invoke(dispex, DISPID_VALUE, &IID_NULL, 0, DISPATCH_METHOD, &dispparams, &ret, NULL, NULL);
+    ok(hr == DISP_E_MEMBERNOTFOUND, "got 0x%08x\n", hr);
+    ok(V_VT(&ret) == VT_EMPTY, "got %d\n", V_VT(&ret));
+    ok(V_DISPATCH(&ret) == (void*)0x1, "got %p\n", V_DISPATCH(&ret));


IXMLDOMParseError's DISPID_VALUE is errorCode property, which doesn't take arguments (in C world, it's no argument except retval). You're passing one argument, so the call should return error (I'd expect DISP_E_BADPARAMCOUNT, but well...).

+    hr = IXMLDOMSchemaCollection_QueryInterface(cache, &IID_IDispatchEx, (void**)&dispex);
+    ok(hr == S_OK, "got 0x%08x\n", hr);
+
+    V_VT(&arg) = VT_I4;
+    V_I4(&arg) = 0;
+    dispparams.cArgs = 1;
+    dispparams.cNamedArgs = 0;
+    dispparams.rgdispidNamedArgs = NULL;
+    dispparams.rgvarg = &arg;
+
+    V_VT(&ret) = VT_EMPTY;
+    V_DISPATCH(&ret) = (void*)0x1;
+    hr = IDispatchEx_Invoke(dispex, DISPID_VALUE, &IID_NULL, 0, DISPATCH_METHOD, &dispparams, &ret, NULL, NULL);
+    ok(hr == DISP_E_MEMBERNOTFOUND, "got 0x%08x\n", hr);
+    ok(V_VT(&ret) == VT_EMPTY, "got %d\n", V_VT(&ret));
+    ok(V_DISPATCH(&ret) == (void*)0x1, "got %p\n", V_DISPATCH(&ret));
  
This one is indeed showing different behaviour, although the test uses an empty collection, so the call probably shouldn't work anyway.

I'm not saying that there should be no custom implementations, but AFAICS majority of objects can be fixed by simple IDispatchEx implementation modification and if there are cases that still need fixing, then we can fall back to custom implementation.

Cheers,
Jacek