[PATCH v4 2/4] windows.media.speech: Partially implement IAsyncOperation.
Bernhard Kölbl
besentv at gmail.com
Wed Apr 20 07:24:48 CDT 2022
Am Di., 19. Apr. 2022 um 14:54 Uhr schrieb Rémi Bernon
<rbernon at codeweavers.com>:
>
> On 4/18/22 23:42, Bernhard Kölbl wrote:
> > Signed-off-by: Bernhard Kölbl <besentv at 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
More information about the wine-devel
mailing list