[PATCH 5/5] qedit: Implement IAMTimelineObj_GetTimelineNoRef and add tests.

[Alex Henrie]

2016-04-26 7:33 GMT-06:00 Andrew Eikum <aeikum at codeweavers.com>:
> On Mon, Apr 25, 2016 at 11:46:09PM -0600, Alex Henrie wrote:
>> 2016-04-25 23:10 GMT-06:00 Nikolay Sivov <bunglehead at gmail.com>:
>> > On 26.04.2016 7:57, Alex Henrie wrote:
>> >> 2016-04-25 22:21 GMT-06:00 Nikolay Sivov <bunglehead at gmail.com>:
>> >>> On 26.04.2016 6:10, Alex Henrie wrote:
>> >>>>  static HRESULT WINAPI TimelineObj_GetTimelineNoRef(IAMTimelineObj *iface, IAMTimeline **timeline)
>> >>>>  {
>> >>>> +    /* MSDN says that this function is "not supported" */
>> >>>>      TimelineObjImpl *This = impl_from_IAMTimelineObj(iface);
>> >>>> -    FIXME("(%p)->(%p): not implemented!\n", This, timeline);
>> >>>> -    return E_NOTIMPL;
>> >>>> +    TRACE("(%p)->(%p)\n", This, timeline);
>> >>>> +    if (!timeline) return E_POINTER;
>> >>>> +    return E_NOINTERFACE;
>> >>>>  }
>> >>>
>> >>> This looks like a call to QI, according to your tests.
>> >>
>> >> It looks that way, but which interface would it be querying? The tests
>> >> show that an IAMTimelineObj cannot double as an IAMTimeline, nor is
>> >> the function supposed to return the IAMTimeline that was used to
>> >> create the IAMTimelineObj. My best guess is that Microsoft never
>> >> bothered implementing this function properly and so whatever
>> >> implementation they have always winds up returning E_NOINTERFACE.
>> >
>> > Yes, return type is IAMTimeline, so I was suggesting a shortcut doing
>> > same thing in one line IAMTimelineObj_QueryInterface(iface,
>> > &IID_IAMTimeline, ...). This should cover for setting out pointer to
>> > NULL, returning E_NOINTERFACE, and probably handling NULL case too,
>> > though it's not a standard QI behavior. None of that is actually
>> > important, what you did is fine, take it as a suggestion, or don't.
>>
>> OK, I see what you mean now, and I'll make that change in version 2 of
>> this patchset.
>>
>
> Eh, I'd rather keep it like it was. It's not clear whether it should
> be QIing on this same object or on the parent Timeline object, and the
> "NoRef" is weird behavior for a QI.  Implementing it on top of QI
> seems to imply confidence we don't have, and would confuse me if I was
> reading this code blind.

OK, I've changed it back, except that I'm still setting *timeline =
NULL so that the new test passes.

-Alex