Am Di., 19. Apr. 2022 um 14:54 Uhr schrieb Rémi Bernon <rbernon(a)codeweavers.com>:
On 4/18/22 23:42, Bernhard Kölbl wrote:
Signed-off-by: Bernhard Kölbl <besentv(a)gmail.com> --- dlls/windows.media.speech/async.c | 142 ++++++++++++++++++++--- dlls/windows.media.speech/tests/speech.c | 88 +++++++------- 2 files changed, 172 insertions(+), 58 deletions(-)
diff --git a/dlls/windows.media.speech/async.c b/dlls/windows.media.speech/async.c index 94d0b35d9c1..cf0da23e493 100644 --- a/dlls/windows.media.speech/async.c +++ b/dlls/windows.media.speech/async.c @@ -23,6 +23,9 @@
WINE_DEFAULT_DEBUG_CHANNEL(speech);
+#define Closed 4 +#define HANDLER_NOT_SET ((void *)~(ULONG_PTR)0) + /* * * IAsyncOperation<IInspectable*> @@ -35,6 +38,11 @@ struct async_operation IAsyncInfo IAsyncInfo_iface; const GUID *iid; LONG ref; + + IAsyncOperationCompletedHandler_IInspectable *handler; + + AsyncStatus status; + HRESULT hr; };
static inline struct async_operation *impl_from_IAsyncOperation_IInspectable(IAsyncOperation_IInspectable *iface) @@ -84,7 +92,14 @@ static ULONG WINAPI async_operation_Release( IAsyncOperation_IInspectable *iface TRACE("iface %p, ref %lu.\n", iface, ref);
if (!ref) + { + IAsyncInfo_Close(&impl->IAsyncInfo_iface); + + if (impl->handler && impl->handler != HANDLER_NOT_SET) + IAsyncOperationCompletedHandler_IInspectable_Release(impl->handler); + free(impl); + }
return ref; } @@ -110,21 +125,79 @@ 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); + HRESULT hr; + + TRACE("iface %p, handler %p.\n", iface, handler); + + if (impl->handler == HANDLER_NOT_SET) + { + /* + impl->hanlder can only be set once with async_operation_put_Completed, + so by default we set a non HANDLER_NOT_SET value, in this case NULL. + */ + impl->handler = NULL; + + if (handler) + { + if (impl->status != Started) + { + IAsyncOperation_IInspectable *operation = &impl->IAsyncOperation_IInspectable_iface; + AsyncStatus status = impl->status; + + IAsyncOperation_IInspectable_AddRef(operation); + IAsyncOperationCompletedHandler_IInspectable_AddRef(handler); + + IAsyncOperationCompletedHandler_IInspectable_Invoke(handler, operation, status); + + IAsyncOperationCompletedHandler_IInspectable_Release(handler); + IAsyncOperation_IInspectable_Release(operation); + } + } + + return S_OK; + } + else if (impl->status == Closed) + hr = E_ILLEGAL_METHOD_CALL; + else if (impl->handler != HANDLER_NOT_SET) + hr = E_ILLEGAL_DELEGATE_ASSIGNMENT; + else + hr = E_UNEXPECTED; + + return hr; }
I think the last branch cannot be taken. Then I think that something like that would be clearer:
if (impl->status == Closed) hr = E_ILLEGAL_METHOD_CALL; else if (impl->handler != HANDLER_NOT_SET) hr = E_ILLEGAL_DELEGATE_ASSIGNMENT; else if ((impl->handler = handler)) { IAsyncOperationCompletedHandler_IInspectable_AddRef(impl->handler); if (impl->status > Started) { IAsyncOperation_IInspectable *operation = &impl->IAsyncOperation_IInspectable_iface; AsyncStatus status = impl->status; impl->handler = NULL; /* prevent concurrent Invoke */
IAsyncOperationCompletedHandler_IInspectable_Invoke(handler, operation, status); IAsyncOperationCompletedHandler_IInspectable_Release(handler);
return S_OK; } }
Yes, but I'd prefer to keep the "set only once" behavior of put_Completed, which doesn't seem to be achieved by this. Also, I'd keep the last path for the sake of error handling: The WinRT C++ wrappers are very good at this and if this code was to have a serious bug, we would noctice it more easily.
I don't think you need to add a reference on "operation" around the call to Invoke.
Agreed.
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;
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... Bernhard