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?