[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 <jacek at codeweavers.com> 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