[PATCH v4 0/3] MR1918: Draft: Implement HTMLDocument_get_body and HTMLDocument_get_all for document fragments
This functions should also work with document fragments, in wine they only return NULL. -- v4: mshtml: Implement HTMLDocument_get_body for document fragments. https://gitlab.winehq.org/wine/wine/-/merge_requests/1918
From: Santino Mazza <smazza(a)codeweavers.com> --- dlls/mshtml/tests/dom.c | 48 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/dlls/mshtml/tests/dom.c b/dlls/mshtml/tests/dom.c index 6a960bfa21f..91bc73fd2d0 100644 --- a/dlls/mshtml/tests/dom.c +++ b/dlls/mshtml/tests/dom.c @@ -10984,7 +10984,7 @@ static IHTMLDocument2 *create_docfrag(IHTMLDocument2 *doc) static void test_docfrag(IHTMLDocument2 *doc) { IHTMLDocument2 *frag, *owner_doc, *doc_node; - IHTMLElement *div, *body, *br; + IHTMLElement *div, *body, *br,*html; IHTMLElementCollection *col; IHTMLLocation *location; HRESULT hres; @@ -10998,6 +10998,13 @@ static void test_docfrag(IHTMLDocument2 *doc) ET_BR }; + static const elem_type_t frag_types[] = { + ET_HTML, + ET_DIV, + ET_DIV, + ET_BODY + }; + frag = create_docfrag(doc); test_disp((IUnknown*)frag, &DIID_DispHTMLDocument, &CLSID_HTMLDocument, L"[object]"); @@ -11030,6 +11037,45 @@ static void test_docfrag(IHTMLDocument2 *doc) test_elem_collection((IUnknown*)col, all_types, ARRAY_SIZE(all_types)); IHTMLElementCollection_Release(col); + html = test_create_elem(doc, L"HTML"); + test_elem_source_index(html, -1); + test_node_append_child((IUnknown*)frag, (IUnknown*)html); + test_elem_source_index(html, 0); + + div = test_create_elem(doc, L"DIV"); + test_elem_source_index(div, -1); + test_node_append_child((IUnknown*)html, (IUnknown*)div); + test_elem_source_index(div, 1); + IHTMLElement_Release(div); + + div = test_create_elem(doc, L"DIV"); + test_elem_source_index(div, -1); + test_node_append_child((IUnknown*)html, (IUnknown*)div); + test_elem_source_index(div, 2); + + body = test_create_elem(doc, L"BODY"); + test_elem_source_index(body, -1); + test_node_append_child((IUnknown*)div, (IUnknown*)body); + test_elem_source_index(body, 3); + IHTMLElement_Release(body); + IHTMLElement_Release(div); + IHTMLElement_Release(html); + + hres = IHTMLDocument2_get_body(frag, &body); + ok(hres == S_OK, "get_body failed: %08lx\n", hres); + todo_wine ok(body != NULL, "body == NULL\n"); + if (body) + IHTMLElement_Release(body); + + col = NULL; + hres = IHTMLDocument2_get_all(frag, &col); + todo_wine ok(hres == S_OK, "get_all failed: %08lx\n", hres); + todo_wine ok(col != NULL, "got null elements collection\n"); + if (col) { + test_elem_collection((IUnknown *) col, frag_types, ARRAY_SIZE(frag_types)); + IHTMLElementCollection_Release(col); + } + div = test_create_elem(frag, L"div"); owner_doc = get_owner_doc((IUnknown*)div); doc_node = get_doc_node(doc); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1918
From: Santino Mazza <smazza(a)codeweavers.com> --- dlls/mshtml/htmldoc.c | 4 ++-- dlls/mshtml/tests/dom.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/dlls/mshtml/htmldoc.c b/dlls/mshtml/htmldoc.c index 90c4706abee..ac471e9e1a8 100644 --- a/dlls/mshtml/htmldoc.c +++ b/dlls/mshtml/htmldoc.c @@ -495,8 +495,8 @@ static HRESULT WINAPI HTMLDocument_get_all(IHTMLDocument2 *iface, IHTMLElementCo TRACE("(%p)->(%p)\n", This, p); if(!This->dom_document) { - WARN("NULL dom_document\n"); - return E_UNEXPECTED; + *p = create_all_collection(&This->node, FALSE); + return S_OK; } nsres = nsIDOMDocument_GetDocumentElement(This->dom_document, &nselem); diff --git a/dlls/mshtml/tests/dom.c b/dlls/mshtml/tests/dom.c index 91bc73fd2d0..a726801a43e 100644 --- a/dlls/mshtml/tests/dom.c +++ b/dlls/mshtml/tests/dom.c @@ -11069,8 +11069,8 @@ static void test_docfrag(IHTMLDocument2 *doc) col = NULL; hres = IHTMLDocument2_get_all(frag, &col); - todo_wine ok(hres == S_OK, "get_all failed: %08lx\n", hres); - todo_wine ok(col != NULL, "got null elements collection\n"); + ok(hres == S_OK, "get_all failed: %08lx\n", hres); + ok(col != NULL, "got null elements collection\n"); if (col) { test_elem_collection((IUnknown *) col, frag_types, ARRAY_SIZE(frag_types)); IHTMLElementCollection_Release(col); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1918
From: Santino Mazza <smazza(a)codeweavers.com> --- dlls/mshtml/htmldoc.c | 11 +++++++++++ dlls/mshtml/tests/dom.c | 2 +- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/dlls/mshtml/htmldoc.c b/dlls/mshtml/htmldoc.c index ac471e9e1a8..38feb2839f1 100644 --- a/dlls/mshtml/htmldoc.c +++ b/dlls/mshtml/htmldoc.c @@ -537,6 +537,17 @@ static HRESULT WINAPI HTMLDocument_get_body(IHTMLDocument2 *iface, IHTMLElement TRACE("Could not get body: %08lx\n", nsres); return E_UNEXPECTED; } + }else { + nsAString nsnode_name; + nsIDOMDocumentFragment *frag; + + hres = nsIDOMNode_QueryInterface(This->node.nsnode, &IID_nsIDOMDocumentFragment, (void**)&frag); + if(hres == S_OK) { + nsAString_InitDepend(&nsnode_name, L"BODY"); + nsIDOMDocumentFragment_QuerySelector(frag, &nsnode_name, (nsIDOMElement **) &nsbody); + nsAString_Finish(&nsnode_name); + nsIDOMDocumentFragment_Release(frag); + } } if(!nsbody) { diff --git a/dlls/mshtml/tests/dom.c b/dlls/mshtml/tests/dom.c index a726801a43e..eea3dbe7126 100644 --- a/dlls/mshtml/tests/dom.c +++ b/dlls/mshtml/tests/dom.c @@ -11063,7 +11063,7 @@ static void test_docfrag(IHTMLDocument2 *doc) hres = IHTMLDocument2_get_body(frag, &body); ok(hres == S_OK, "get_body failed: %08lx\n", hres); - todo_wine ok(body != NULL, "body == NULL\n"); + ok(body != NULL, "body == NULL\n"); if (body) IHTMLElement_Release(body); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1918
Down-casting in contrast is totally fine, since it inherits from nsIDOMNode. But the other way around isn't.
Do you mean up-casting (casting from a more specific type to a less specific type)? Because I'm about to leave a review comment about down-casting (casting from a less specific type to a more specific type). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1918#note_20495
Jinoh Kang (@iamahuman) commented about dlls/mshtml/htmldoc.c:
TRACE("Could not get body: %08lx\n", nsres); return E_UNEXPECTED; } + }else { + nsAString nsnode_name; + nsIDOMDocumentFragment *frag; + + hres = nsIDOMNode_QueryInterface(This->node.nsnode, &IID_nsIDOMDocumentFragment, (void**)&frag); + if(hres == S_OK) { + nsAString_InitDepend(&nsnode_name, L"BODY"); + nsIDOMDocumentFragment_QuerySelector(frag, &nsnode_name, (nsIDOMElement **) &nsbody);
Do we want to use QueryInterface here too? cc/ @insn -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1918#note_20496
On Sat Jan 7 04:18:20 2023 +0000, Jinoh Kang wrote:
Down-casting in contrast is totally fine, since it inherits from nsIDOMNode. But the other way around isn't. Do you mean up-casting (casting from a more specific type to a less specific type)? Because I'm about to leave a review comment about down-casting (casting from a less specific type to a more specific type). Yes, sorry, mixed the terms up.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1918#note_20499
On Sat Jan 7 04:19:14 2023 +0000, Jinoh Kang wrote:
Do we want to use QueryInterface here too? cc/ @insn In this case, it's not necessary, we are actually only using it as an element, even later it's up-casted to element. If anything, I think the variable should be an element to begin with (this would move the cast from get_element to GetBody, and get rid of this cast), but I don't think it's worth it as a change. Depends how Jacek feels.
BTW I just realized here it's using hres. This is a nitpick, of course, but it should be using a `nsresult nsres` and check for `NS_OK` instead of `S_OK`, it's more correct code (even though it compiles to the same thing). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1918#note_20500
Jacek Caban (@jacek) commented about dlls/mshtml/tests/dom.c:
+ div = test_create_elem(doc, L"DIV"); + test_elem_source_index(div, -1); + test_node_append_child((IUnknown*)html, (IUnknown*)div); + test_elem_source_index(div, 2); + + body = test_create_elem(doc, L"BODY"); + test_elem_source_index(body, -1); + test_node_append_child((IUnknown*)div, (IUnknown*)body); + test_elem_source_index(body, 3); + IHTMLElement_Release(body); + IHTMLElement_Release(div); + IHTMLElement_Release(html); + + hres = IHTMLDocument2_get_body(frag, &body); + ok(hres == S_OK, "get_body failed: %08lx\n", hres); + todo_wine ok(body != NULL, "body == NULL\n"); It would be interesting to compare body to previously created element to make sure it's the same object. This will probably need `iface_cmp` (native seems to use some relay interfaces, we don't have that).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1918#note_20567
Jacek Caban (@jacek) commented about dlls/mshtml/htmldoc.c:
TRACE("(%p)->(%p)\n", This, p);
if(!This->dom_document) { - WARN("NULL dom_document\n"); - return E_UNEXPECTED; + *p = create_all_collection(&This->node, FALSE); + return S_OK; }
Could we just unconditionally use this code path (even if we have dom_document)? It seems to me that it should just always work. This would change handling empty collection (we currently return NULL in get_all), but that could maybe be handled in create_all_collection (although some tests would be nice to confirm it). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1918#note_20566
If anything, I think the variable should be an element to begin with (this would move the cast from get_element to GetBody, and get rid of this cast), but I don't think it's worth it as a change. Depends how Jacek feels.
Yes, I think it would be better to change variable type. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1918#note_20568
On Mon Jan 9 13:59:56 2023 +0000, Jacek Caban wrote:
If anything, I think the variable should be an element to begin with (this would move the cast from get_element to GetBody, and get rid of this cast), but I don't think it's worth it as a change. Depends how Jacek feels. Yes, I think it would be better to change variable type. If I change the variable type to nsIDOMElement I will have to down-cast nsbody to nsIDOMHTMLElement for GetBody, I suppose in this case is not really a problem because we are assigning it a new object and not using it. But when I have to Release the element, is it okay to use nsIDOMElement_Release(nsbody)? Because I created the nsbody using GetBody which returns an nsIDOMHTMLElement, and even though the type is nsIDOMElement, at the end it's an nsIDOMHTMLElement, so I'm not sure if we could be leaking something.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1918#note_20573
On Mon Jan 9 13:57:34 2023 +0000, Jacek Caban wrote:
Could we just unconditionally use this code path (even if we have dom_document)? It seems to me that it should just always work. This would change handling empty collection (we currently return NULL in get_all), but that could maybe be handled in create_all_collection (although some tests would be nice to confirm it). I tested it, and it works, I think this functions should be like get_body, that it always returns S_OK and when it fails it returns a NULL body. Not sure how can we test that though, the only test I get in my mind is to test if it fails with an empty document.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1918#note_20577
On Mon Jan 9 17:37:16 2023 +0000, Santino Mazza wrote:
If I change the variable type to nsIDOMElement I will have to down-cast nsbody to nsIDOMHTMLElement for GetBody, I suppose in this case is not really a problem because we are assigning it a new object and not using it. But when I have to Release the element, is it okay to use nsIDOMElement_Release(nsbody)? Because I created the nsbody using GetBody which returns an nsIDOMHTMLElement, and even though the type is nsIDOMElement, at the end it's an nsIDOMHTMLElement, so I'm not sure if we could be leaking something. nsIDOMHTMLElement inherits from nsIDOMElement so it's perfectly fine. Also, they all inherit from nsISupports (which is basically IUnknown), anyway. You can quickly see this in `nsiface.idl`.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1918#note_20579
participants (5)
-
Gabriel Ivăncescu -
Jacek Caban (＠jacek)
-
Jinoh Kang (＠iamahuman)
-
Santino Mazza
-
Santino Mazza (＠tati1454)