Re: [PATCH v2 1/8] mshtml: Implement IHTMLElement::toString.

[Jacek Caban]

Hi Gabriel,

On 9/24/21 3:45 PM, Gabriel Ivăncescu wrote:
> Signed-off-by: Gabriel Ivăncescu
>
> dlls/mshtml/dispex.c              |  10 ++
>   dlls/mshtml/htmlanchor.c          |   3 +-
>   dlls/mshtml/htmlarea.c            |   3 +-
>   dlls/mshtml/htmlbody.c            |   3 +-
>   dlls/mshtml/htmlcomment.c         |   3 +-
>   dlls/mshtml/htmlelem.c            | 163 +++++++++++++++++++++++++-----
>   dlls/mshtml/htmlform.c            |   3 +-
>   dlls/mshtml/htmlframe.c           |   6 +-
>   dlls/mshtml/htmlhead.c            |  12 ++-
>   dlls/mshtml/htmlimg.c             |   3 +-
>   dlls/mshtml/htmlinput.c           |   9 +-
>   dlls/mshtml/htmllink.c            |   3 +-
>   dlls/mshtml/htmlobject.c          |   6 +-
>   dlls/mshtml/htmlscript.c          |   3 +-
>   dlls/mshtml/htmlselect.c          |   6 +-
>   dlls/mshtml/htmlstyleelem.c       |   3 +-
>   dlls/mshtml/htmltable.c           |   9 +-
>   dlls/mshtml/htmltextarea.c        |   3 +-
>   dlls/mshtml/mshtml_private.h      |   2 +
>   dlls/mshtml/tests/documentmode.js | 142 ++++++++++++++++++++++++++
>   20 files changed, 341 insertions(+), 54 deletions(-)
>
> diff --git a/dlls/mshtml/dispex.c b/dlls/mshtml/dispex.c
> index 64ead8f..797f024 100644
> --- a/dlls/mshtml/dispex.c
> +++ b/dlls/mshtml/dispex.c
> @@ -1482,6 +1482,16 @@ compat_mode_t dispex_compat_mode(DispatchEx *dispex)
>           : dispex->info->desc->vtbl->get_compat_mode(dispex);
>   }
>
> +const WCHAR *dispex_tostring(DispatchEx *dispex)
> +{
> - const WCHAR *str = dispex->info->desc->tostring_name;
> -
> - /* Empty string is special for all modes */
> - if((!str || str[0]) && dispex_compat_mode(dispex) < COMPAT_MODE_IE9)

The special case for an empty string should not be needed.
> -
>    str = L"[object]";
> - return str;
>
> +}Gabriel

I was hoping that it could take care of entire implementation. Caller 
could be about to do just:

