[PATCH 0/5] MR6850: mshtml: Expose the props properly for Node prototype chain.
This is just nodes for now. The tests in the last patch may seem a bit overkill now, but it will be used for all the other prototypes, so they'll only need extra entries in the array/table, making it way simpler and less redundant. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6850
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Since it's used when filling the props and by getOwnPropertyNames which includes non-enumerable props. Enumeration on jscript side should still filter the props correctly, if needed. Note that we have to only enumerate those that are found on the actual object, not its prototypes. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/dispex.c | 15 +++++++++------ dlls/mshtml/htmlelem.c | 2 +- dlls/mshtml/mshtml_private.h | 2 +- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/dlls/mshtml/dispex.c b/dlls/mshtml/dispex.c index 65c54ed8aec..f69f1fe242c 100644 --- a/dlls/mshtml/dispex.c +++ b/dlls/mshtml/dispex.c @@ -52,9 +52,10 @@ typedef struct { typedef struct { DISPID id; - BSTR name; tid_t tid; + BSTR name; dispex_hook_invoke_t hook; + BOOLEAN on_prototype; SHORT call_vtbl_off; SHORT put_vtbl_off; SHORT get_vtbl_off; @@ -594,8 +595,10 @@ static dispex_data_t *preprocess_dispex_data(dispex_static_data_t *desc, compat_ data->name_table = malloc(data->func_cnt * sizeof(func_info_t*)); for(i=0; i < data->func_cnt; i++) { /* Don't expose properties that are exposed by object's prototype */ - if(find_prototype_member(data, data->funcs[i].id)) + if(find_prototype_member(data, data->funcs[i].id)) { + data->funcs[i].on_prototype = TRUE; continue; + } data->name_table[data->name_cnt++] = data->funcs+i; } qsort(data->name_table, data->name_cnt, sizeof(func_info_t*), func_name_cmp); @@ -2455,7 +2458,7 @@ static HRESULT next_dynamic_id(DispatchEx *dispex, DWORD idx, DISPID *ret_id) return S_OK; } -HRESULT dispex_next_id(DispatchEx *dispex, DISPID id, DISPID *ret) +HRESULT dispex_next_id(DispatchEx *dispex, DISPID id, BOOL enum_all_own_props, DISPID *ret) { func_info_t *func; HRESULT hres; @@ -2480,7 +2483,7 @@ HRESULT dispex_next_id(DispatchEx *dispex, DISPID id, DISPID *ret) } while(func < dispex->info->funcs + dispex->info->func_cnt) { - if(func->func_disp_idx == -1) { + if(enum_all_own_props ? (!func->on_prototype) : (func->func_disp_idx == -1)) { *ret = func->id; return S_OK; } @@ -2513,7 +2516,7 @@ static HRESULT WINAPI DispatchEx_GetNextDispID(IWineJSDispatchHost *iface, DWORD return E_OUTOFMEMORY; if(This->jsdisp) return IWineJSDispatch_GetNextDispID(This->jsdisp, grfdex, id, pid); - return dispex_next_id(This, id, pid); + return dispex_next_id(This, id, FALSE, pid); } static HRESULT WINAPI DispatchEx_GetNameSpaceParent(IWineJSDispatchHost *iface, IUnknown **ppunk) @@ -2616,7 +2619,7 @@ static HRESULT WINAPI JSDispatchHost_NextProperty(IWineJSDispatchHost *iface, DI TRACE("%s (%p)->(%lx)\n", This->info->name, This, id); - hres = dispex_next_id(This, id, &next); + hres = dispex_next_id(This, id, TRUE, &next); if(hres != S_OK) return hres; diff --git a/dlls/mshtml/htmlelem.c b/dlls/mshtml/htmlelem.c index 0cc41bf41a3..72aeaae816e 100644 --- a/dlls/mshtml/htmlelem.c +++ b/dlls/mshtml/htmlelem.c @@ -7513,7 +7513,7 @@ static HRESULT get_attr_dispid_by_relative_idx(HTMLAttributeCollection *This, LO FIXME("filter non-enumerable attributes out\n"); while(1) { - hres = dispex_next_id(&This->elem->node.event_target.dispex, id, &id); + hres = dispex_next_id(&This->elem->node.event_target.dispex, id, FALSE, &id); if(FAILED(hres)) return hres; else if(hres == S_FALSE) diff --git a/dlls/mshtml/mshtml_private.h b/dlls/mshtml/mshtml_private.h index 591bb3eddd1..11ee0afd3c7 100644 --- a/dlls/mshtml/mshtml_private.h +++ b/dlls/mshtml/mshtml_private.h @@ -631,7 +631,7 @@ HRESULT dispex_prop_put(DispatchEx *dispex, DISPID id, LCID lcid, VARIANT *v, EX IServiceProvider *caller); HRESULT dispex_get_chain_builtin_id(DispatchEx *dispex, const WCHAR *name, DWORD flags, DISPID *pid); HRESULT dispex_get_id(DispatchEx *dispex, const WCHAR *name, DWORD flags, DISPID *pid); -HRESULT dispex_next_id(DispatchEx *dispex, DISPID id, DISPID *ret); +HRESULT dispex_next_id(DispatchEx *dispex, DISPID id, BOOL enum_all_own_props, DISPID *ret); HRESULT dispex_prop_name(DispatchEx *dispex, DISPID id, BSTR *ret); HRESULT dispex_define_property(DispatchEx *dispex, const WCHAR *name, DWORD flags, VARIANT *v, DISPID *id); HRESULT dispex_index_prop_desc(DispatchEx*,DISPID,struct property_info*); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6850
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> For some reason it's exposed higher up the chain in various other prototypes, but not the doctype. Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/htmldoc.c | 4 +--- dlls/mshtml/htmlnode.c | 13 ++++++++----- dlls/mshtml/tests/documentmode.js | 23 +++++++++++++++++++++++ 3 files changed, 32 insertions(+), 8 deletions(-) diff --git a/dlls/mshtml/htmldoc.c b/dlls/mshtml/htmldoc.c index 70bc028f3a0..e8470c0d4cb 100644 --- a/dlls/mshtml/htmldoc.c +++ b/dlls/mshtml/htmldoc.c @@ -316,9 +316,6 @@ static const event_target_vtbl_t DocumentType_event_target_vtbl = { static const tid_t DocumentType_iface_tids[] = { IDOMDocumentType_tid, - IHTMLDOMNode_tid, - IHTMLDOMNode2_tid, - IHTMLDOMNode3_tid, 0 }; @@ -328,6 +325,7 @@ dispex_static_data_t DocumentType_dispex = { .vtbl = &DocumentType_event_target_vtbl.dispex_vtbl, .disp_tid = DispDOMDocumentType_tid, .iface_tids = DocumentType_iface_tids, + .init_info = HTMLDOMNode_init_dispex_info, }; HRESULT create_doctype_node(HTMLDocumentNode *doc, nsIDOMNode *nsnode, HTMLDOMNode **ret) diff --git a/dlls/mshtml/htmlnode.c b/dlls/mshtml/htmlnode.c index 7fad8f22357..64a5b6c85ea 100644 --- a/dlls/mshtml/htmlnode.c +++ b/dlls/mshtml/htmlnode.c @@ -24,6 +24,7 @@ #include "winbase.h" #include "winuser.h" #include "ole2.h" +#include "mshtmdid.h" #include "wine/debug.h" @@ -1252,6 +1253,13 @@ static HRESULT HTMLDOMNode_clone(HTMLDOMNode *This, nsIDOMNode *nsnode, HTMLDOMN void HTMLDOMNode_init_dispex_info(dispex_data_t *info, compat_mode_t mode) { + static const dispex_hook_t ie9_hooks[] = { + {DISPID_IHTMLDOMNODE_REMOVENODE, NULL}, + {DISPID_UNKNOWN} + }; + + dispex_info_add_interface(info, IHTMLDOMNode_tid, mode >= COMPAT_MODE_IE9 ? ie9_hooks : NULL); + if(mode >= COMPAT_MODE_IE9) dispex_info_add_interface(info, IHTMLDOMNode3_tid, NULL); @@ -1293,15 +1301,10 @@ static const dispex_static_data_vtbl_t Node_dispex_vtbl = { .unlink = HTMLDOMNode_unlink }; -static const tid_t HTMLDOMNode_iface_tids[] = { - IHTMLDOMNode_tid, - 0 -}; dispex_static_data_t Node_dispex = { .id = PROT_Node, .vtbl = &Node_dispex_vtbl, .disp_tid = IHTMLDOMNode_tid, - .iface_tids = HTMLDOMNode_iface_tids, .init_info = HTMLDOMNode_init_dispex_info, }; diff --git a/dlls/mshtml/tests/documentmode.js b/dlls/mshtml/tests/documentmode.js index 0ee20de7a90..91e5ad1c2fb 100644 --- a/dlls/mshtml/tests/documentmode.js +++ b/dlls/mshtml/tests/documentmode.js @@ -512,6 +512,7 @@ sync_test("elem_props", function() { test_exposed("readyState", v < 11); test_exposed("clientTop", true); test_exposed("title", true); + test_exposed("removeNode", true); test_exposed("querySelectorAll", v >= 8); test_exposed("textContent", v >= 9); test_exposed("prefix", v >= 9); @@ -586,6 +587,27 @@ sync_test("docfrag_props", function() { test_exposed("compareDocumentPosition", v >= 9); }); +sync_test("textnode_props", function() { + var node = document.createTextNode("testNode"); + + function test_exposed(prop, expect) { + if(expect) + ok(prop in node, prop + " not found in text node."); + else + ok(!(prop in node), prop + " found in text node."); + } + + var v = document.documentMode; + + test_exposed("childNodes", true); + test_exposed("nodeName", true); + test_exposed("nodeValue", true); + test_exposed("removeNode", true); + test_exposed("compareDocumentPosition", v >= 9); + test_exposed("isEqualNode", v >= 9); + test_exposed("prefix", v >= 9); +}); + sync_test("window_props", function() { function test_exposed(prop, expect, is_todo) { var ok_ = is_todo ? todo_wine.ok : ok; @@ -1180,6 +1202,7 @@ sync_test("doctype", function() { } ok(doctype.name === "html", "doctype.name = " + doctype.name); + ok(!("removeNode" in doctype), "removeNode found in doctype."); }); async_test("iframe_doc_mode", function() { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6850
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/htmlnode.c | 1 + dlls/mshtml/tests/documentmode.js | 3 +++ 2 files changed, 4 insertions(+) diff --git a/dlls/mshtml/htmlnode.c b/dlls/mshtml/htmlnode.c index 64a5b6c85ea..b4ad9d6861c 100644 --- a/dlls/mshtml/htmlnode.c +++ b/dlls/mshtml/htmlnode.c @@ -1255,6 +1255,7 @@ void HTMLDOMNode_init_dispex_info(dispex_data_t *info, compat_mode_t mode) { static const dispex_hook_t ie9_hooks[] = { {DISPID_IHTMLDOMNODE_REMOVENODE, NULL}, + {DISPID_IHTMLDOMNODE_REPLACENODE, NULL}, {DISPID_UNKNOWN} }; diff --git a/dlls/mshtml/tests/documentmode.js b/dlls/mshtml/tests/documentmode.js index 91e5ad1c2fb..8d1720f75bd 100644 --- a/dlls/mshtml/tests/documentmode.js +++ b/dlls/mshtml/tests/documentmode.js @@ -513,6 +513,7 @@ sync_test("elem_props", function() { test_exposed("clientTop", true); test_exposed("title", true); test_exposed("removeNode", true); + test_exposed("replaceNode", true); test_exposed("querySelectorAll", v >= 8); test_exposed("textContent", v >= 9); test_exposed("prefix", v >= 9); @@ -603,6 +604,7 @@ sync_test("textnode_props", function() { test_exposed("nodeName", true); test_exposed("nodeValue", true); test_exposed("removeNode", true); + test_exposed("replaceNode", true); test_exposed("compareDocumentPosition", v >= 9); test_exposed("isEqualNode", v >= 9); test_exposed("prefix", v >= 9); @@ -1203,6 +1205,7 @@ sync_test("doctype", function() { ok(doctype.name === "html", "doctype.name = " + doctype.name); ok(!("removeNode" in doctype), "removeNode found in doctype."); + ok(!("replaceNode" in doctype), "replaceNode found in doctype."); }); async_test("iframe_doc_mode", function() { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6850
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/htmlnode.c | 1 + dlls/mshtml/tests/documentmode.js | 3 +++ 2 files changed, 4 insertions(+) diff --git a/dlls/mshtml/htmlnode.c b/dlls/mshtml/htmlnode.c index b4ad9d6861c..703bbd43262 100644 --- a/dlls/mshtml/htmlnode.c +++ b/dlls/mshtml/htmlnode.c @@ -1256,6 +1256,7 @@ void HTMLDOMNode_init_dispex_info(dispex_data_t *info, compat_mode_t mode) static const dispex_hook_t ie9_hooks[] = { {DISPID_IHTMLDOMNODE_REMOVENODE, NULL}, {DISPID_IHTMLDOMNODE_REPLACENODE, NULL}, + {DISPID_IHTMLDOMNODE_SWAPNODE, NULL}, {DISPID_UNKNOWN} }; diff --git a/dlls/mshtml/tests/documentmode.js b/dlls/mshtml/tests/documentmode.js index 8d1720f75bd..49b8a0656fb 100644 --- a/dlls/mshtml/tests/documentmode.js +++ b/dlls/mshtml/tests/documentmode.js @@ -514,6 +514,7 @@ sync_test("elem_props", function() { test_exposed("title", true); test_exposed("removeNode", true); test_exposed("replaceNode", true); + test_exposed("swapNode", true); test_exposed("querySelectorAll", v >= 8); test_exposed("textContent", v >= 9); test_exposed("prefix", v >= 9); @@ -605,6 +606,7 @@ sync_test("textnode_props", function() { test_exposed("nodeValue", true); test_exposed("removeNode", true); test_exposed("replaceNode", true); + test_exposed("swapNode", true); test_exposed("compareDocumentPosition", v >= 9); test_exposed("isEqualNode", v >= 9); test_exposed("prefix", v >= 9); @@ -1206,6 +1208,7 @@ sync_test("doctype", function() { ok(doctype.name === "html", "doctype.name = " + doctype.name); ok(!("removeNode" in doctype), "removeNode found in doctype."); ok(!("replaceNode" in doctype), "replaceNode found in doctype."); + ok(!("swapNode" in doctype), "swapNode found in doctype."); }); async_test("iframe_doc_mode", function() { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6850
From: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> Signed-off-by: Gabriel Ivăncescu <gabrielopcode(a)gmail.com> --- dlls/mshtml/htmlnode.c | 4 +- dlls/mshtml/tests/documentmode.js | 77 ++++++++++++++++++++++++++++++- 2 files changed, 79 insertions(+), 2 deletions(-) diff --git a/dlls/mshtml/htmlnode.c b/dlls/mshtml/htmlnode.c index 703bbd43262..7df7b5bdc6a 100644 --- a/dlls/mshtml/htmlnode.c +++ b/dlls/mshtml/htmlnode.c @@ -1262,8 +1262,10 @@ void HTMLDOMNode_init_dispex_info(dispex_data_t *info, compat_mode_t mode) dispex_info_add_interface(info, IHTMLDOMNode_tid, mode >= COMPAT_MODE_IE9 ? ie9_hooks : NULL); - if(mode >= COMPAT_MODE_IE9) + if(mode >= COMPAT_MODE_IE9) { + dispex_info_add_interface(info, IHTMLDOMNode2_tid, NULL); dispex_info_add_interface(info, IHTMLDOMNode3_tid, NULL); + } EventTarget_init_dispex_info(info, mode); } diff --git a/dlls/mshtml/tests/documentmode.js b/dlls/mshtml/tests/documentmode.js index 49b8a0656fb..d2db9d0fd20 100644 --- a/dlls/mshtml/tests/documentmode.js +++ b/dlls/mshtml/tests/documentmode.js @@ -604,6 +604,7 @@ sync_test("textnode_props", function() { test_exposed("childNodes", true); test_exposed("nodeName", true); test_exposed("nodeValue", true); + test_exposed("ownerDocument", true); test_exposed("removeNode", true); test_exposed("replaceNode", true); test_exposed("swapNode", true); @@ -3381,7 +3382,7 @@ sync_test("form", function() { }); sync_test("prototypes", function() { - var v = document.documentMode; + var v = document.documentMode, i, j, k; if(v < 9) return; @@ -3595,4 +3596,78 @@ sync_test("prototypes", function() { check(Attr.prototype, Node.prototype, "attr prototype"); check(document.createDocumentFragment(), DocumentFragment.prototype, "fragment"); check(DocumentFragment.prototype, Node.prototype, "fragment prototype"); + + var props = [ + [ "CSSStyleRule", [ "readOnly", "selectorText", "style" ] ], + [ "CustomEvent", [ "detail", "initCustomEvent" ] ], + [ "DocumentType", [ "entities", "internalSubset", "name", "notations", "publicId", "systemId" ] ], + [ "Event", [ + ["AT_TARGET",true], ["BUBBLING_PHASE",true], ["CAPTURING_PHASE",true], + "bubbles", "cancelBubble", "cancelable", "currentTarget", "defaultPrevented", "eventPhase", "initEvent", "isTrusted", + "preventDefault", "srcElement", "stopImmediatePropagation", "stopPropagation", "target", "timeStamp", "type" + ] ], + [ "HTMLUnknownElement", [ "namedRecordset", "recordset" ] ], + [ "MessageEvent", [ "data", "initMessageEvent", "origin", ["ports",10,v >= 10], "source" ] ], + [ "MouseEvent", [ + "altKey", "button", "buttons", "clientX", "clientY", "ctrlKey", "fromElement", "getModifierState", + "initMouseEvent", "layerX", "layerY", "metaKey", "offsetX", "offsetY", "pageX", "pageY", "relatedTarget", + "screenX", "screenY", "shiftKey", "toElement", "which", "x", "y" + ] ], + [ "Node", [ + ["ATTRIBUTE_NODE",true], ["CDATA_SECTION_NODE",true], ["COMMENT_NODE",true], ["DOCUMENT_FRAGMENT_NODE",true], + ["DOCUMENT_NODE",true], ["DOCUMENT_POSITION_CONTAINED_BY",true], ["DOCUMENT_POSITION_CONTAINS",true], + ["DOCUMENT_POSITION_DISCONNECTED",true], ["DOCUMENT_POSITION_FOLLOWING",true], + ["DOCUMENT_POSITION_IMPLEMENTATION_SPECIFIC",true], ["DOCUMENT_POSITION_PRECEDING",true], + ["DOCUMENT_TYPE_NODE",true], ["ELEMENT_NODE",true], ["ENTITY_NODE",true], ["ENTITY_REFERENCE_NODE",true], + ["NOTATION_NODE",true], ["PROCESSING_INSTRUCTION_NODE",true], ["TEXT_NODE",true], + "addEventListener", "appendChild", "attributes", "childNodes", "cloneNode", "compareDocumentPosition", + "dispatchEvent", "firstChild", ["hasAttributes",true], "hasChildNodes", "insertBefore", "isDefaultNamespace", + "isEqualNode", "isSameNode", "isSupported", "lastChild", "localName", "lookupNamespaceURI", "lookupPrefix", + "namespaceURI", "nextSibling", "nodeName", "nodeType", "nodeValue", ["normalize",true], "ownerDocument", + "parentNode", "prefix", "previousSibling", "removeChild", "removeEventListener", "replaceChild", "textContent" + ] ], + [ "StorageEvent", [ "initStorageEvent", "key", "newValue", "oldValue", "storageArea", "url" ] ], + [ "UIEvent", [ "detail", "initUIEvent", "view" ] ] + ]; + + for(i = 0; i < props.length; i++) { + var proto = props[i][0], seen_constructor = false; + var expected = props[i][1], enumerated = Object.getOwnPropertyNames(window[proto].prototype).sort(); + + for(k = 0, j = 0; j < expected.length; j++) { + var name = expected[j], minv = 0, maxv = 11, ok_ = ok; + if(Array.isArray(name)) { + if(typeof(name[1]) === "number") { + minv = name[1]; + if(name.length > 2 && typeof(name[2]) === "number") + maxv = name[2]; + } + if(typeof(name[name.length-1]) === "boolean" && name[name.length-1]) + ok_ = todo_wine.ok; + name = name[0]; + } + if(enumerated[k] === "constructor") { + seen_constructor = true; + k++; + } + while(enumerated[k] < name && k < enumerated.length) { + ok(false, enumerated[k] + " is a prop of " + proto + ".prototype"); + k++; + } + if(enumerated[k] !== name) + ok_(v < minv || v > maxv, name + " not a prop of " + proto + ".prototype"); + else { + ok_(v >= minv && v <= maxv, name + " is a prop of " + proto + ".prototype"); + k++; + } + } + while(k < enumerated.length) { + if(enumerated[k] === "constructor") + seen_constructor = true; + else + ok(false, enumerated[k] + " is a prop of " + proto + ".prototype"); + k++; + } + if(broken(true) /* todo_wine */) ok(seen_constructor, "constructor not a prop of " + proto + ".prototype"); + } }); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6850
Jacek Caban (@jacek) commented about dlls/mshtml/htmlnode.c:
dispex_info_add_interface(info, IHTMLDOMNode_tid, mode >= COMPAT_MODE_IE9 ? ie9_hooks : NULL);
- if(mode >= COMPAT_MODE_IE9) + if(mode >= COMPAT_MODE_IE9) { + dispex_info_add_interface(info, IHTMLDOMNode2_tid, NULL);
You're removing it for document type nodes in earlier commit, creating a temporary regression until this commit. Overall, I'd prefer the test part to be way sooner in the series, ideally before changing `Node` prototype object. Unless I'm missing something, it should be trivial to do with `todo_wine`s. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6850#note_88049
Jacek Caban (@jacek) commented about dlls/mshtml/tests/documentmode.js:
check(Attr.prototype, Node.prototype, "attr prototype"); check(document.createDocumentFragment(), DocumentFragment.prototype, "fragment"); check(DocumentFragment.prototype, Node.prototype, "fragment prototype"); + + var props = [ + [ "CSSStyleRule", [ "readOnly", "selectorText", "style" ] ],
Why is it a string instead of just `CSSStyleRule` or `CSSStyleRule.prototype`? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6850#note_88050
Jacek Caban (@jacek) commented about dlls/mshtml/tests/documentmode.js:
+ ["DOCUMENT_NODE",true], ["DOCUMENT_POSITION_CONTAINED_BY",true], ["DOCUMENT_POSITION_CONTAINS",true], + ["DOCUMENT_POSITION_DISCONNECTED",true], ["DOCUMENT_POSITION_FOLLOWING",true], + ["DOCUMENT_POSITION_IMPLEMENTATION_SPECIFIC",true], ["DOCUMENT_POSITION_PRECEDING",true], + ["DOCUMENT_TYPE_NODE",true], ["ELEMENT_NODE",true], ["ENTITY_NODE",true], ["ENTITY_REFERENCE_NODE",true], + ["NOTATION_NODE",true], ["PROCESSING_INSTRUCTION_NODE",true], ["TEXT_NODE",true], + "addEventListener", "appendChild", "attributes", "childNodes", "cloneNode", "compareDocumentPosition", + "dispatchEvent", "firstChild", ["hasAttributes",true], "hasChildNodes", "insertBefore", "isDefaultNamespace", + "isEqualNode", "isSameNode", "isSupported", "lastChild", "localName", "lookupNamespaceURI", "lookupPrefix", + "namespaceURI", "nextSibling", "nodeName", "nodeType", "nodeValue", ["normalize",true], "ownerDocument", + "parentNode", "prefix", "previousSibling", "removeChild", "removeEventListener", "replaceChild", "textContent" + ] ], + [ "StorageEvent", [ "initStorageEvent", "key", "newValue", "oldValue", "storageArea", "url" ] ], + [ "UIEvent", [ "detail", "initUIEvent", "view" ] ] + ]; + + for(i = 0; i < props.length; i++) { I think that it would be useful to move content of this loop in a helper, something like `test_own_props` will likely be useful for other objects too.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6850#note_88051
Jacek Caban (@jacek) commented about dlls/mshtml/tests/documentmode.js:
+ } + if(enumerated[k] !== name) + ok_(v < minv || v > maxv, name + " not a prop of " + proto + ".prototype"); + else { + ok_(v >= minv && v <= maxv, name + " is a prop of " + proto + ".prototype"); + k++; + } + } + while(k < enumerated.length) { + if(enumerated[k] === "constructor") + seen_constructor = true; + else + ok(false, enumerated[k] + " is a prop of " + proto + ".prototype"); + k++; + } + if(broken(true) /* todo_wine */) ok(seen_constructor, "constructor not a prop of " + proto + ".prototype"); That's `todo_wine_if(!seen_constructor).ok( ... )`.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6850#note_88052
On Tue Nov 19 17:50:54 2024 +0000, Jacek Caban wrote:
I think that it would be useful to move content of this loop in a helper, something like `test_own_props` will likely be useful for other objects too. What should I do about the constructor check? Since other objects won't have it.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6850#note_88053
On Tue Nov 19 17:50:54 2024 +0000, Jacek Caban wrote:
Why is it a string instead of just `CSSStyleRule` or `CSSStyleRule.prototype`? Some others aren't available in other compat modes, so this would throw an error in such case.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6850#note_88054
On Tue Nov 19 17:50:54 2024 +0000, Jacek Caban wrote:
You're removing it for document type nodes in earlier commit, creating a temporary regression until this commit. Overall, I'd prefer the test part to be way sooner in the series, ideally before changing `Node` prototype object. Unless I'm missing something, it should be trivial to do with `todo_wine`s. I'm not sure I can add the tests too early, since I can't add todo wines on extra props exposed by wine (those aren't even in the array and would fail on Windows if they were), but only on props wine does not expose. But good point about the regression.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6850#note_88055
On Tue Nov 19 18:36:53 2024 +0000, Gabriel Ivăncescu wrote:
I'm not sure I can add the tests too early, since I can't add todo wines on extra props exposed by wine (those aren't even in the array and would fail on Windows if they were), but only on props wine does not expose. But good point about the regression. What i mean is, if wine exposes (wrongly) "foobar" from an object I can't make that todo_wine, but I can if Windows exposes "barfoo" while wine does not.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6850#note_88056
On Tue Nov 19 18:37:24 2024 +0000, Gabriel Ivăncescu wrote:
What i mean is, if wine exposes (wrongly) "foobar" from an object I can't make that todo_wine, but I can if Windows exposes "barfoo" while wine does not. Being able to express that `todo_wine` sounds useful then. I guess that would be a reason to decouple property array from todos. It would mean passing twice properties that are missing in Wine, but that doesn't seem too bad, esp. if we could use it to decrypt the test. One way I could imagine it would be to have a global helper that's not at all specific to prototypes (so no special "constructor" property handling):
``` function test_own_props(obj, props, todos) { // we can just use something like todo_wine_if(todos && todos.indexOf(prop_name) != 1) } ``` Move all your test to separated tests, "prototypes" is long enough. Then define a local helper: ``` function check(constr, props, todos) { // a hack really, but also an interesting test on its own; we don't need todo_wine for constructor property after it ok(constr.prototype.constructor === constr, "..."); props.push("constructor"); // concatenate arrays from arguments[3..arguments.length-1]; this gives a convenient way to handle version specific props test_own_props(constr.prototype, props, todos); } ``` The test would be then a series of calls, like: ``` check(CustomEvent, ["detail", "initCustomEvent"]); check(MessageEvent, ["data", "initMessageEvent", "origin", "source"], null, v >= 10 ? ["ports"] : null); check(Evemt, ["AT_TARGET", ...], ["AT_TARGET", ...]); ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6850#note_88061
On Tue Nov 19 18:29:40 2024 +0000, Gabriel Ivăncescu wrote:
What should I do about the constructor check? Since other objects won't have it. Ideally handle it in prototype-specific code, see my example.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6850#note_88063
On Tue Nov 19 18:32:52 2024 +0000, Gabriel Ivăncescu wrote:
Some others aren't available in other compat modes, so this would throw an error in such case. With the example above, that would be
if(v >= ...) check(...);
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6850#note_88062
On Tue Nov 19 19:34:48 2024 +0000, Jacek Caban wrote:
Being able to express that `todo_wine` sounds useful then. I guess that would be a reason to decouple property array from todos. It would mean passing twice properties that are missing in Wine, but that doesn't seem too bad, esp. if we could use it to decrypt the test. One way I could imagine it would be to have a global helper that's not at all specific to prototypes (so no special "constructor" property handling): ``` function test_own_props(obj, props, todos) { // we can just use something like todo_wine_if(todos && todos.indexOf(prop_name) != 1) } ``` Move all your test to separated tests, "prototypes" is long enough. Then define a local helper: ``` function check(constr, props, todos) { // a hack really, but also an interesting test on its own; we don't need todo_wine for constructor property after it ok(constr.prototype.constructor === constr, "..."); props.push("constructor"); // concatenate arrays from arguments[3..arguments.length-1]; this gives a convenient way to handle version specific props test_own_props(constr.prototype, props, todos); } ``` The test would be then a series of calls, like: ``` check(CustomEvent, ["detail", "initCustomEvent"]); check(MessageEvent, ["data", "initMessageEvent", "origin", "source"], null, v >= 10 ? ["ports"] : null); check(Evemt, ["AT_TARGET", ...], ["AT_TARGET", ...]); ``` I replied too soon, for version-specific cases, it may be more convenient to just do it in-place instead of the concatenation above, that would be:
check(MessageEvent, ["data", "initMessageEvent", "origin", v >= 10 ? "ports" : null, "source"]);
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6850#note_88064
On Tue Nov 19 19:39:13 2024 +0000, Jacek Caban wrote:
I replied too soon, for version-specific cases, it may be more convenient to just do it in-place instead of the concatenation above, that would be: ``` check(MessageEvent, ["data", "initMessageEvent", "origin", v >= 10 ? "ports" : null, "source"]); ``` OK i will try to split this into functions, but ideally I'll keep the version check in the array/data because some objects will have a lot of them (some are specific to some IE versions only too, like IE9-only), and this will balloon up the test and make it worse, IMO.
For the "todos" arg I only include the extra props that wine exposes, right? The others I can keep using existing logic. I mean I have to separate them somehow, because for the extra-wine-props I must not test them on Windows, but the others exist on Windows. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6850#note_88068
On Tue Nov 19 19:53:22 2024 +0000, Gabriel Ivăncescu wrote:
OK i will try to split this into functions, but ideally I'll keep the version check in the array/data because some objects will have a lot of them (some are specific to some IE versions only too, like IE9-only), and this will balloon up the test and make it worse, IMO. For the "todos" arg I only include the extra props that wine exposes, right? The others I can keep using existing logic. I mean I have to separate them somehow, because for the extra-wine-props I must not test them on Windows, but the others exist on Windows. I guess it depends on the actual implementation, but I'd expect a single way of handling todos (that is replacing your current logic instead of having two mechanisms for the same thing) to be nicer.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6850#note_88069
On Tue Nov 19 19:58:27 2024 +0000, Jacek Caban wrote:
I guess it depends on the actual implementation, but I'd expect a single way of handling todos (that is replacing your current logic instead of having two mechanisms for the same thing) to be nicer. Well I can't see how it can work without two mechanisms (meaning, separated) because one tests lack of prop on wine (but found on Windows), and one tests extra prop on wine (but not found on Windows). Just think from Windows' perspective: the former must be tested (because it exists on Windows), but the latter either must not be tested, or tested for non-existence (IMO it should just not be tested, considering it's wine's fault and eventually fixed).
Basically, they're different types of todos. Also unrelated but, idk why the test fails on specific test machine with that `deviceSessionId` prop. I suppose I should mark that prop as flaky or something? Considering it's not available on other test machines. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6850#note_88073
On Tue Nov 19 20:06:13 2024 +0000, Gabriel Ivăncescu wrote:
Well I can't see how it can work without two mechanisms (meaning, separated) because one tests lack of prop on wine (but found on Windows), and one tests extra prop on wine (but not found on Windows). Just think from Windows' perspective: the former must be tested (because it exists on Windows), but the latter either must not be tested, or tested for non-existence (IMO it should just not be tested, considering it's wine's fault and eventually fixed). Basically, they're different types of todos. Also unrelated but, idk why the test fails on specific test machine with that `deviceSessionId` prop. I suppose I should mark that prop as flaky or something? Considering it's not available on other test machines. If it's both in `props` and `todos`, then it should be tested for existence. If it's only in `todos`, then it should not be present on Windows.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6850#note_88077
participants (2)
-
Gabriel Ivăncescu -
Jacek Caban (＠jacek)