Re: [PATCH v2 1/7] wmp: Add IWMPPlayer interface

[Jacek Caban]

Hi Anton,

On 02/06/2018 06:20 PM, Anton Romanov wrote:
> Jacek,
>
> On Tue, Feb 6, 2018 at 8:34 AM, Jacek Caban  wrote:
> > Hi Anton,
> >
> > On 02/02/2018 07:00 AM, Anton Romanov wrote:
> > >      TRACE("(%p)->(IID_IWMPPlayer4 %p)\n", This, ppv);
> > >      *ppv = &This->IWMPPlayer4_iface;
> > > - }else if(IsEqualGUID(riid, &IID_IWMPPlayer)) {
> > > -
> > >    TRACE("(%p)->(IID_IWMPPlayer %p)\n", This, ppv);
> > > -
> > >    *ppv = &This->IWMPPlayer4_iface;
> >
> > You can't do that. IWMPPlayer4 doesn't inherit from IWMPPlayer, so it's
> > not the same interface. Also a short test (just calling QueryInterface()
> > and checking the result) would be nice.
>
> Thanks for feedback.
> Have you had a chance to look at other patches in this series?
> I'm just unsure should I address something else as well before
> submitting new version.

I had a quick look, but didn't look closely yet. I noticed that there
are no tests, while at least a bit of them would be nice. Here are some
thoughts:

- patch 3: It seems like IWMPControls should be implemented in a
separated object. With your implementation, QueryInterface will not do
the right thing (it will never return IWMPControls, but when called on
IWMPControls from get_controls(), it will find other WindowsMediaPlayer
interfaces). Also we usually keep function implementations in the same
order as declared in IDL file.

- patch 4 worries me a bit. It misses license header and has comments
similar to PSDK version. Note that copying PSDK headers is not allowed.

- patch 5 and 6 has similar problems to patch 3.

In general, I'd suggest to resend the first patch or two and wait with
others until this one is merged. It's often a case that feedback from
the first patch will be important for others as well.

Thanks,
Jacek