Re: [PATCH v2 2/4] windows.media.speech: Implement IInstalledVoicesStatic stub.

[Zebediah Figura (she/her)]

On 3/5/21 7:51 AM, Jacek Caban wrote:
> Hi Rémi,
>
> The series looks better now, but it has some COM problems. Also, there
> is not much to test yet, but it would still be interesting to see some
> basic creation and QueryInterface tests.
>
> On 05.03.2021 09:53, Rémi Bernon wrote:
> > Signed-off-by: Rémi Bernon
> >
> > dlls/windows.media.speech.dll/main.c | 104 +++++++++++++++++++++++++++
> >    1 file changed, 104 insertions(+)
> >
> > diff --git a/dlls/windows.media.speech.dll/main.c b/dlls/windows.media.speech.dll/main.c
> > index 909ea6db748..db57acac651 100644
> > --- a/dlls/windows.media.speech.dll/main.c
> > +++ b/dlls/windows.media.speech.dll/main.c
> > @@ -29,7 +29,10 @@
> >    #include "initguid.h"
> >    #include "activation.h"
> >
> > +#define WIDL_using_Windows_Foundation
> > +#define WIDL_using_Windows_Foundation_Collections
> >    #include "windows.foundation.h"
> > +#define WIDL_using_Windows_Media_SpeechSynthesis
> >    #include "windows.media.speechsynthesis.h"
> >
> > WINE_DEFAULT_DEBUG_CHANNEL(speech);
> > @@ -46,6 +49,7 @@ static const char *debugstr_hstring(HSTRING hstr)
> >    struct windows_media_speech
> >    {
> >        IActivationFactory IActivationFactory_iface;
> > - IInstalledVoicesStatic IInstalledVoicesStatic_iface;
> >   LONG ref;
> > };
> >
> > @@ -54,6 +58,98 @@ static inline struct windows_media_speech *impl_from_IActivationFactory(IActivat
> >        return CONTAINING_RECORD(iface, struct windows_media_speech, IActivationFactory_iface);
> >    }
> >
> > +static inline struct windows_media_speech *impl_from_IInstalledVoicesStatic(IInstalledVoicesStatic *iface)
> > +{
> > - return CONTAINING_RECORD(iface, struct windows_media_speech, IInstalledVoicesStatic_iface);
> >
> > +}
> > -
> >
> > +static HRESULT STDMETHODCALLTYPE installed_voices_static_QueryInterface(
> > -
> >    IInstalledVoicesStatic *iface, REFIID iid, void **out)
> >
> > +{
> > - TRACE("iface %p, iid %s, out %p stub!\n", iface, debugstr_guid(iid), out);
> > -
> > - if (IsEqualGUID(iid, &IID_IUnknown) ||
> > -
> >    IsEqualGUID(iid, &IID_IAgileObject) ||
> > -
> >    IsEqualGUID(iid, &IID_IInspectable) ||
> > -
> >    IsEqualGUID(iid, &IID_IInstalledVoicesStatic))
> > - {
> > -
> >    IUnknown_AddRef(iface);
> > -
> >    *out = iface;
> > -
> >    return S_OK;
> > - }
> > -
> > - WARN("%s not implemented, returning E_NOINTERFACE.\n", debugstr_guid(iid));
> > - *out = NULL;
> > - return E_NOINTERFACE;
> >
> > +}
>
> It's missing IActivationFactrory interface.
> > @@ -69,6 +165,13 @@ static HRESULT STDMETHODCALLTYPE windows_media_speech_QueryInterface(
> >            return S_OK;
> >        }
> > - if (IsEqualGUID(iid, &IID_IInstalledVoicesStatic))
> > - {
> > -
> >    IUnknown_AddRef(iface);
> > -
> >    *out = &impl->IInstalledVoicesStatic_iface;
> > -
> >    return S_OK;
> > - }
>
> It's missing IID_IAgileObject interface now and it generally just
> duplicates installed_voices_static_QueryInterface. The usual practice is
> to have one QueryInterface implementation per object and forward other
> calls. Same for AddRef and Release (at least in case where we have
> actual ref counting).
>
> Note static objects don't need ref counting at all, because you don't
> have the destructor anyway. If there is a reason to have reference
> counting, then patch 3 is missing AddRef call.
>
> Static object also don't really need a structs like
> windows_media_speech, but it's fine to have it. However,
> IVectorView_VoiceInformation doesn't really belong to
> windows_media_speech object as it looks like a separate object.

As far as I can tell, it's not a mutable interface and therefore doesn't 
need to be made into a separate object, right?
> Thanks,
>
> Jacek