[PATCH v4 2/4] windows.media.speech: Partially implement IAsyncOperation.

[Rémi Bernon]

On 4/20/22 14:24, Bernhard Kölbl wrote:
>>>
>>>    static HRESULT WINAPI async_operation_get_Completed( IAsyncOperation_IInspectable *iface,
>>>                                                         IAsyncOperationCompletedHandler_IInspectable **handler )
>>>    {
>>> -    FIXME("iface %p, handler %p stub!\n", iface, handler);
>>> -    return E_NOTIMPL;
>>> +    struct async_operation *impl = impl_from_IAsyncOperation_IInspectable(iface);
>>> +    HRESULT hr;
>>> +
>>> +    FIXME("iface %p, handler %p semi stub!\n", iface, handler);
>>> +
>>> +    if (impl->handler != HANDLER_NOT_SET && impl->status == Closed)
>>> +        hr = E_ILLEGAL_METHOD_CALL;
>>> +    else
>>> +        hr = S_OK;
>>> +
>>> +    *handler = NULL; /* Hanlder *seems* to always be NULL from the tests. */
>>> +    return hr;
>>>    }
>>>
>>>    static HRESULT WINAPI async_operation_GetResults( IAsyncOperation_IInspectable *iface, IInspectable **results )
>>>    {
>>> -    FIXME("iface %p, results %p stub!\n", iface, results);
>>> -    return E_NOTIMPL;
>>> +    /* NOTE: Despite the name, this function only returns one result! */
>>> +    struct async_operation *impl = impl_from_IAsyncOperation_IInspectable(iface);
>>> +    HRESULT hr;
>>> +
>>> +    TRACE("iface %p, results %p.\n", iface, results);
>>> +
>>> +    if (impl->status == Closed)
>>> +        hr = E_ILLEGAL_METHOD_CALL;
>>> +    else
>>> +        hr = S_OK;
>>> +
>>> +    return hr;
>>>    }
>>
>>
>> Does it really succeed if status is Started? I think that the check is
>> more likely to be if (status != Completed) instead.
>>
> 
> It's probably nearly impossible to test but, how about something like:
> 
> if (impl->status == Closed)
>      hr = E_ILLEGAL_METHOD_CALL;
> else if  (impl->status > Started)
>      hr = S_OK;
> else
>      hr = E_UNEXPECTED;
> 


Sure.


>>
>>>
>>>    static const struct IAsyncOperation_IInspectableVtbl async_operation_vtbl =
>>> @@ -159,26 +232,53 @@ static HRESULT WINAPI async_operation_info_get_Id( IAsyncInfo *iface, UINT32 *id
>>>
>>>    static HRESULT WINAPI async_operation_info_get_Status( IAsyncInfo *iface, AsyncStatus *status )
>>>    {
>>> -    FIXME("iface %p, status %p stub!\n", iface, status);
>>> -    return E_NOTIMPL;
>>> +    struct async_operation *impl = impl_from_IAsyncInfo(iface);
>>> +    HRESULT hr;
>>> +
>>> +    TRACE("iface %p, status %p.\n", iface, status);
>>> +
>>> +    if (impl->status == Closed)
>>> +        hr = E_ILLEGAL_METHOD_CALL;
>>> +    else
>>> +        hr = S_OK;
>>> +
>>> +    *status = impl->status;
>>> +    return hr;
>>>    }
>>>
>>>    static HRESULT WINAPI async_operation_info_get_ErrorCode( IAsyncInfo *iface, HRESULT *error_code )
>>>    {
>>> -    FIXME("iface %p, error_code %p stub!\n", iface, error_code);
>>> -    return E_NOTIMPL;
>>> +    struct async_operation *impl = impl_from_IAsyncInfo(iface);
>>> +    TRACE("iface %p, error_code %p.\n", iface, error_code);
>>> +
>>> +    *error_code = impl->hr;
>>> +    return S_OK;
>>>    }
>>>
>>>    static HRESULT WINAPI async_operation_info_Cancel( IAsyncInfo *iface )
>>>    {
>>> -    FIXME("iface %p stub!\n", iface);
>>> -    return E_NOTIMPL;
>>> +    struct async_operation *impl = impl_from_IAsyncInfo(iface);
>>> +    HRESULT hr;
>>> +
>>> +    TRACE("iface %p.\n", iface);
>>> +    hr = S_OK;
>>> +
>>> +    if (impl->status == Closed)
>>> +        hr = E_ILLEGAL_METHOD_CALL;
>>> +    else if (impl->status == Started)
>>> +        impl->status = Canceled;
>>> +
>>> +    return hr;
>>>    }
>>>
>>>    static HRESULT WINAPI async_operation_info_Close( IAsyncInfo *iface )
>>>    {
>>> -    FIXME("iface %p stub!\n", iface);
>>> -    return E_NOTIMPL;
>>> +    struct async_operation *impl = impl_from_IAsyncInfo(iface);
>>> +
>>> +    TRACE("iface %p.\n", iface);
>>> +
>>> +    impl->status = Closed;
>>> +    return S_OK;
>>>    }
>>>
>>>    static const struct IAsyncInfoVtbl async_operation_info_vtbl =
>>> @@ -202,11 +302,14 @@ static const struct IAsyncInfoVtbl async_operation_info_vtbl =
>>>    HRESULT async_operation_create( const GUID *iid, IAsyncOperation_IInspectable **out )
>>>    {
>>>        struct async_operation *impl;
>>> +    HRESULT hr;
>>> +
>>> +    *out = NULL;
>>>
>>>        if (!(impl = calloc(1, sizeof(*impl))))
>>>        {
>>> -        *out = NULL;
>>> -        return E_OUTOFMEMORY;
>>> +        hr = E_OUTOFMEMORY;
>>> +        goto error;
>>>        }
>>>
>>>        impl->IAsyncOperation_IInspectable_iface.lpVtbl = &async_operation_vtbl;
>>> @@ -214,7 +317,14 @@ HRESULT async_operation_create( const GUID *iid, IAsyncOperation_IInspectable **
>>>        impl->iid = iid;
>>>        impl->ref = 1;
>>>
>>> +    impl->handler = HANDLER_NOT_SET;
>>> +    impl->status = Closed;
>>> +
>>
>>
>> I'm thinking that for this patch, and as it's still all stubs, it'd be
>> better to set the initial status to Completed. This way put_Completed
>> would invoke the handler, and you wouldn't have to bother adding
>> Wine-specific and temporary wait timeouts.
> 
> I agree on setting it to Completed for this patch, but as mentioned
> before, the Wait-Timeouts are actually not related to this.
> 
>>
>> It may require a temporary tweak or two in the tests, like GetResults
>> which would succeed but not return any result. I think you could set it
>> to 0xdeadbeef and check that GetResults succeeded and returned a result
>> != 0xdeadbeef (or skip the result tests).
>>
> 
> I don't really understand this one:
> Do you want me to skip/remove the GetResults tests?
> I could add a test for the case of being in the Closed state,
> but for the Started state it would probably rely on races...


I mean, if you set the initial status to Completed, GetResults will 
succeed, but not return any result as there's none to return yet.

The current test only checks SUCCEEDED(hr) to decide whether it should 
skip checking the results pointer. You will need to change that to make 
sure the returned result pointer is valid.

-- 
Rémi Bernon <rbernon at codeweavers.com>