Re: [2/2]wmvcore: add WMMetadataEditor stub implementation - Try 3

[Dmitry Timoshkov]

Jeff Latimer  wrote:
> +HRESULT WINAPI WMCreateEditor_close(IWMMetadataEditor *iface)
> +{
> - WMMetadataEditor *This = impl_from_IWMMetadataEditor(iface);
> - FIXME("(%p)\n", iface);
> - heap_free(This);
> - return S_OK;
>
> +}

Why not pass 'This' to  WMCreateEditor_close() directly? Why a FIXME?
There is still a stray empty line after WMCreateEditor().

[2/2]wmvcore: add WMMetadataEditor stub implementation - Try 3

[Qian Hong]

Hi Jeff,

On Sat, Apr 12, 2014 at 11:57 AM, Dmitry Timoshkov  wrote:
> Jeff Latimer  wrote:
> > +HRESULT WINAPI WMCreateEditor_close(IWMMetadataEditor *iface)
> > +{
> > - WMMetadataEditor *This = impl_from_IWMMetadataEditor(iface);
> > - FIXME("(%p)\n", iface);
> > - heap_free(This);
> > - return S_OK;
> >
> > +}
>
> Why not pass 'This' to  WMCreateEditor_close() directly? Why a FIXME?
> There is still a stray empty line after WMCreateEditor().

I think it is correct to pass 'iface' to WMCreateEditor_close, which
is a public interface:
http://msdn.microsoft.com/en-us/library/windows/desktop/dd757235(v=vs.85).as...

However, I doubt it is correct to free the object in either ::Close()
or ::Flush(), do you have any evidence/testcase showing that the
current implementation is correct? It seems we should free the object
in ::Release() but not in ::Close() and ::Flush()

[2/2]wmvcore: add WMMetadataEditor stub implementation - Try 3

[Dmitry Timoshkov]

Qian Hong  wrote:
> > > +HRESULT WINAPI WMCreateEditor_close(IWMMetadataEditor *iface)
> > > +{
> > > - WMMetadataEditor *This = impl_from_IWMMetadataEditor(iface);
> > > - FIXME("(%p)\n", iface);
> > > - heap_free(This);
> > > - return S_OK;
> > >
> > > +}
> >
> > Why not pass 'This' to  WMCreateEditor_close() directly? Why a FIXME?
> > There is still a stray empty line after WMCreateEditor().
>
> I think it is correct to pass 'iface' to WMCreateEditor_close, which
> is a public interface:
> http://msdn.microsoft.com/en-us/library/windows/desktop/dd757235(v=vs.85).as...

Well, I had an impression that it's just an internal helper, thanks
for the clarification. In that case freeing the implementation looks
wrong.