Re: [PATCH v3 3/3] mshtml: Add console object stub implementation.

[Jacek Caban]

Hi Paul,

On 7/7/21 2:03 PM, Paul Gofman wrote:
> Signed-off-by: Paul Gofman
>
> v3:
>      - add console getter interface to HTMLWindow.
>
> dlls/mshtml/Makefile.in              |   1 +
>   dlls/mshtml/console.c                | 294 +++++++++++++++++++++++++++

We already have more files than we should in mshtml, this could go to 
omnavigator.c instead.
> +void create_console(IWineMSHTMLConsole **ret)
> +{
> - struct console *obj;
> -
> - obj = heap_alloc_zero(sizeof(*obj));
> - if(!obj)
> - {
> -
>    ERR("No memory.\n");
> -
>    return;
> - }
> -
> - obj->IWineMSHTMLConsole_iface.lpVtbl = &WineMSHTMLConsoleVtbl;
> - obj->ref = 1;
> - init_dispatch(&obj->dispex, (IUnknown*)&obj->IWineMSHTMLConsole_iface, &console_dispex, COMPAT_MODE_NONE);

Compat mode affects some basic IDispatchEx behaviour now, so it should 
be specified properly here. For that, you will probably want to delay 
object creation.
> -
> - *ret = &obj->IWineMSHTMLConsole_iface;
>
> +}
> diff --git a/dlls/mshtml/dispex.c b/dlls/mshtml/dispex.c
> index 6d248691d6b..e0c2ea2c121 100644
> --- a/dlls/mshtml/dispex.c
> +++ b/dlls/mshtml/dispex.c
> @@ -111,7 +111,7 @@ struct dispex_dynamic_data_t {
>
> #define FDEX_VERSION_MASK 0xf0000000
>
> -static ITypeLib *typelib;
> +static ITypeLib *typelib, *typelib_private;
>   static ITypeInfo *typeinfos[LAST_tid];
>   static struct list dispex_data_list = LIST_INIT(dispex_data_list);
>
> @@ -119,6 +119,8 @@ static REFIID tid_ids[] = {
>   #define XIID(iface) &IID_ ## iface,
>   #define XDIID(iface) &DIID_ ## iface,
>   TID_LIST
> - NULL,
>
> +PRIVATE_TID_LIST
>   #undef XIID
>   #undef XDIID
>   };
> @@ -136,7 +138,17 @@ static HRESULT load_typelib(void)
>   if(InterlockedCompareExchangePointer((void**)&typelib, tl, NULL))
>       ITypeLib_Release(tl);
> - return hres;
> -
> - hres = LoadTypeLibEx(L"mshtml.dll\1", REGKIND_NONE, &tl);

It would be better to use something like GetModuleFileName() instead of 
hardcoding it.
> diff --git a/dlls/mshtml/mshtml_private_iface.idl b/dlls/mshtml/mshtml_private_iface.idl
> index 60ad70eede7..d721d09190a 100644
> --- a/dlls/mshtml/mshtml_private_iface.idl
> +++ b/dlls/mshtml/mshtml_private_iface.idl
> @@ -30,4 +30,119 @@ import "dispex.idl";
>   ]
>   library MSHTML_private
>   {
> -
>
> +importlib("stdole2.tlb");
> -
>
> +[
> - odl,
> - oleautomation,
> - dual,
> - hidden,
> - uuid(fd55b4b6-2813-4fb4-829d-380099474ab1)
>
> +]
> +interface IWineMSHTMLConsole : IDispatch
> +{
> - [id(1)]
> - HRESULT assert([in] VARIANT_BOOL *assertion,
> -
>               [in, optional] VARIANT *varargStart);
> - [id(2)]
> - HRESULT clear();
> - [id(3)]
> - HRESULT count([in, optional] VARIANT *label);
> - [id(4)]
> - HRESULT debug([in, optional] VARIANT *varargStart);
> - [id(5)]
> - HRESULT dir([in, optional] VARIANT *object);
> - [id(6)]
> - HRESULT dirxml([in, optional] VARIANT *object);
> - [id(7)]
> - HRESULT error([in, optional] VARIANT *varargStart);
> - [id(8)]
> - HRESULT group([in, optional] VARIANT *label);
> - [id(9)]
> - HRESULT groupCollapsed([in, optional] VARIANT *label);
> - [id(10)]
> - HRESULT groupEnd();
> - [id(11)]
> - HRESULT info([in, optional] VARIANT *varargStart);
> - [id(12)]
> - HRESULT log([in, optional] VARIANT *varargStart);
> - [id(13)]
> - HRESULT time([in, optional] VARIANT *label);
> - [id(14)]
> - HRESULT timeEnd([in, optional] VARIANT *label);
> - [id(15)]
> - HRESULT trace([in, optional] VARIANT *varargStart);
> - [id(16)]
> - HRESULT warn([in, optional] VARIANT *varargStart);
>
> +}
> -
>
> +[
> - hidden,
> - uuid(b61c3206-7c3d-4b56-bf16-a9f264747f58)
>
> +]
> +dispinterface DispWineMSHTMLConsole
> +{
> +properties:
> +methods:
> - [id(1)]
> - void assert([in] VARIANT_BOOL *assertion,
> -
>            [in, optional] VARIANT *varargStart);
> - [id(2)]
> - void clear();
> - [id(3)]
> - void count([in, optional] VARIANT *label);
> - [id(4)]
> - void debug([in, optional] VARIANT *varargStart);
> - [id(5)]
> - void dir([in, optional] VARIANT *object);
> - [id(6)]
> - void dirxml([in, optional] VARIANT *object);
> - [id(7)]
> - void error([in, optional] VARIANT *varargStart);
> - [id(8)]
> - void group([in, optional] VARIANT *label);
> - [id(9)]
> - void groupCollapsed([in, optional] VARIANT *label);
> - [id(10)]
> - void groupEnd();
> - [id(11)]
> - void info([in, optional] VARIANT *varargStart);
> - [id(12)]
> - void log([in, optional] VARIANT *varargStart);
> - [id(13)]
> - void time([in, optional] VARIANT *label);
> - [id(14)]
> - void timeEnd([in, optional] VARIANT *label);
> - [id(15)]
> - void trace([in, optional] VARIANT *varargStart);
> - [id(16)]
> - void warn([in, optional] VARIANT *varargStart);
>
> +}

You shouldn't need dispinterface at all.
> -
>
> +[
> - odl,
> - oleautomation,
> - dual,
> - hidden,
> - uuid(1b5939fc-8f84-43f3-8d89-f9a92069fad7)
>
> +]
> +interface IWineConsoleGetter : IDispatch
> +{
> - [propget, id(1)]
> - HRESULT console([retval, out] IDispatch **console);
>
> +}

It would be have a more generic name for this. I was hoping that we 
could use it for requestAnimationFrame as well (which would be a nice 
excuse to introduce it in a separated patch).
> -
>
> +[
> - hidden,
> - uuid(b84d405a-8057-4cc7-86c6-e872bb6205ad)
>
> +]
> +dispinterface DispWineConsoleGetter
> +{
> +properties:
> +methods:
> - [propget, id(1)]
> - IDispatch *console();
>
> +}

Same here, dispinterface is not needed.
> } /* library MSHTML_private */
> diff --git a/dlls/mshtml/tests/es5.js b/dlls/mshtml/tests/es5.js
> index a520253569c..e57aed5ca35 100644
> --- a/dlls/mshtml/tests/es5.js
> +++ b/dlls/mshtml/tests/es5.js
> @@ -1448,3 +1448,67 @@ sync_test("functions scope", function() {
>       ok(val == 8, "val != 8");
>       ok(w == 9, "w != 9");
>   });
> -
>
> +sync_test("console", function() {
> - var except
> -
> - window.console.log('1', '2');
> - console.info('1', '2', '3');
> - console.info();
> - console.log();
> - console.trace();
> - console.warn();
> - console.debug();
> - console.error();
> -
> - console.assert(false, '1');
> - console.assert(true, '1');
> - console.assert('1');
> -
> - console.clear();
> - console.count('1');
> - console.count(1);
> -
> -
> - except = false;
> - try
> - {
> -
>    console.countReset('1');
> - }
> - catch(e)
> - {
> -
>    except = true;
> - }
> - ok(except, "console.countReset: expected exception");
> - console.dir(document);
> - console.dir();
> - console.dirxml(document);
> - console.group('1');
> - console.groupCollapsed('1');
> - console.groupEnd();
> -
> - except = false;
> - try
> - {
> -
>    console.table(['1', '2']);
> - }
> - catch(e)
> - {
> -
>    except = true;
> - }
> - ok(except, "console.table: expected exception");
> -
> - console.time('1');
> - console.timeEnd('1');
> -
> - except = false;
> - try
> - {
> -
>    console.timeLog('1');
> - }
> - catch(e)
> - {
> -
>    except = true;
> - }
> - ok(except, "console.timeLog: expected exception");
>
> +});

A test in documentmode.js "window_props" would be nice as well.

Thanks,

Jacek