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

[Nikolay Sivov]

On 26.04.2016 6:09, Alex Henrie wrote:
> Cc: Andrew Eikum <aeikum(a)codeweavers.com>
>
> Signed-off-by: Alex Henrie <alexhenrie24(a)gmail.com>
> ---
>  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?