Re: [PATCH 4/6] windows.media.speech: Implement handler for IAsyncOperation.

[Rémi Bernon]

On 3/31/22 18:17, Bernhard Kölbl wrote:
> Signed-off-by: Bernhard Kölbl
>
> dlls/windows.media.speech/async.c        | 40 ++++++++++++++++++++----
>   dlls/windows.media.speech/private.h      |  1 +
>   dlls/windows.media.speech/recognizer.c   | 10 +++++-
>   dlls/windows.media.speech/tests/speech.c | 25 +++++++--------
>   4 files changed, 55 insertions(+), 21 deletions(-)
>
> diff --git a/dlls/windows.media.speech/async.c b/dlls/windows.media.speech/async.c
> index 5b74ec60be1..767b4a3a4e4 100644
> --- a/dlls/windows.media.speech/async.c
> +++ b/dlls/windows.media.speech/async.c
> @@ -35,6 +35,10 @@ struct async_operation
>       IAsyncInfo IAsyncInfo_iface;
>       const GUID *iid;
>       LONG ref;
> -
> -
>
> IAsyncOperationCompletedHandler_IInspectable *handler;
> -
>
> BOOLEAN handler_set;
> -
>
> AsyncStatus status;
> };
>
> static inline struct async_operation *impl_from_IAsyncOperation_IInspectable(IAsyncOperation_IInspectable *iface)
>
> @@ -110,15 +114,26 @@ static HRESULT WINAPI async_operation_GetTrustLevel( IAsyncOperation_IInspectabl
>   static HRESULT WINAPI async_operation_put_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);
> -
> -
>
> TRACE("iface %p, handler %p.\n", iface, handler);
> -
> -
>
> if (impl->handler_set) return E_ILLEGAL_DELEGATE_ASSIGNMENT;
> -
>
> impl->handler = handler;
> -
>
> impl->handler_set = TRUE;
> -
> -
>
> if (impl->status == Completed)
> -
>    return async_operation_notify(iface);
> -
> -
>
> return S_OK;
> }
>
> 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;
> -
>
> FIXME("iface %p, handler %p semi stub!\n", iface, handler);
> -
>
> *handler = NULL;
> -
>
> return S_OK;
> }
>
> static HRESULT WINAPI async_operation_GetResults( IAsyncOperation_IInspectable *iface, IInspectable ***results )
>
> @@ -159,8 +174,10 @@ 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);
> -
>
> TRACE("iface %p, status %p.\n", iface, status);
> -
>
> *status = impl->status;
> -
>
> return S_OK;
> }
>
> static HRESULT WINAPI async_operation_info_get_ErrorCode( IAsyncInfo *iface, HRESULT *error_code )
>
> @@ -218,3 +235,14 @@ HRESULT async_operation_create( const GUID *iid, IAsyncOperation_IInspectable **
>       TRACE("created %p\n", *out);
>       return S_OK;
>   }
> -
>
> +HRESULT async_operation_notify( IAsyncOperation_IInspectable *operation )
> +{
> - struct async_operation *impl = impl_from_IAsyncOperation_IInspectable(operation);
> -
> - impl->status = Completed;
> - if (impl->handler)
> -
>    IAsyncOperationCompletedHandler_IInspectable_Invoke(impl->handler, operation, impl->status);
> -
> - return S_OK;
>
> +}
> \ No newline at end of file

It might require a bit of reorganization of the patches, to introduce 
the result first, but I think you only need one private helper overall, 
which would be something like "async_operation_complete". I don't think 
it makes much sense to have two separate "set_result" + "notify" operations.

It would also be interesting to add tests for the IAsyncInfo and results 
before setting a completion handler. As far as I can tell, the async is 
actually completed by the put_Completed call, not right when it is 
created. This would make the "already completed" case in put_Completed 
go away.

Imho adding the tests first, in a separate patch, including with the 
results tests also would make things a bit more clear in term of to 
which direction the implementation should go.