Re: [PATCH 2/2] wmp: Implement IConnectionPoint[Container] and add _WMPOCXEvents

[Jacek Caban]

On 02/09/2018 04:21 PM, Jacek Caban wrote:
> Hi Anton,
>
> On 02/07/2018 09:24 PM, Anton Romanov wrote:
> > - while(cConnections--) {
> > -
> >    while(This->iter < This->cp->sinks_size && !This->cp->sinks[This->iter])
> > -
> >        This->iter++;
> > -
> >    if(This->iter == This->cp->sinks_size)
> > -
> >        break;
> > -
> > -
> >    pgcd[cnt].pUnk = (IUnknown*)This->cp->sinks[This->iter];
>
> I think you're missing IUnknown_AddRef() here.
> > +void call_sink(ConnectionPoint *This, DISPID dispid, DISPPARAMS *dispparams)
> > +{
> > - DWORD i;
> > -
> > - for(i=0; i<This->sinks_size; i++) {
> > -
> >    if(This->sinks[i])
> > -
> >        IDispatch_Invoke(This->sinks[i], dispid, &IID_NULL, LOCALE_SYSTEM_DEFAULT,
> > -
> >                         DISPATCH_METHOD, dispparams, NULL, NULL, NULL);
> > - }
> >
> > +}
>
> You don't use it in this patch, so it's a dead code. Please remove it
> from this patch (and introduce it with a patch that uses it).
> > HRESULT WINAPI WMPFactory_CreateInstance(IClassFactory *iface, IUnknown *outer,
> >          REFIID riid, void **ppv)
> >  {
> > @@ -941,22 +894,25 @@ HRESULT WINAPI WMPFactory_CreateInstance(IClassFactory *iface, IUnknown *outer,
> >      wmp->IProvideClassInfo2_iface.lpVtbl = &ProvideClassInfo2Vtbl;
> >      wmp->IPersistStreamInit_iface.lpVtbl = &PersistStreamInitVtbl;
> >      wmp->IOleInPlaceObjectWindowless_iface.lpVtbl = &OleInPlaceObjectWindowlessVtbl;
> > -
> >
> > wmp->IConnectionPointContainer_iface.lpVtbl = &ConnectionPointContainerVtbl;
> > wmp->IOleControl_iface.lpVtbl = &OleControlVtbl;
> >
> > wmp->ref = 1;
> > -
> >
> > init_player_ifaces(wmp);
> > -
> > -
> >
> > hdc = GetDC(0);
> > -
> >
> > dpi_x = GetDeviceCaps(hdc, LOGPIXELSX);
> > -
> >
> > dpi_y = GetDeviceCaps(hdc, LOGPIXELSY);
> > -
> >
> > ReleaseDC(0, hdc);
> > - hres = init_player(wmp);
> > - if (hres == S_OK) {
> > -
> >    ConnectionPointContainer_Init(wmp);
> > -
> >    hdc = GetDC(0);
> > -
> >    dpi_x = GetDeviceCaps(hdc, LOGPIXELSX);
> > -
> >    dpi_y = GetDeviceCaps(hdc, LOGPIXELSY);
> > -
> >    ReleaseDC(0, hdc);
> >
> > - wmp->extent.cx = MulDiv(192, 2540, dpi_x);
> > - wmp->extent.cy = MulDiv(192, 2540, dpi_y);
> > -
> >    wmp->extent.cx = MulDiv(192, 2540, dpi_x);
> > -
> >    wmp->extent.cy = MulDiv(192, 2540, dpi_y);
> >
> > - hres = IOleObject_QueryInterface(&wmp->IOleObject_iface, riid, ppv);
> > - IOleObject_Release(&wmp->IOleObject_iface);
> > -
> >    hres = IOleObject_QueryInterface(&wmp->IOleObject_iface, riid, ppv);
> > -
> >    IOleObject_Release(&wmp->IOleObject_iface);
> > - }
> > - if(hres != S_OK)
> > -
> >    destroy_player(wmp);
> > return hres;
>
> Error handling looks wrong here. If QueryInterface() fails, you'd both
> release wmp and call destroy_wmp. Also init_player can't fail, so it
> should return void.
>
> Thanks,
> Jacek

Also events.c is missing license header.

Thanks,

Jacek