return dispex_to_string(&This->dispex, p);
> HRESULT HTMLCommentElement_Create(HTMLDocumentNode *doc, nsIDOMNode *nsnode, HTMLElement **elem)
> diff --git a/dlls/mshtml/htmlelem.c b/dlls/mshtml/htmlelem.c
> index d9554f4..8546c19 100644
> --- a/dlls/mshtml/htmlelem.c
> +++ b/dlls/mshtml/htmlelem.c
> @@ -47,31 +47,116 @@ typedef struct {
>   } tag_desc_t;
>
> static const tag_desc_t tag_descs[] = {
> - {L"A",         HTMLAnchorElement_Create},
> - {L"AREA",      HTMLAreaElement_Create},
> - {L"BODY",      HTMLBodyElement_Create},
> - {L"BUTTON",    HTMLButtonElement_Create},
> - {L"EMBED",     HTMLEmbedElement_Create},
> - {L"FORM",      HTMLFormElement_Create},
> - {L"FRAME",     HTMLFrameElement_Create},
> - {L"HEAD",      HTMLHeadElement_Create},
> - {L"HTML",      HTMLHtmlElement_Create},
> - {L"IFRAME",    HTMLIFrame_Create},
> - {L"IMG",       HTMLImgElement_Create},
> - {L"INPUT",     HTMLInputElement_Create},
> - {L"LABEL",     HTMLLabelElement_Create},
> - {L"LINK",      HTMLLinkElement_Create},
> - {L"META",      HTMLMetaElement_Create},
> - {L"OBJECT",    HTMLObjectElement_Create},
> - {L"OPTION",    HTMLOptionElement_Create},
> - {L"SCRIPT",    HTMLScriptElement_Create},
> - {L"SELECT",    HTMLSelectElement_Create},
> - {L"STYLE",     HTMLStyleElement_Create},
> - {L"TABLE",     HTMLTable_Create},
> - {L"TD",        HTMLTableCell_Create},
> - {L"TEXTAREA",  HTMLTextAreaElement_Create},
> - {L"TITLE",     HTMLTitleElement_Create},
> - {L"TR",        HTMLTableRow_Create}
> - {L"A",              HTMLAnchorElement_Create},
> - {L"ABBR",           NULL},
> - {L"ACRONYM",        NULL},
> - {L"ADDRESS",        NULL},
> - {L"APPLET",         NULL},
> - {L"AREA",           HTMLAreaElement_Create},
> - {L"ARTICLE",        NULL},
> - {L"ASIDE",          NULL},
> - {L"AUDIO",          NULL},
> - {L"B",              NULL},
> - {L"BASE",           NULL},
> - {L"BASEFONT",       NULL},
> - {L"BDO",            NULL},
> - {L"BIG",            NULL},
> - {L"BLOCKQUOTE",     NULL},
> - {L"BODY",           HTMLBodyElement_Create},
> - {L"BR",             NULL},
> - {L"BUTTON",         HTMLButtonElement_Create},
> - {L"CANVAS",         NULL},
> - {L"CAPTION",        NULL},
> - {L"CENTER",         NULL},
> - {L"CITE",           NULL},
> - {L"CODE",           NULL},
> - {L"COL",            NULL},
> - {L"COLGROUP",       NULL},
> - {L"DATALIST",       NULL},
> - {L"DD",             NULL},
> - {L"DEL",            NULL},
> - {L"DFN",            NULL},
> - {L"DIR",            NULL},
> - {L"DIV",            NULL},
> - {L"DL",             NULL},
> - {L"DT",             NULL},
> - {L"EM",             NULL},
> - {L"EMBED",          HTMLEmbedElement_Create},
> - {L"FIELDSET",       NULL},
> - {L"FIGCAPTION",     NULL},
> - {L"FIGURE",         NULL},
> - {L"FONT",           NULL},
> - {L"FOOTER",         NULL},
> - {L"FORM",           HTMLFormElement_Create},
> - {L"FRAME",          HTMLFrameElement_Create},
> - {L"FRAMESET",       NULL},
> - {L"H1",             NULL},
> - {L"H2",             NULL},
> - {L"H3",             NULL},
> - {L"H4",             NULL},
> - {L"H5",             NULL},
> - {L"H6",             NULL},
> - {L"HEAD",           HTMLHeadElement_Create},
> - {L"HEADER",         NULL},
> - {L"HR",             NULL},
> - {L"HTML",           HTMLHtmlElement_Create},
> - {L"I",              NULL},
> - {L"IFRAME",         HTMLIFrame_Create},
> - {L"IMG",            HTMLImgElement_Create},
> - {L"INPUT",          HTMLInputElement_Create},
> - {L"INS",            NULL},
> - {L"KBD",            NULL},
> - {L"LABEL",          HTMLLabelElement_Create},
> - {L"LEGEND",         NULL},
> - {L"LI",             NULL},
> - {L"LINK",           HTMLLinkElement_Create},
> - {L"MAP",            NULL},
> - {L"MARK",           NULL},
> - {L"META",           HTMLMetaElement_Create},
> - {L"NAV",            NULL},
> - {L"NOFRAMES",       NULL},
> - {L"NOSCRIPT",       NULL},
> - {L"OBJECT",         HTMLObjectElement_Create},
> - {L"OL",             NULL},
> - {L"OPTGROUP",       NULL},
> - {L"OPTION",         HTMLOptionElement_Create},
> - {L"P",              NULL},
> - {L"PARAM",          NULL},
> - {L"PRE",            NULL},
> - {L"PROGRESS",       NULL},
> - {L"Q",              NULL},
> - {L"RP",             NULL},
> - {L"RT",             NULL},
> - {L"RUBY",           NULL},
> - {L"S",              NULL},
> - {L"SAMP",           NULL},
> - {L"SCRIPT",         HTMLScriptElement_Create},
> - {L"SECTION",        NULL},
> - {L"SELECT",         HTMLSelectElement_Create},
> - {L"SMALL",          NULL},
> - {L"SOURCE",         NULL},
> - {L"SPAN",           NULL},
> - {L"STRIKE",         NULL},
> - {L"STRONG",         NULL},
> - {L"STYLE",          HTMLStyleElement_Create},
> - {L"SUB",            NULL},
> - {L"SUP",            NULL},
> - {L"TABLE",          HTMLTable_Create},
> - {L"TBODY",          NULL},
> - {L"TD",             HTMLTableCell_Create},
> - {L"TEXTAREA",       HTMLTextAreaElement_Create},
> - {L"TFOOT",          NULL},
> - {L"TH",             NULL},
> - {L"THEAD",          NULL},
> - {L"TITLE",          HTMLTitleElement_Create},
> - {L"TR",             HTMLTableRow_Create},
> - {L"TRACK",          NULL},
> - {L"TT",             NULL},
> - {L"U",              NULL},
> - {L"UL",             NULL},
> - {L"VAR",            NULL},
> - {L"VIDEO",          NULL},
> - {L"WBR",            NULL}
> };

