Re: [PATCH 2/5] qedit: Stub out AMTimelineObj.

[Nikolay Sivov]

On 26.04.2016 6:09, Alex Henrie wrote:
> Cc: Andrew Eikum
> Signed-off-by: Alex Henrie
>
> dlls/qedit/Makefile.in      |   3 +-
>  dlls/qedit/qedit_private.h  |   2 +
>  dlls/qedit/tests/timeline.c |   3 -
>  dlls/qedit/timeline.c       |  10 +-
>  dlls/qedit/timelineobj.c    | 470 ++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 482 insertions(+), 6 deletions(-)
>  create mode 100644 dlls/qedit/timelineobj.c

I think it's better to add this to timeline.c.
> diff --git a/dlls/qedit/timeline.c b/dlls/qedit/timeline.c
> index 52123a7..0398f28 100644
> --- a/dlls/qedit/timeline.c
> +++ b/dlls/qedit/timeline.c
> @@ -129,8 +129,14 @@ static HRESULT WINAPI Timeline_IAMTimeline_CreateEmptyNode(IAMTimeline *iface, I
>                                                             TIMELINE_MAJOR_TYPE type)
>  {
>      TimelineImpl *This = impl_from_IAMTimeline(iface);
> - FIXME("(%p)->(%p,%04x): not implemented!\n", This, obj, type);
> - return E_NOTIMPL;
> - HRESULT hr;
> -
> - TRACE("(%p)->(%p,%04x)\n", This, obj, type);
> -
> - hr = AMTimelineObj_create(type, obj);
> - if (FAILED(hr)) return hr;
> -
> - return S_OK;
>
> }

You can simply 'return AMTimelineObj_create...;'.
> -
>
> +#include <assert.h>

This one is not used.
> +static ULONG WINAPI TimelineObj_inner_Release(IUnknown *iface)
> +{
> - TimelineObjImpl *This = impl_from_IUnknown(iface);
> - ULONG ref = InterlockedDecrement(&This->ref);
> -
> - TRACE("(%p) new ref = %u\n", This, ref);
> -
> - if (ref == 0)
> - {
> -
>    CoTaskMemFree(This);
> -
>    return 0;
> - }
> -
> - return ref;
>
> +}

Extra 'return 0;' does not add anything.
> +HRESULT AMTimelineObj_create(TIMELINE_MAJOR_TYPE type, IAMTimelineObj **ppv)
> +{
> - TimelineObjImpl* obj = NULL;

No need to initialize it.
> -
> - TRACE("(%d,%p)\n", type, ppv);
> -
> - switch (type)
> - {
> -
>    case TIMELINE_MAJOR_TYPE_COMPOSITE:
> -
>    case TIMELINE_MAJOR_TYPE_TRACK:
> -
>    case TIMELINE_MAJOR_TYPE_SOURCE:
> -
>    case TIMELINE_MAJOR_TYPE_TRANSITION:
> -
>    case TIMELINE_MAJOR_TYPE_EFFECT:
> -
>    case TIMELINE_MAJOR_TYPE_GROUP:
> -
>        break;
> -
>    default:
> -
>        return E_INVALIDARG;
> - }

What happens to returned pointer on type error?
> -
> - obj = CoTaskMemAlloc(sizeof(TimelineObjImpl));
> - if (NULL == obj) {
> -
>    *ppv = NULL;
> -
>    return E_OUTOFMEMORY;
> - }
> - ZeroMemory(obj, sizeof(TimelineObjImpl));
> -
> - obj->ref = 1;
> - obj->IUnknown_inner.lpVtbl = &timelineobj_vtbl;
> - obj->IAMTimelineObj_iface.lpVtbl = &IAMTimelineObj_VTable;
> -
> - *ppv = &obj->IAMTimelineObj_iface;
> - return S_OK;
>
> +}

memset is not useful, you're setting all the fields anyway. Also, why do
you need inner IUnknown for this object?

[PATCH 2/5] qedit: Stub out AMTimelineObj.

[Alex Henrie]

2016-04-25 22:17 GMT-06:00 Nikolay Sivov :
> On 26.04.2016 6:09, Alex Henrie wrote:
> > Cc: Andrew Eikum
> > Signed-off-by: Alex Henrie
> >
> > dlls/qedit/Makefile.in      |   3 +-
> >  dlls/qedit/qedit_private.h  |   2 +
> >  dlls/qedit/tests/timeline.c |   3 -
> >  dlls/qedit/timeline.c       |  10 +-
> >  dlls/qedit/timelineobj.c    | 470 ++++++++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 482 insertions(+), 6 deletions(-)
> >  create mode 100644 dlls/qedit/timelineobj.c
>
> I think it's better to add this to timeline.c.

I'm not sure it's a good idea to shove everything into one file.
Andrew, what do you think?
> > -
> > - TRACE("(%d,%p)\n", type, ppv);
> > -
> > - switch (type)
> > - {
> > -
> >    case TIMELINE_MAJOR_TYPE_COMPOSITE:
> > -
> >    case TIMELINE_MAJOR_TYPE_TRACK:
> > -
> >    case TIMELINE_MAJOR_TYPE_SOURCE:
> > -
> >    case TIMELINE_MAJOR_TYPE_TRANSITION:
> > -
> >    case TIMELINE_MAJOR_TYPE_EFFECT:
> > -
> >    case TIMELINE_MAJOR_TYPE_GROUP:
> > -
> >        break;
> > -
> >    default:
> > -
> >        return E_INVALIDARG;
> > - }
>
> What happens to returned pointer on type error?

Nothing, but I'll add a test for this and resubmit. I will also make
the rest of the changes to this patch that you suggested.

-Alex

[PATCH 2/5] qedit: Stub out AMTimelineObj.

[Andrew Eikum]

On Mon, Apr 25, 2016 at 10:49:07PM -0600, Alex Henrie wrote:
> 2016-04-25 22:17 GMT-06:00 Nikolay Sivov :
> > On 26.04.2016 6:09, Alex Henrie wrote:
> > > Cc: Andrew Eikum
> > > Signed-off-by: Alex Henrie
> > >
> > > dlls/qedit/Makefile.in      |   3 +-
> > >  dlls/qedit/qedit_private.h  |   2 +
> > >  dlls/qedit/tests/timeline.c |   3 -
> > >  dlls/qedit/timeline.c       |  10 +-
> > >  dlls/qedit/timelineobj.c    | 470 ++++++++++++++++++++++++++++++++++++++++++++
> > >  5 files changed, 482 insertions(+), 6 deletions(-)
> > >  create mode 100644 dlls/qedit/timelineobj.c
> >
> > I think it's better to add this to timeline.c.
>
> I'm not sure it's a good idea to shove everything into one file.
> Andrew, what do you think?

I agree with Nikolay, we generally prefer fewer source files. The
timeline and timeline objects are all related, so let's keep them in
the same file.
> Nothing, but I'll add a test for this and resubmit. I will also make
> the rest of the changes to this patch that you suggested.

Like Nikolay said, since there's no API to create instances as
aggregate objects, there's no need for the separate IUnknown
implementation.

Personally, I like leaving the ZeroMemory in since this isn't
performance-critical code and it future-proofs against forgetting to
initialize newly added fields. But I wouldn't reject a patch either
way.

Andrew