Re: [PATCH 5/5] qedit: Implement IAMTimelineObj_GetTimelineNoRef and add tests.
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.
2016-04-25 22:21 GMT-06:00 Nikolay Sivov <bunglehead(a)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. -Alex
On 26.04.2016 7:57, Alex Henrie wrote:
2016-04-25 22:21 GMT-06:00 Nikolay Sivov <bunglehead(a)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.
-Alex
2016-04-25 23:10 GMT-06:00 Nikolay Sivov <bunglehead(a)gmail.com>:
On 26.04.2016 7:57, Alex Henrie wrote:
2016-04-25 22:21 GMT-06:00 Nikolay Sivov <bunglehead(a)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. As always, you can see my most up-to-date work at <https://github.com/alexhenrie/wine/commits/master>. Andrew: Don't bother signing off any of these patches. Just tell me if you want timelineobj.c merged into timeline.c, and any other comments you have. After that I will resubmit all 5 patches with the new tests etc. that Nikolay suggested. -Alex
On Mon, Apr 25, 2016 at 11:46:09PM -0600, Alex Henrie wrote:
2016-04-25 23:10 GMT-06:00 Nikolay Sivov <bunglehead(a)gmail.com>:
On 26.04.2016 7:57, Alex Henrie wrote:
2016-04-25 22:21 GMT-06:00 Nikolay Sivov <bunglehead(a)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. Andrew
2016-04-26 7:33 GMT-06:00 Andrew Eikum <aeikum(a)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(a)gmail.com>:
On 26.04.2016 7:57, Alex Henrie wrote:
2016-04-25 22:21 GMT-06:00 Nikolay Sivov <bunglehead(a)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
On 26.04.2016 16:33, Andrew Eikum wrote:
On Mon, Apr 25, 2016 at 11:46:09PM -0600, Alex Henrie wrote:
2016-04-25 23:10 GMT-06:00 Nikolay Sivov <bunglehead(a)gmail.com>:
On 26.04.2016 7:57, Alex Henrie wrote:
2016-04-25 22:21 GMT-06:00 Nikolay Sivov <bunglehead(a)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.
I agree, it's better to keep it simple, and NoRef implies something non-standard. Sorry for the noise.
Andrew
participants (3)
-
Alex Henrie -
Andrew Eikum -
Nikolay Sivov