Re: msxml3: Handle NULL parameter in IXMLDOMElement::get_ownerDocument().

[Nikolay Sivov]

On 14.07.2015 16:31, Zhenbo Li wrote:
> Hi Nikolay,
>
> 2015-07-14 20:17 GMT+08:00 Nikolay Sivov :
> > In principle this is most likely fine, but I'd really prefer to have similar
> > test for all node types that support this method, so we can move a check
> > directly to node_get_owner_doc(). For those additional tests it would be
> > better to add a new test function called test_get_ownerDocument(). Next step
> > would be to make this test run on all document versions (3, 4 and 6), but
> > that should be a separate patch, and I'm not asking you to work on it.
>
> Thanks for your reply.
> I agree with you that move the check into node_get_owner_doc might be
> more general
>
> Do you mean I should write a helper function like:
>
> #define test_get_ownerDocument_call(el,doc)
> _test_get_ownerDocument_call(__LINE__,el,doc)
> static void _test_get_ownerDocument_call(unsigned line, IXMLDOMElement
> *element, IXMLDOMDocument *doc)
> {
>      IXMLDOMDocument *owner;
>      HRESULT r;
>      r = IXMLDOMElement_get_ownerDocument( element, NULL );
>      ok( r == E_INVALIDARG, "got %08x, expected E_INVALIDARG\n", r);
>  owner = NULL;
>  r = IXMLDOMElement_get_ownerDocument( element, &owner );
>  ok( r == S_OK, "get_ownerDocument return code\n");
>  ok( owner != doc, "get_ownerDocument return\n");
>  IXMLDOMDocument_Release(owner);
>
> }

No, helper like that won't work, because the point is to check how all 
node types work, not just elements. What I meant is to have a separate 
test_* function in START_TEST() that will load a document with all node 
types you need and iterating through them call get_ownderDocument.

msxml3: Handle NULL parameter in IXMLDOMElement::get_ownerDocument().

[Zhenbo Li]

2015-07-15 16:24 GMT+08:00 Nikolay Sivov :
> >
>
> No, helper like that won't work, because the point is to check how all node
> types work, not just elements. What I meant is to have a separate test_*
> function in START_TEST() that will load a document with all node types you
> need and iterating through them call get_ownderDocument.

Hi Nikolay,
Do you mean an iteration like that? Have I tested all the included nodes?
Also, we already have a helper function: test_get_ownerDocument()
Should I use this?

My draft patch has been attached.

Thanks

msxml3: Handle NULL parameter in IXMLDOMElement::get_ownerDocument().

[Nikolay Sivov]

On 18.07.2015 18:21, Zhenbo Li wrote:
> 2015-07-15 16:24 GMT+08:00 Nikolay Sivov :
> > >
> >
> > No, helper like that won't work, because the point is to check how all node
> > types work, not just elements. What I meant is to have a separate test_*
> > function in START_TEST() that will load a document with all node types you
> > need and iterating through them call get_ownderDocument.
>
> Hi Nikolay,
> Do you mean an iteration like that? Have I tested all the included nodes?

In principle yes, but you're not testing anything in that patch, 
ownerDocument() is IXMLDOMNode property, you have to call it on 'node'.
Also this document doesn't cover anything but elements I think, because 
childNodes() only iterates through immediate children, and 
grand-children are not included (I think, it's been a while since I 
looked into that). So I suggest adding a new document, with all kinds of 
nodes, and run a loop on it. Attribute nodes still need special handling.
> Also, we already have a helper function: test_get_ownerDocument()
> Should I use this?

Right, I missed that, please move new loop there.
> My draft patch has been attached.
>
> Thanks

msxml3: Handle NULL parameter in IXMLDOMElement::get_ownerDocument().

[Zhenbo Li]

Hi Nikolay,

2015-07-18 23:34 GMT+08:00 Nikolay Sivov :
> > Do you mean an iteration like that? Have I tested all the included nodes?
>
> In principle yes, but you're not testing anything in that patch,
> ownerDocument() is IXMLDOMNode property, you have to call it on 'node'.
> Also this document doesn't cover anything but elements I think, because
> childNodes() only iterates through immediate children, and grand-children
> are not included (I think, it's been a while since I looked into that). So I
> suggest adding a new document, with all kinds of nodes, and run a loop on
> it. Attribute nodes still need special handling.

Okay, so I write a recursive test for it. Could you have a check with it?
> > Also, we already have a helper function: test_get_ownerDocument()
> > Should I use this?
>
> Right, I missed that, please move new loop there.

I'm wondering how to combine the tests now.
Can I just call test_null_owner in test_get_ownerDocument()?
It seems easier.

Thanks