Re: mshtml: Add IHTMLXMLHttpRequest::responseXML() method implementation. (try 2)

[Jacek Caban]

Hi Zhenbo,

On 11/04/15 15:34, Zhenbo Li wrote:
> Try 2:
> Thanks Nikolay & Jacek
> Making the ERR() less verbose
> Signed-off-by: Zhenbo Li
>
> dlls/mshtml/tests/xmlhttprequest.c | 41 +++++++++++++++++++++++++++++
>  dlls/mshtml/xmlhttprequest.c       | 53
> ++++++++++++++++++++++++++++++++++++--
>  2 files changed, 92 insertions(+), 2 deletions(-)

Sorry for the delay, I was waiting for wine-devel discussion to come to
conclusion, but failed to reply when it did. It would be nice to move it
from pending state, although I'm not sure we can do that before code freeze.

+
+    hres = IXMLDOMDocument_loadXML(xmldoc, str, &vbool);
+    SysFreeString(str);
+    if(hres != S_OK || vbool != VARIANT_TRUE) {
+        ERR("loadXML failed: %08x\n", hres);
+        IXMLDOMDocument_Release(xmldoc);
+        return hres;
+    }

Note that if loadXML fails, you will return S_FALSE here, which I'm sure is desired. It would be nice to have a test for that.

Also discussion with Nikolay convinced me that assert() would be a better way to handle IObjectSafety errors.

Thanks,
Jacek

mshtml: Add IHTMLXMLHttpRequest::responseXML() method implementation. (try 2)

[Zhenbo Li]

Hi Jacek,

2015-11-13 19:39 GMT+08:00 Jacek Caban :
> Sorry for the delay, I was waiting for wine-devel discussion to come to
> conclusion, but failed to reply when it did. It would be nice to move it
> from pending state, although I'm not sure we can do that before code freeze.
> -
> - hres = IXMLDOMDocument_loadXML(xmldoc, str, &vbool);
> - SysFreeString(str);
> - if(hres != S_OK || vbool != VARIANT_TRUE) {
> -
>    ERR("loadXML failed: %08x\n", hres);
> -
>    IXMLDOMDocument_Release(xmldoc);
> -
>    return hres;
> - }
>
> Note that if loadXML fails, you will return S_FALSE here, which I'm sure is desired. It would be nice to have a test for that.

Yes, but I'm wondering how to test it, as we can find test cases for
IXMLDOMDocument_loadXML in msxml3/tests
> Also discussion with Nikolay convinced me that assert() would be a better way to handle IObjectSafety errors.
>
> Thanks,
> Jacek

Also, I found that there was a hidden bug:
As we will test these urls for responseXML:
    static const char xml_url[] = "http://test.winehq.org/tests/xmltest.xml";
    static const char large_page_url[] =
"http://test.winehq.org/tests/data.php";

char large_page_url is not a valid xml file, but native mshtml.dll can
handle it properly. It shows we can't just past the responseText to
msxml3.dll. I'll check it more carefully it later.

mshtml: Add IHTMLXMLHttpRequest::responseXML() method implementation. (try 2)

[Zhenbo Li]

2015-11-14 17:33 GMT+08:00 Zhenbo Li :
> Also, I found that there was a hidden bug:
> As we will test these urls for responseXML:
>     static const char xml_url[] = "http://test.winehq.org/tests/xmltest.xml";
>     static const char large_page_url[] =
> "http://test.winehq.org/tests/data.php";
>
> char large_page_url is not a valid xml file, but native mshtml.dll can
> handle it properly. It shows we can't just past the responseText to
> msxml3.dll. I'll check it more carefully it later.

Hi, sorry for my late reply.
I tested it on testbot, and found that Windows mshtml.dll would return
an empty xml object

mshtml: Add IHTMLXMLHttpRequest::responseXML() method implementation. (try 2)

[Jacek Caban]

Hi Zhenbo,