I think that having a separated constructor instead of all those NULLs 
would avoid the mess that you need in HTMLElement_toString:
> static const tag_desc_t *get_tag_desc(const WCHAR *tag_name)
> @@ -2112,8 +2197,30 @@ static HRESULT WINAPI HTMLElement_get_ondragstart(IHTMLElement *iface, VARIANT *
>   static HRESULT WINAPI HTMLElement_toString(IHTMLElement *iface, BSTR *String)
>   {
>       HTMLElement *This = impl_from_IHTMLElement(iface);
> - FIXME("(%p)->(%p)\n", This, String);
> - return E_NOTIMPL;
> - const PRUnichar *str;
> - nsAString tag_str;
> - nsresult nsres;
> -
> - TRACE("(%p)->(%p)\n", This, String);
> -
> - if(!String)
> -
>    return E_INVALIDARG;
> -
> - str = dispex_tostring(&This->node.event_target.dispex);
> - if(!str) {
> -
>    nsAString_Init(&tag_str, NULL);
> -
>    nsres = nsIDOMElement_GetTagName(This->dom_element, &tag_str);
> -
>    if(NS_FAILED(nsres)) {
> -
>        nsAString_Finish(&tag_str);
> -
>        return map_nsresult(nsres);
> -
>    }
> -
>    nsAString_GetData(&tag_str, &str);
> -
>    str = get_tag_desc(str) ? L"[object HTMLElement]" : L"[object HTMLUnknownElement]";
> -
>    nsAString_Finish(&tag_str);
> - }
> -
> - *String = SysAllocString(str);
> - return *String ? S_OK : E_OUTOFMEMORY;
> }

(...)
> diff --git a/dlls/mshtml/mshtml_private.h b/dlls/mshtml/mshtml_private.h
> index c1e7e78..67ea0ad 100644
> --- a/dlls/mshtml/mshtml_private.h
> +++ b/dlls/mshtml/mshtml_private.h
> @@ -331,6 +331,7 @@ typedef struct {
>       const tid_t disp_tid;
>       const tid_t* const iface_tids;
>       void (*init_info)(dispex_data_t*,compat_mode_t);
> - const WCHAR *tostring_name;
>  dispex_data_t *info_cache[COMPAT_MODE_CNT];
>  dispex_data_t *delayed_init_info;
> } dispex_static_data_t;

It would be cleaner to no tie it so tightly with toString and store just 
the type name. "[object ...]" decorations can be a part of toString 
implementation.

I also think it would be nicer to have it as the first field of 
dispex_static_data_t.

Thanks,

Jacek