[PATCH v2 0/3] MR5685: Implement MarkupServices_ParseString
-- v2: mshtml: Implement MarkupServices_ParseString. mshtml: Implement MarkupServices_CreateMarkupContainer. mshtml/tests: Test for IMarkupServices. https://gitlab.winehq.org/wine/wine/-/merge_requests/5685
From: Santino Mazza <smazza(a)codeweavers.com> --- dlls/mshtml/tests/htmldoc.c | 116 ++++++++++++++++++++++++++++++++++++ 1 file changed, 116 insertions(+) diff --git a/dlls/mshtml/tests/htmldoc.c b/dlls/mshtml/tests/htmldoc.c index f41d5749b3b..1f382b23523 100644 --- a/dlls/mshtml/tests/htmldoc.c +++ b/dlls/mshtml/tests/htmldoc.c @@ -8335,6 +8335,121 @@ static void test_MHTMLDocument(void) release_document(doc); } +static LONG get_document_elements_count(IHTMLDocument2 *doc) +{ + LONG elements_count; + IHTMLElementCollection *elements = NULL; + + IHTMLDocument2_get_all(doc, &elements); + IHTMLElementCollection_get_length(elements, &elements_count); + + IHTMLElementCollection_Release(elements); + return elements_count; +} + +static void test_MarkupContainer(IMarkupServices *markup_services) +{ + IHTMLDOMNode *doc_node; + IHTMLDocument2 *markup_container_doc; + ITargetContainer *target_container_doc; + IInternetSecurityManager *sec_manager; + IMarkupContainer *container = NULL; + ICustomDoc *custom_doc; + IHTMLLocation *location = NULL; + IHTMLElement *body = NULL; + LONG elements_count; + HRESULT hres; + + hres = IMarkupServices_CreateMarkupContainer(markup_services, &container); + todo_wine ok(hres == S_OK, "got 0x%08lx\n", hres); + todo_wine ok(container != NULL, "MarkupContainer is null.\n"); + if (!container) return; + + hres = IMarkupContainer_QueryInterface(container, &IID_IHTMLDocument2, (void**)&markup_container_doc); + todo_wine ok(hres == S_OK, "got 0x%08lx\n", hres); + + hres = IHTMLDocument2_get_location(markup_container_doc, &location); + todo_wine ok(hres == E_UNEXPECTED, "expected E_UNEXPECTED, got 0x%08lx\n", hres); + todo_wine ok(location == NULL, "expected null location\n"); + + hres = IHTMLDocument2_get_body(markup_container_doc, &body); + todo_wine ok(hres == S_OK, "got 0x%08lx\n", hres); + todo_wine ok(body == NULL, "expected null body\n"); + + elements_count = get_document_elements_count(markup_container_doc); + todo_wine ok(elements_count == 0, "expected document to not have elements\n"); + + hres = IMarkupContainer_QueryInterface(container, &IID_IHTMLDOMNode, (void**)&doc_node); + todo_wine ok(hres == E_NOINTERFACE, "expected to fail with E_NOINTERFACE got 0x%08lx\n", hres); + + hres = IHTMLDocument2_QueryInterface(markup_container_doc, &IID_IInternetSecurityManager, (void**)&sec_manager); + todo_wine ok(hres == E_NOINTERFACE, "expected to fail with E_NOINTERFACE got 0x%08lx\n", hres); + + hres = IHTMLDocument2_QueryInterface(markup_container_doc, &IID_ITargetContainer, (void**)&target_container_doc); + todo_wine ok(hres == E_NOINTERFACE, "expected to fail with E_NOINTERFACE got 0x%08lx\n", hres); + + hres = IHTMLDocument2_QueryInterface(markup_container_doc, &IID_ICustomDoc, (void**)&custom_doc); + todo_wine ok(hres == E_NOINTERFACE, "expected to fail with E_NOINTERFACE got 0x%08lx\n", hres); + + IHTMLDocument2_Release(markup_container_doc); + IMarkupContainer_Release(container); +} + +static void test_MarkupServices_ParseString(IMarkupServices *markup_services, IHTMLDocument2 *doc) +{ + HRESULT hres; + LONG document_elements_count, markup_container_elements_count; + IHTMLElement *body; + BSTR inner_text; + IHTMLDocument2 *markup_container_doc = NULL; + IMarkupContainer *markup_container = NULL; + + hres = IMarkupServices_ParseString(markup_services, (OLECHAR*)L"<div>Hello World</div>", 0, &markup_container, NULL, NULL); + todo_wine ok(hres == S_OK, "got 0x%08lx\n", hres); + todo_wine ok(markup_container != NULL, "MarkupContainer is null.\n"); + if (!markup_container) return; + + hres = IMarkupContainer_QueryInterface(markup_container, &IID_IHTMLDocument2, (void**)&markup_container_doc); + todo_wine ok(hres == S_OK, "failed to query interface of MarkupContainer 0x%08lx\n", hres); + + markup_container_elements_count = get_document_elements_count(markup_container_doc); + todo_wine ok(markup_container_elements_count == 5, "expected markup container to have 5 elements but got %ld\n", + markup_container_elements_count); + + document_elements_count = get_document_elements_count(doc); + todo_wine ok(document_elements_count != markup_container_elements_count, + "expected document to not have the same elements count of the markup container %ld == %ld\n", + document_elements_count, markup_container_elements_count); + + hres = IHTMLDocument2_get_body(markup_container_doc, &body); + todo_wine ok(hres == S_OK, "got 0x%08lx\n", hres); + todo_wine ok(body != NULL, "got null body\n"); + + hres = IHTMLElement_get_innerText(body, &inner_text); + todo_wine ok(hres == S_OK, "failed to get inner text error 0x%08lx\n", hres); + todo_wine ok(inner_text != NULL, "got a null pointer for inner text\n"); + todo_wine ok(!wcscmp(inner_text, L"Hello World"), "strings don't match, got %ls\n", inner_text); + + SysFreeString(inner_text); + IHTMLElement_Release(body); + IHTMLDocument2_Release(markup_container_doc); + IMarkupContainer_Release(markup_container); +} + +static void test_MarkupServices(IHTMLDocument2 *doc) +{ + HRESULT hres; + IMarkupServices *markup_services = NULL; + + hres = IHTMLDocument2_QueryInterface(doc, &IID_IMarkupServices, (void**)&markup_services); + ok(hres == S_OK, "got 0x%08lx\n", hres); + + test_MarkupContainer(markup_services); + test_MarkupServices_ParseString(markup_services, doc); + + IMarkupServices_Release(markup_services); +} + static void test_HTMLDocument_hlink(DWORD status) { IHTMLDocument2 *doc; @@ -8501,6 +8616,7 @@ static void test_HTMLDocument_http(BOOL with_wbapp) test_travellog(doc); test_binding_ui((IUnknown*)doc); test_doc_domain(doc); + test_MarkupServices(doc); nav_url = nav_serv_url = L"http://test.winehq.org/tests/winehq_snapshot/"; /* for valid prev nav_url */ if(support_wbapp) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5685
From: Santino Mazza <smazza(a)codeweavers.com> --- dlls/mshtml/htmldoc.c | 16 ++++++++++++++-- dlls/mshtml/tests/htmldoc.c | 22 +++++++++++----------- 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/dlls/mshtml/htmldoc.c b/dlls/mshtml/htmldoc.c index 118f4706f37..c34fe804720 100644 --- a/dlls/mshtml/htmldoc.c +++ b/dlls/mshtml/htmldoc.c @@ -5260,8 +5260,20 @@ static HRESULT WINAPI MarkupServices_CreateMarkupPointer(IMarkupServices *iface, static HRESULT WINAPI MarkupServices_CreateMarkupContainer(IMarkupServices *iface, IMarkupContainer **ppMarkupContainer) { HTMLDocumentNode *This = impl_from_IMarkupServices(iface); - FIXME("(%p)->(%p)\n", This, ppMarkupContainer); - return E_NOTIMPL; + IHTMLDocument2 *frag; + HRESULT hres; + TRACE("(%p)->(%p)\n", This, ppMarkupContainer); + + hres = IHTMLDocument3_createDocumentFragment(&This->IHTMLDocument3_iface, &frag); + if(hres != S_OK) + return hres; + + hres = IHTMLDocument2_QueryInterface(frag, &IID_IMarkupContainer, (void**)ppMarkupContainer); + if(hres != S_OK) + return hres; + + IHTMLDocument2_Release(frag); + return S_OK; } static HRESULT WINAPI MarkupServices_CreateElement(IMarkupServices *iface, diff --git a/dlls/mshtml/tests/htmldoc.c b/dlls/mshtml/tests/htmldoc.c index 1f382b23523..412dc5bb8bb 100644 --- a/dlls/mshtml/tests/htmldoc.c +++ b/dlls/mshtml/tests/htmldoc.c @@ -8361,35 +8361,35 @@ static void test_MarkupContainer(IMarkupServices *markup_services) HRESULT hres; hres = IMarkupServices_CreateMarkupContainer(markup_services, &container); - todo_wine ok(hres == S_OK, "got 0x%08lx\n", hres); - todo_wine ok(container != NULL, "MarkupContainer is null.\n"); + ok(hres == S_OK, "got 0x%08lx\n", hres); + ok(container != NULL, "MarkupContainer is null.\n"); if (!container) return; hres = IMarkupContainer_QueryInterface(container, &IID_IHTMLDocument2, (void**)&markup_container_doc); - todo_wine ok(hres == S_OK, "got 0x%08lx\n", hres); + ok(hres == S_OK, "got 0x%08lx\n", hres); hres = IHTMLDocument2_get_location(markup_container_doc, &location); - todo_wine ok(hres == E_UNEXPECTED, "expected E_UNEXPECTED, got 0x%08lx\n", hres); - todo_wine ok(location == NULL, "expected null location\n"); + ok(hres == E_UNEXPECTED, "expected E_UNEXPECTED, got 0x%08lx\n", hres); + ok(location == NULL, "expected null location\n"); hres = IHTMLDocument2_get_body(markup_container_doc, &body); - todo_wine ok(hres == S_OK, "got 0x%08lx\n", hres); - todo_wine ok(body == NULL, "expected null body\n"); + ok(hres == S_OK, "got 0x%08lx\n", hres); + ok(body == NULL, "expected null body\n"); elements_count = get_document_elements_count(markup_container_doc); - todo_wine ok(elements_count == 0, "expected document to not have elements\n"); + ok(elements_count == 0, "expected document to not have elements\n"); hres = IMarkupContainer_QueryInterface(container, &IID_IHTMLDOMNode, (void**)&doc_node); todo_wine ok(hres == E_NOINTERFACE, "expected to fail with E_NOINTERFACE got 0x%08lx\n", hres); hres = IHTMLDocument2_QueryInterface(markup_container_doc, &IID_IInternetSecurityManager, (void**)&sec_manager); - todo_wine ok(hres == E_NOINTERFACE, "expected to fail with E_NOINTERFACE got 0x%08lx\n", hres); + ok(hres == E_NOINTERFACE, "expected to fail with E_NOINTERFACE got 0x%08lx\n", hres); hres = IHTMLDocument2_QueryInterface(markup_container_doc, &IID_ITargetContainer, (void**)&target_container_doc); - todo_wine ok(hres == E_NOINTERFACE, "expected to fail with E_NOINTERFACE got 0x%08lx\n", hres); + ok(hres == E_NOINTERFACE, "expected to fail with E_NOINTERFACE got 0x%08lx\n", hres); hres = IHTMLDocument2_QueryInterface(markup_container_doc, &IID_ICustomDoc, (void**)&custom_doc); - todo_wine ok(hres == E_NOINTERFACE, "expected to fail with E_NOINTERFACE got 0x%08lx\n", hres); + ok(hres == E_NOINTERFACE, "expected to fail with E_NOINTERFACE got 0x%08lx\n", hres); IHTMLDocument2_Release(markup_container_doc); IMarkupContainer_Release(container); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5685
From: Santino Mazza <smazza(a)codeweavers.com> --- dlls/mshtml/htmldoc.c | 59 +++++++++++++++++++++++++++++++++++-- dlls/mshtml/tests/htmldoc.c | 20 ++++++------- 2 files changed, 67 insertions(+), 12 deletions(-) diff --git a/dlls/mshtml/htmldoc.c b/dlls/mshtml/htmldoc.c index c34fe804720..ccb6d736c01 100644 --- a/dlls/mshtml/htmldoc.c +++ b/dlls/mshtml/htmldoc.c @@ -5346,9 +5346,64 @@ static HRESULT WINAPI MarkupServices_ParseString(IMarkupServices *iface, OLECHAR *pchHTML, DWORD dwFlags, IMarkupContainer **ppContainerResult, IMarkupPointer *pPointerStart, IMarkupPointer *pPointerFinish) { + HRESULT hres; + IMarkupContainer *container; + IHTMLDocument2 *doc; + IHTMLDOMNode *node, *html_node, *new_html_node = NULL; + IHTMLElement *html, *body; HTMLDocumentNode *This = impl_from_IMarkupServices(iface); - FIXME("(%p)->(%s,%lx,%p,%p,%p)\n", This, debugstr_w(pchHTML), dwFlags, ppContainerResult, pPointerStart, pPointerFinish); - return E_NOTIMPL; + TRACE("(%p)->(%s,%lx,%p,%p,%p)\n", This, debugstr_w(pchHTML), dwFlags, ppContainerResult, pPointerStart, pPointerFinish); + + if(dwFlags != 0) + FIXME("flags %lx not implemented.\n", dwFlags); + if(pPointerStart || pPointerFinish) { + FIXME("Pointers not implemented.\n"); + return E_NOTIMPL; + } + + hres = IMarkupServices_CreateMarkupContainer(iface, &container); + if(hres != S_OK) + return hres; + + hres = IMarkupContainer_QueryInterface(container, &IID_IHTMLDocument2, (void**)&doc); + if(hres != S_OK) { + IMarkupContainer_Release(container); + return hres; + } + + hres = IHTMLDocument2_QueryInterface(doc, &IID_IHTMLDOMNode, (void**)&node); + if(hres != S_OK) { + IHTMLDocument2_Release(doc); + IMarkupContainer_Release(container); + return hres; + } + + IHTMLDocument2_createElement(&This->IHTMLDocument2_iface, (BSTR)L"html", &html); + IHTMLElement_put_innerHTML(html, (BSTR)L"<head><title></title></head><body></body>"); + + hres = IHTMLElement_QueryInterface(html, &IID_IHTMLDOMNode, (void**)&html_node); + if(hres != S_OK) { + IHTMLElement_Release(html); + IHTMLDocument2_Release(doc); + IMarkupContainer_Release(container); + return hres; + } + IHTMLElement_Release(html); + + IHTMLDOMNode_appendChild(node, html_node, &new_html_node); + IHTMLDOMNode_Release(node); + IHTMLDOMNode_Release(html_node); + IHTMLDOMNode_Release(new_html_node); + + IHTMLDocument2_get_body(doc, &body); + IHTMLElement_put_innerHTML(body, pchHTML); + + IHTMLDocument2_Release(doc); + IHTMLElement_Release(body); + + *ppContainerResult = container; + + return S_OK; } static HRESULT WINAPI MarkupServices_ParseGlobal(IMarkupServices *iface, diff --git a/dlls/mshtml/tests/htmldoc.c b/dlls/mshtml/tests/htmldoc.c index 412dc5bb8bb..77d67adaace 100644 --- a/dlls/mshtml/tests/htmldoc.c +++ b/dlls/mshtml/tests/htmldoc.c @@ -8405,30 +8405,30 @@ static void test_MarkupServices_ParseString(IMarkupServices *markup_services, IH IMarkupContainer *markup_container = NULL; hres = IMarkupServices_ParseString(markup_services, (OLECHAR*)L"<div>Hello World</div>", 0, &markup_container, NULL, NULL); - todo_wine ok(hres == S_OK, "got 0x%08lx\n", hres); - todo_wine ok(markup_container != NULL, "MarkupContainer is null.\n"); + ok(hres == S_OK, "got 0x%08lx\n", hres); + ok(markup_container != NULL, "MarkupContainer is null.\n"); if (!markup_container) return; hres = IMarkupContainer_QueryInterface(markup_container, &IID_IHTMLDocument2, (void**)&markup_container_doc); - todo_wine ok(hres == S_OK, "failed to query interface of MarkupContainer 0x%08lx\n", hres); + ok(hres == S_OK, "failed to query interface of MarkupContainer 0x%08lx\n", hres); markup_container_elements_count = get_document_elements_count(markup_container_doc); - todo_wine ok(markup_container_elements_count == 5, "expected markup container to have 5 elements but got %ld\n", + ok(markup_container_elements_count == 5, "expected markup container to have 5 elements but got %ld\n", markup_container_elements_count); document_elements_count = get_document_elements_count(doc); - todo_wine ok(document_elements_count != markup_container_elements_count, + ok(document_elements_count != markup_container_elements_count, "expected document to not have the same elements count of the markup container %ld == %ld\n", document_elements_count, markup_container_elements_count); hres = IHTMLDocument2_get_body(markup_container_doc, &body); - todo_wine ok(hres == S_OK, "got 0x%08lx\n", hres); - todo_wine ok(body != NULL, "got null body\n"); + ok(hres == S_OK, "got 0x%08lx\n", hres); + ok(body != NULL, "got null body\n"); hres = IHTMLElement_get_innerText(body, &inner_text); - todo_wine ok(hres == S_OK, "failed to get inner text error 0x%08lx\n", hres); - todo_wine ok(inner_text != NULL, "got a null pointer for inner text\n"); - todo_wine ok(!wcscmp(inner_text, L"Hello World"), "strings don't match, got %ls\n", inner_text); + ok(hres == S_OK, "failed to get inner text error 0x%08lx\n", hres); + ok(inner_text != NULL, "got a null pointer for inner text\n"); + ok(!wcscmp(inner_text, L"Hello World"), "strings don't match, got %ls\n", inner_text); SysFreeString(inner_text); IHTMLElement_Release(body); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5685
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=145780 Your paranoid android. === debian11b (64 bit WoW report) === mfmediaengine: mfmediaengine.c:2564: Test failed: Unexpected time 0.000000. mfmediaengine.c:2616: Test failed: Unexpected time 0.133467. === debian11b (build log) === 009c:err:winediag:gnutls_process_attach failed to load libgnutls, no support for encryption
Is there a reason you're creating a html node and appending it instead of using `HTMLDocument3_get_documentElement`? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5685#note_71229
On Mon May 27 16:01:35 2024 +0000, Gabriel Ivăncescu wrote:
Is there a reason you're creating a html node and appending it instead of using `HTMLDocument3_get_documentElement`? Sorry for taking too long to respond, I had to test some stuff to remember why I did it heh (It's been a year since I wrote this changes). Document fragments don't have the documentElement property (https://developer.mozilla.org/en-US/docs/Web/API/Document/documentElement) like normal document objects.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5685#note_71424
On Mon May 27 16:02:11 2024 +0000, Santino Mazza wrote:
Sorry for taking too long to respond, I had to test some stuff to remember why I did it heh (It's been a year since I wrote this changes). Document fragments don't have the documentElement property (https://developer.mozilla.org/en-US/docs/Web/API/Document/documentElement) like normal document objects. I see. In fact it does look like doc frags don't have `html` tags at all, even on Windows:
```javascript var docfrag = document.createDocumentFragment(); ok(docfrag.querySelector("html") === null, "html tag in doc frag"); ``` (IE8+ since previous ones don't expose querySelector) So maybe your approach is correct, maybe Jacek has an idea. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5685#note_72476
On Wed Jun 5 15:20:42 2024 +0000, Gabriel Ivăncescu wrote:
I see. In fact it does look like doc frags don't have `html` tags at all, even on Windows: ```javascript var docfrag = document.createDocumentFragment(); ok(docfrag.querySelector("html") === null, "html tag in doc frag"); ``` (IE8+ since previous ones don't expose querySelector) So maybe your approach is correct, maybe Jacek has an idea. I suspect some similar mechanism to clipboard data handing is in play; it probably feels the document fragment to full document on parse. That does rise some questions like what happens if you pass a string containing, for example, `<body>` element.
Anyway, for such an obscure feature, it's likely that digging too deep doesn't make much sense; those old APIs tend to differ badly from what we can get from Gecko. I will have some minor comments, but overall I guess that's fine. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5685#note_72535
Jacek Caban (@jacek) commented about dlls/mshtml/tests/htmldoc.c:
+ ITargetContainer *target_container_doc; + IInternetSecurityManager *sec_manager; + IMarkupContainer *container = NULL; + ICustomDoc *custom_doc; + IHTMLLocation *location = NULL; + IHTMLElement *body = NULL; + LONG elements_count; + HRESULT hres; + + hres = IMarkupServices_CreateMarkupContainer(markup_services, &container); + todo_wine ok(hres == S_OK, "got 0x%08lx\n", hres); + todo_wine ok(container != NULL, "MarkupContainer is null.\n"); + if (!container) return; + + hres = IMarkupContainer_QueryInterface(container, &IID_IHTMLDocument2, (void**)&markup_container_doc); + todo_wine ok(hres == S_OK, "got 0x%08lx\n", hres); This `todo_wine` and all bellow in this function are never needed, we return after previous todo anyway. Same to `test_MarkupServices_ParseString`.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5685#note_72536
Jacek Caban (@jacek) commented about dlls/mshtml/htmldoc.c:
+ IHTMLElement *html, *body; HTMLDocumentNode *This = impl_from_IMarkupServices(iface); - FIXME("(%p)->(%s,%lx,%p,%p,%p)\n", This, debugstr_w(pchHTML), dwFlags, ppContainerResult, pPointerStart, pPointerFinish); - return E_NOTIMPL; + TRACE("(%p)->(%s,%lx,%p,%p,%p)\n", This, debugstr_w(pchHTML), dwFlags, ppContainerResult, pPointerStart, pPointerFinish); + + if(dwFlags != 0) + FIXME("flags %lx not implemented.\n", dwFlags); + if(pPointerStart || pPointerFinish) { + FIXME("Pointers not implemented.\n"); + return E_NOTIMPL; + } + + hres = IMarkupServices_CreateMarkupContainer(iface, &container); + if(hres != S_OK) + return hres; Since you know we're dealing with our own implementation of the interface, we know that those QIs can't fail. You could as well use assert or skip it to simplify it.
On the other hand, other functions may return valid errors. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5685#note_72538
Jacek Caban (@jacek) commented about dlls/mshtml/htmldoc.c:
static HRESULT WINAPI MarkupServices_CreateMarkupContainer(IMarkupServices *iface, IMarkupContainer **ppMarkupContainer) { HTMLDocumentNode *This = impl_from_IMarkupServices(iface); - FIXME("(%p)->(%p)\n", This, ppMarkupContainer); - return E_NOTIMPL; + IHTMLDocument2 *frag; + HRESULT hres; + TRACE("(%p)->(%p)\n", This, ppMarkupContainer); + + hres = IHTMLDocument3_createDocumentFragment(&This->IHTMLDocument3_iface, &frag); + if(hres != S_OK) + return hres; + + hres = IHTMLDocument2_QueryInterface(frag, &IID_IMarkupContainer, (void**)ppMarkupContainer); + if(hres != S_OK) + return hres; You leak `frag` on this return. In this case, you could just skip that `if()` and return `hres` in the end (or see the other comment).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5685#note_72537
participants (5)
-
Gabriel Ivăncescu -
Jacek Caban (＠jacek)
-
Marvin -
Santino Mazza
-
Santino Mazza (＠tati1454)