[PATCH 2/3] msscript.ocx: Implement IScriptControl::Run.

[Gabriel Ivăncescu]

Hi Jacek,

Thanks for the review.

On 9/27/19 5:15 PM, Jacek Caban wrote:
> Hi Gabriel,
> 
> On 9/26/19 4:43 PM, Gabriel Ivăncescu wrote:
>>   static HRESULT WINAPI ScriptControl_Run(IScriptControl *iface, BSTR 
>> procedure_name, SAFEARRAY **parameters, VARIANT *res)
>>   {
>>       ScriptControl *This = impl_from_IScriptControl(iface);
>> -    FIXME("(%p)->(%s %p %p)\n", This, debugstr_w(procedure_name), 
>> parameters, res);
>> -    return E_NOTIMPL;
>> +    IDispatchEx *dispex;
>> +    IDispatch *disp;
>> +    SAFEARRAY *sa;
>> +    DISPPARAMS dp;
>> +    DISPID dispid;
>> +    HRESULT hr;
>> +    UINT i = 0;
>> +
>> +    TRACE("(%p)->(%s %p %p)\n", This, debugstr_w(procedure_name), 
>> parameters, res);
>> +
>> +    if (!parameters || !res) return E_POINTER;
>> +    if (!(sa = *parameters)) return E_POINTER;
>> +
>> +    if (sa->cDims == 0) return DISP_E_BADINDEX;
> 
> 
> Shouldn't it be '!= 1'? Are cDims > 1 expected to work?
> 
> 

Yeah, they're supposed to "work", Windows seems to just ignore the other 
dimensions and only read the first one, without giving errors (I did add 
tests for it, so no problem there).

>> +    if (!(sa->fFeatures & FADF_VARIANT)) return DISP_E_BADVARTYPE;
>> +
>> +    hr = start_script(This);
>> +    if (FAILED(hr)) return hr;
>> +
>> +    dp.cArgs = sa->rgsabound[0].cElements;
>> +    dp.rgdispidNamedArgs = NULL;
>> +    dp.cNamedArgs = 0;
>> +
>> +    dp.rgvarg = heap_alloc(dp.cArgs * sizeof(*dp.rgvarg));
>> +    if (!dp.rgvarg) return E_OUTOFMEMORY;
>> +
>> +    hr = SafeArrayLock(sa);
>> +    if (FAILED(hr)) goto err;
> 
> 
> Error case will try to clean array elements that are not yet initialized.
> 
> 

It counts down from 'i' (wherever it stopped at), not the entire array, 
so it should be safe, unless I'm missing something?

>> +
>> +    for (i = 0; i < dp.cArgs; i++)
>> +    {
>> +        /* The DISPPARAMS are stored in reverse order */
>> +        VARIANT *src = (VARIANT*)((char*)(sa->pvData) + (dp.cArgs - i 
>> - 1) * sa->cbElements);
>> +
>> +        V_VT(&dp.rgvarg[i]) = VT_EMPTY;
>> +        hr = VariantCopy(&dp.rgvarg[i], src);
> 
> 
> Do we really need VariantCopy here? I think that simple assignment 
> should be enough in this case would would simplify the code.
> 
> 

Actually, I'm not sure. :-)

I'll do some more tests with this and see what happens.