On 11/27/15 15:49, Zhenbo Li wrote:
> 2015-11-14 17:33 GMT+08:00 Zhenbo Li :
> > > Also, I found that there was a hidden bug:
> > > As we will test these urls for responseXML:
> > >     static const char xml_url[] = "http://test.winehq.org/tests/xmltest.xml";
> > >     static const char large_page_url[] =
> > > "http://test.winehq.org/tests/data.php";
> > >
> > > char large_page_url is not a valid xml file, but native mshtml.dll can
> > > handle it properly. It shows we can't just past the responseText to
> > > msxml3.dll. I'll check it more carefully it later.
>
> Hi, sorry for my late reply.
> I tested it on testbot, and found that Windows mshtml.dll would return
> an empty xml object

It looks better now.
> static BOOL doc_complete;
>  static IHTMLDocument2 *notif_doc;
> +static BOOL ILLEGAL_XML;

Using this global variable makes things harder to follow. Please add a
new argument to test_sync_xhr instead.
> @@ -334,8 +337,52 @@ static HRESULT WINAPI HTMLXMLHttpRequest_get_responseText(IHTMLXMLHttpRequest *i
>  static HRESULT WINAPI HTMLXMLHttpRequest_get_responseXML(IHTMLXMLHttpRequest *iface, IDispatch **p)
>  {
>      HTMLXMLHttpRequest *This = impl_from_IHTMLXMLHttpRequest(iface);
> - FIXME("(%p)->(%p)\n", This, p);
> - return E_NOTIMPL;
> - IXMLDOMDocument *xmldoc = NULL;
> - BSTR str;
> - HRESULT hres;
> - VARIANT_BOOL vbool;
> - IObjectSafety *safety;
> -
> - TRACE("(%p)->(%p)\n", This, p);
> -
> - hres = CoCreateInstance(&CLSID_DOMDocument, NULL, CLSCTX_INPROC_SERVER, &IID_IXMLDOMDocument, (void**)&xmldoc);
> - if(FAILED(hres)) {
> -
>    ERR("CoCreateInstance failed: %08x\n", hres);
> -
>    return hres;
> - }
> -
> - hres = IHTMLXMLHttpRequest_get_responseText(iface, &str);
> - if(FAILED(hres)) {
> -
>    IXMLDOMDocument_Release(xmldoc);
> -
>    ERR("get_responseText failed: %08x\n", hres);
> -
>    return hres;
> - }
> -
> - hres = IXMLDOMDocument_loadXML(xmldoc, str, &vbool);
> - SysFreeString(str);
> - if(hres != S_OK || vbool != VARIANT_TRUE) {
> -
>    ERR("loadXML failed: %08x, returning an cmpty xmldoc\n", hres);
> - }

ERR is not appropriate here. Please use WARN instead.
> -
> - hres = IXMLDOMDocument_QueryInterface(xmldoc, &IID_IObjectSafety, (void**)&safety);
> - if(SUCCEEDED(hres)) {

Again, this error handling is more complicated than it needs to be.
Please use assert(hres == S_OK) instead.
> -
>    hres = IObjectSafety_SetInterfaceSafetyOptions(safety, NULL,
> -
>        INTERFACESAFE_FOR_UNTRUSTED_CALLER | INTERFACESAFE_FOR_UNTRUSTED_DATA | INTERFACE_USES_SECURITY_MANAGER,
> -
>        INTERFACESAFE_FOR_UNTRUSTED_CALLER | INTERFACESAFE_FOR_UNTRUSTED_DATA | INTERFACE_USES_SECURITY_MANAGER);
> -
>    IObjectSafety_Release(safety);
> -
>    if(FAILED(hres)) {

Same here.

Thanks,
Jacek

mshtml: Add IHTMLXMLHttpRequest::responseXML() method implementation. (try 2)

[Christopher Harrington]

On Mon, Nov 30, 2015 at 5:32 AM Jacek Caban  wrote:
> Hi Zhenbo,
>
> On 11/27/15 15:49, Zhenbo Li wrote:
>
> 2015-11-14 17:33 GMT+08:00 Zhenbo Li  :
> - hres = IXMLDOMDocument_loadXML(xmldoc, str, &vbool);
> - SysFreeString(str);
> - if(hres != S_OK || vbool != VARIANT_TRUE) {
> -
>    ERR("loadXML failed: %08x, returning an cmpty xmldoc\n", hres);
> - }
>
> ERR is not appropriate here. Please use WARN instead.

Also "cmpty" possibly a typo for "empty"?
> Thanks,
> Jacek