Re: [PATCH 2/3] msscript: Added basic script hosting stub support

[Jacek Caban]

Hi Nikolay,

On 06/11/2016 12:02 PM, Nikolay Sivov wrote:
> Signed-off-by: Nikolay Sivov
>
> This deprecates patch 123111.
>
> dlls/msscript.ocx/msscript.c        | 237 ++++++++++++++++-
>  dlls/msscript.ocx/tests/Makefile.in |   2 +-
>  dlls/msscript.ocx/tests/msscript.c  | 488 ++++++++++++++++++++++++++++++++++++
>  include/activscp.idl                |  16 ++
>  4 files changed, 737 insertions(+), 6 deletions(-)
>
> diff --git a/dlls/msscript.ocx/msscript.c b/dlls/msscript.ocx/msscript.c
> index 5a03f0c..b6764f4 100644
> --- a/dlls/msscript.ocx/msscript.c
> +++ b/dlls/msscript.ocx/msscript.c
> @@ -22,6 +22,7 @@
>  #include "initguid.h"
>  #include "ole2.h"
>  #include "olectl.h"
> +#include "activscp.h"
>  #include "rpcproxy.h"
>  #include "msscript.h"
>
> @@ -29,14 +30,21 @@
>
> WINE_DEFAULT_DEBUG_CHANNEL(msscript);
>
> +static const WCHAR vbscriptW[] = {'V','B','S','c','r','i','p','t',0};
> +static const WCHAR jscriptW[] = {'J','S','c','r','i','p','t',0};
> -
>
> struct ScriptControl {
>      IScriptControl IScriptControl_iface;
>      IPersistStreamInit IPersistStreamInit_iface;
>      IOleObject IOleObject_iface;
>      IOleControl IOleControl_iface;
> - IActiveScriptSite IActiveScriptSite_iface;
> LONG ref;
> IOleClientSite *site;
> SIZEL extent;
> - BSTR lang;
> - IActiveScript *script;
> - SCRIPTSTATE script_state;
>
> };

It's a good moment to think a bit about the design here. I don't think
you want to reuse IActiveScriptSite for all script engines you load. A
separated object would allow better ref counting, would be cleaner and
safer in my opinion. A quick test on Windows suggests that native does that.
> static HINSTANCE msscript_instance;
> @@ -120,6 +128,29 @@ static void release_typelib(void)
>      ITypeLib_Release(typelib);
>  }
>
> +static void release_script_engine(ScriptControl *sc)
> +{
> - if (!sc->script)
> -
>    return;
> -
> - switch (sc->script_state) {
> - case SCRIPTSTATE_CONNECTED:
> -
>    IActiveScript_SetScriptState(sc->script, SCRIPTSTATE_DISCONNECTED);
> -
> - case SCRIPTSTATE_STARTED:
> - case SCRIPTSTATE_DISCONNECTED:
> - case SCRIPTSTATE_INITIALIZED:
> -
>    IActiveScript_Close(sc->script);
> -
> - default:
> -
>    ;
> - }

Some "fall through" comments to avoid static analyser fixes follow ups
would be nice.
> -static HRESULT WINAPI ScriptControl_put_Language(IScriptControl *iface, BSTR language)
> +static HRESULT WINAPI ScriptControl_put_Language(IScriptControl *iface, BSTR lang)
>  {
>      ScriptControl *This = impl_from_IScriptControl(iface);
> - FIXME("(%p)->(%s)\n", This, debugstr_w(language));
> - return E_NOTIMPL;
> - HRESULT hr;
> - GUID guid;
> -
> - TRACE("(%p)->(%s)\n", This, debugstr_w(lang));
> -
> - if (!lang) {
> -
>    release_script_engine(This);
> -
>    set_script_name(This, NULL);
> -
>    return S_OK;
> - }
> -
> - hr = CLSIDFromProgID(lang, &guid);
> - if (FAILED(hr))
> -
>    return CTL_E_INVALIDPROPERTYVALUE;
> -
> - hr = CoCreateInstance(&guid, NULL, CLSCTX_INPROC_SERVER|CLSCTX_INPROC_HANDLER,
> -
>        &IID_IActiveScript, (void**)&This->script);
> - if (FAILED(hr))
> -
>    return CTL_E_INVALIDPROPERTYVALUE;
> - else if (!init_script_engine(This)) {
> -
>    release_script_engine(This);
> -
>    return CTL_E_INVALIDPROPERTYVALUE;
> - }
> -
> - /* store engine name */
> - if (!lstrcmpiW(lang, vbscriptW))
> -
>    This->lang = SysAllocString(vbscriptW);
> - else if (!lstrcmpiW(lang, jscriptW))
> -
>    This->lang = SysAllocString(jscriptW);
> - else

Why do you need those? My guess is that returned script may have
different upper/lower cases, but if that the case, then we should
probably use ProgIDFromCLSID in getter and just store CLSID.
> -
>    This->lang = SysAllocString(lang);
> -
> - return S_OK;
>
> }
>
> static HRESULT WINAPI ScriptControl_get_State(IScriptControl *iface, ScriptControlStates *p)
> @@ -873,8 +1096,12 @@ static HRESULT WINAPI ScriptControl_CreateInstance(IClassFactory *iface, IUnknow
>      script_control->IPersistStreamInit_iface.lpVtbl = &PersistStreamInitVtbl;
>      script_control->IOleObject_iface.lpVtbl = &OleObjectVtbl;
>      script_control->IOleControl_iface.lpVtbl = &OleControlVtbl;
> -
>
> script_control->IActiveScriptSite_iface.lpVtbl = &ActiveScriptSiteVtbl;
> script_control->ref = 1;
> script_control->site = NULL;
> -
>
> script_control->lang = NULL;
> -
>
> script_control->script_state = SCRIPTSTATE_UNINITIALIZED;
> -
>
> script_control->script = NULL;
>
> hdc = GetDC(0);
> dpi_x = GetDeviceCaps(hdc, LOGPIXELSX);
>
> diff --git a/dlls/msscript.ocx/tests/Makefile.in b/dlls/msscript.ocx/tests/Makefile.in
> index 8d769d3..cc62590 100644
> --- a/dlls/msscript.ocx/tests/Makefile.in
> +++ b/dlls/msscript.ocx/tests/Makefile.in
> @@ -1,5 +1,5 @@
>  TESTDLL   = msscript.ocx
> -IMPORTS   = user32 gdi32 ole32
> +IMPORTS   = user32 gdi32 ole32 oleaut32 advapi32
>
> C_SRCS = \
>    msscript.c
> diff --git a/dlls/msscript.ocx/tests/msscript.c b/dlls/msscript.ocx/tests/msscript.c
> index 12f7003..d1d553b 100644
> --- a/dlls/msscript.ocx/tests/msscript.c
> +++ b/dlls/msscript.ocx/tests/msscript.c
> @@ -22,10 +22,411 @@
>  #include <initguid.h>
>  #include <ole2.h>
>  #include <olectl.h>
> +#include "dispex.h"
> +#include "activscp.h"
> +#include "activdbg.h"
> +#include "objsafe.h"
>
> #include "msscript.h"
>  #include "wine/test.h"
>
> +#define TESTSCRIPT_CLSID "{178fc164-f585-4e24-9c13-4bb7faf80746}"
> +static const GUID CLSID_TestScript =
> - {0x178fc164,0xf585,0x4e24,{0x9c,0x13,0x4b,0xb7,0xfa,0xf8,0x07,0x46}};
> -
>
> +#ifdef _WIN64
> -
>
> +#define CTXARG_T DWORDLONG
> +#define IActiveScriptParseVtbl IActiveScriptParse64Vtbl
> +#define IActiveScriptSiteDebug_Release IActiveScriptSiteDebug64_Release
> -
>
> +#else
> -
>
> +#define CTXARG_T DWORD
> +#define IActiveScriptParseVtbl IActiveScriptParse32Vtbl
> +#define IActiveScriptSiteDebug_Release IActiveScriptSiteDebug32_Release
> -
>
> +#endif
> -
>
> +static IActiveScriptSite *site;
> +static SCRIPTSTATE state;
> -
>
> +static HRESULT WINAPI ActiveScriptParse_QueryInterface(IActiveScriptParse *iface, REFIID riid, void **ppv)
> +{
> - *ppv = NULL;
> - ok(0, "unexpected call\n");
> - return E_NOINTERFACE;
>
> +}
> -
>
> +static ULONG WINAPI ActiveScriptParse_AddRef(IActiveScriptParse *iface)
> +{
> - return 2;
>
> +}
> -
>
> +static ULONG WINAPI ActiveScriptParse_Release(IActiveScriptParse *iface)
> +{
> - return 1;
>
> +}
> -
>
> +static HRESULT WINAPI ActiveScriptParse_InitNew(IActiveScriptParse *iface)
> +{
> - return S_OK;
>
> +}
> -
>
> +static HRESULT WINAPI ActiveScriptParse_AddScriptlet(IActiveScriptParse *iface,
> -
>    LPCOLESTR pstrDefaultName, LPCOLESTR pstrCode, LPCOLESTR pstrItemName,
> -
>    LPCOLESTR pstrSubItemName, LPCOLESTR pstrEventName, LPCOLESTR pstrDelimiter,
> -
>    CTXARG_T dwSourceContextCookie, ULONG ulStartingLineNumber, DWORD dwFlags,
> -
>    BSTR *pbstrName, EXCEPINFO *pexcepinfo)
>
> +{
> - ok(0, "unexpected call\n");
> - return E_NOTIMPL;
>
> +}
> -
>
> +static HRESULT WINAPI ActiveScriptParse_ParseScriptText(IActiveScriptParse *iface,
> -
>    LPCOLESTR pstrCode, LPCOLESTR pstrItemName, IUnknown *punkContext,
> -
>    LPCOLESTR pstrDelimiter, CTXARG_T dwSourceContextCookie, ULONG ulStartingLine,
> -
>    DWORD dwFlags, VARIANT *pvarResult, EXCEPINFO *pexcepinfo)
>
> +{
> - ok(0, "unexpected call\n");
> - return E_NOTIMPL;
>
> +}
> -
>
> +static const IActiveScriptParseVtbl ActiveScriptParseVtbl = {
> - ActiveScriptParse_QueryInterface,
> - ActiveScriptParse_AddRef,
> - ActiveScriptParse_Release,
> - ActiveScriptParse_InitNew,
> - ActiveScriptParse_AddScriptlet,
> - ActiveScriptParse_ParseScriptText
>
> +};
> -
>
> +static IActiveScriptParse ActiveScriptParse = { &ActiveScriptParseVtbl };
> -
>
> +static HRESULT WINAPI ObjectSafety_QueryInterface(IObjectSafety *iface, REFIID riid, void **ppv)
> +{
> - *ppv = NULL;
> - ok(0, "unexpected call %s\n", wine_dbgstr_guid(riid));
> - return E_NOINTERFACE;
>
> +}
> -
>
> +static ULONG WINAPI ObjectSafety_AddRef(IObjectSafety *iface)
> +{
> - return 2;
>
> +}
> -
>
> +static ULONG WINAPI ObjectSafety_Release(IObjectSafety *iface)
> +{
> - return 1;
>
> +}
> -
>
> +static HRESULT WINAPI ObjectSafety_GetInterfaceSafetyOptions(IObjectSafety *iface, REFIID riid,
> -
>    DWORD *pdwSupportedOptions, DWORD *pdwEnabledOptions)
>
> +{
> - ok(IsEqualGUID(&IID_IActiveScriptParse, riid), "unexpected riid %s\n", wine_dbgstr_guid(riid));
> - ok(pdwSupportedOptions != NULL, "pdwSupportedOptions == NULL\n");
> - ok(pdwEnabledOptions != NULL, "pdwEnabledOptions == NULL\n");
> -
> - *pdwSupportedOptions = INTERFACESAFE_FOR_UNTRUSTED_DATA|INTERFACE_USES_DISPEX|INTERFACE_USES_SECURITY_MANAGER;
> - *pdwEnabledOptions = INTERFACE_USES_DISPEX;
> -
> - return S_OK;
>
> +}
> -
>
> +static HRESULT WINAPI ObjectSafety_SetInterfaceSafetyOptions(IObjectSafety *iface, REFIID riid,
> -
>    DWORD mask, DWORD options)
>
> +{
> - ok(IsEqualGUID(&IID_IActiveScriptParse, riid), "unexpected riid %s\n", wine_dbgstr_guid(riid));
> -
> - ok(mask == INTERFACESAFE_FOR_UNTRUSTED_DATA, "option mask = %x\n", mask);
> - ok(options == 0, "options = %x\n", options);
> -
> - return S_OK;
>
> +}
> -
>
> +static const IObjectSafetyVtbl ObjectSafetyVtbl = {
> - ObjectSafety_QueryInterface,
> - ObjectSafety_AddRef,
> - ObjectSafety_Release,
> - ObjectSafety_GetInterfaceSafetyOptions,
> - ObjectSafety_SetInterfaceSafetyOptions
>
> +};
> -
>
> +static IObjectSafety ObjectSafety = { &ObjectSafetyVtbl };
> -
>
> +static HRESULT WINAPI ActiveScript_QueryInterface(IActiveScript *iface, REFIID riid, void **ppv)
> +{
> - *ppv = NULL;
> -
> - if(IsEqualGUID(&IID_IUnknown, riid) || IsEqualGUID(&IID_IActiveScript, riid)) {
> -
>    *ppv = iface;
> -
>    return S_OK;
> - }
> -
> - if(IsEqualGUID(&IID_IObjectSafety, riid)) {
> -
>    *ppv = &ObjectSafety;
> -
>    return S_OK;
> - }
> -
> - if(IsEqualGUID(&IID_IActiveScriptParse, riid)) {
> -
>    *ppv = &ActiveScriptParse;
> -
>    return S_OK;
> - }
> -
> - if(IsEqualGUID(&IID_IActiveScriptGarbageCollector, riid)) {
> -
>    return E_NOINTERFACE;
> - }
> -
> - ok(0, "unexpected riid %s\n", wine_dbgstr_guid(riid));
> - return E_NOINTERFACE;
>
> +}
> -
>
> +static ULONG WINAPI ActiveScript_AddRef(IActiveScript *iface)
> +{
> - return 2;
>
> +}
> -
>
> +static ULONG WINAPI ActiveScript_Release(IActiveScript *iface)
> +{
> - return 1;
>
> +}
> -
>
> +static HRESULT WINAPI ActiveScript_SetScriptSite(IActiveScript *iface, IActiveScriptSite *pass)
> +{
> - IActiveScriptSiteInterruptPoll *poll;
> - IActiveScriptSiteDebug *debug;
> - IServiceProvider *service;
> - ICanHandleException *canexpection;
> - LCID lcid;
> - HRESULT hres;
> -
> - ok(pass != NULL, "pass == NULL\n");
> -
> - hres = IActiveScriptSite_QueryInterface(pass, &IID_IActiveScriptSiteInterruptPoll, (void**)&poll);
> - ok(hres == E_NOINTERFACE, "Could not get IActiveScriptSiteInterruptPoll interface: %08x\n", hres);
> -
> - hres = IActiveScriptSite_GetLCID(pass, &lcid);
> - ok(hres == S_OK, "GetLCID failed: %08x\n", hres);
> -
> - hres = IActiveScriptSite_OnStateChange(pass, (state = SCRIPTSTATE_INITIALIZED));
>
> +todo_wine
> - ok(hres == E_NOTIMPL, "OnStateChange failed: %08x\n", hres);
> -
> - hres = IActiveScriptSite_QueryInterface(pass, &IID_IActiveScriptSiteDebug, (void**)&debug);
> - ok(hres == E_NOINTERFACE, "Could not get IActiveScriptSiteDebug interface: %08x\n", hres);
> -
> - hres = IActiveScriptSite_QueryInterface(pass, &IID_ICanHandleException, (void**)&canexpection);
> - ok(hres == E_NOINTERFACE, "Could not get IID_ICanHandleException interface: %08x\n", hres);
> -
> - hres = IActiveScriptSite_QueryInterface(pass, &IID_IServiceProvider, (void**)&service);
>
> +todo_wine
> - ok(hres == S_OK, "Could not get IServiceProvider interface: %08x\n", hres);
> - if(SUCCEEDED(hres))
> -
>    IServiceProvider_Release(service);
> -
> - site = pass;
> - IActiveScriptSite_AddRef(site);
> - return S_OK;
>
> +}
> -
>
> +static HRESULT WINAPI ActiveScript_GetScriptSite(IActiveScript *iface, REFIID riid,
> -
>                                        void **ppvObject)
>
> +{
> - ok(0, "unexpected call\n");
> - return E_NOTIMPL;
>
> +}
> -
>
> +static HRESULT WINAPI ActiveScript_SetScriptState(IActiveScript *iface, SCRIPTSTATE ss)
> +{
> - ok(0, "unexpected call\n");
> - return E_NOTIMPL;
>
> +}
> -
>
> +static HRESULT WINAPI ActiveScript_GetScriptState(IActiveScript *iface, SCRIPTSTATE *pssState)
> +{
> - ok(0, "unexpected call\n");
> - return E_NOTIMPL;
>
> +}
> -
>
> +static HRESULT WINAPI ActiveScript_Close(IActiveScript *iface)
> +{
> - return S_OK;
>
> +}
> -
>
> +static HRESULT WINAPI ActiveScript_AddNamedItem(IActiveScript *iface,
> -
>    LPCOLESTR pstrName, DWORD dwFlags)
>
> +{
> - ok(0, "unexpected call\n");
> - return E_NOTIMPL;
>
> +}
> -
>
> +static HRESULT WINAPI ActiveScript_AddTypeLib(IActiveScript *iface, REFGUID rguidTypeLib,
> -
>                                     DWORD dwMajor, DWORD dwMinor, DWORD dwFlags)
>
> +{
> - ok(0, "unexpected call\n");
> - return E_NOTIMPL;
>
> +}
> -
>
> +static HRESULT WINAPI ActiveScript_GetScriptDispatch(IActiveScript *iface, LPCOLESTR pstrItemName,
> -
>                                            IDispatch **ppdisp)
>
> +{
> - ok(0, "unexpected call\n");
> - return E_NOTIMPL;
>
> +}
> -
>
> +static HRESULT WINAPI ActiveScript_GetCurrentScriptThreadID(IActiveScript *iface,
> -
>                                                   SCRIPTTHREADID *pstridThread)
>
> +{
> - ok(0, "unexpected call\n");
> - return E_NOTIMPL;
>
> +}
> -
>
> +static HRESULT WINAPI ActiveScript_GetScriptThreadID(IActiveScript *iface,
> -
>                                            DWORD dwWin32ThreadId, SCRIPTTHREADID *pstidThread)
>
> +{
> - ok(0, "unexpected call\n");
> - return E_NOTIMPL;
>
> +}
> -
>
> +static HRESULT WINAPI ActiveScript_GetScriptThreadState(IActiveScript *iface,
> -
>    SCRIPTTHREADID stidThread, SCRIPTTHREADSTATE *pstsState)
>
> +{
> - ok(0, "unexpected call\n");
> - return E_NOTIMPL;
>
> +}
> -
>
> +static HRESULT WINAPI ActiveScript_InterruptScriptThread(IActiveScript *iface,
> -
>    SCRIPTTHREADID stidThread, const EXCEPINFO *pexcepinfo, DWORD dwFlags)
>
> +{
> - ok(0, "unexpected call\n");
> - return E_NOTIMPL;
>
> +}
> -
>
> +static HRESULT WINAPI ActiveScript_Clone(IActiveScript *iface, IActiveScript **ppscript)
> +{
> - ok(0, "unexpected call\n");
> - return E_NOTIMPL;
>
> +}
> -
>
> +static const IActiveScriptVtbl ActiveScriptVtbl = {
> - ActiveScript_QueryInterface,
> - ActiveScript_AddRef,
> - ActiveScript_Release,
> - ActiveScript_SetScriptSite,
> - ActiveScript_GetScriptSite,
> - ActiveScript_SetScriptState,
> - ActiveScript_GetScriptState,
> - ActiveScript_Close,
> - ActiveScript_AddNamedItem,
> - ActiveScript_AddTypeLib,
> - ActiveScript_GetScriptDispatch,
> - ActiveScript_GetCurrentScriptThreadID,
> - ActiveScript_GetScriptThreadID,
> - ActiveScript_GetScriptThreadState,
> - ActiveScript_InterruptScriptThread,
> - ActiveScript_Clone
>
> +};
> -
>
> +static IActiveScript ActiveScript = { &ActiveScriptVtbl };
> -
>
> +static HRESULT WINAPI ClassFactory_QueryInterface(IClassFactory *iface, REFIID riid, void **ppv)
> +{
> - *ppv = NULL;
> -
> - if(IsEqualGUID(&IID_IUnknown, riid) || IsEqualGUID(&IID_IClassFactory, riid)) {
> -
>    *ppv = iface;
> -
>    return S_OK;
> - }
> -
> - if(IsEqualGUID(&IID_IMarshal, riid))
> -
>    return E_NOINTERFACE;
> -
> - ok(0, "unexpected riid %s\n", wine_dbgstr_guid(riid));
> - return E_NOINTERFACE;
>
> +}
> -
>
> +static ULONG WINAPI ClassFactory_AddRef(IClassFactory *iface)
> +{
> - return 2;
>
> +}
> -
>
> +static ULONG WINAPI ClassFactory_Release(IClassFactory *iface)
> +{
> - return 1;
>
> +}
> -
>
> +static HRESULT WINAPI ClassFactory_CreateInstance(IClassFactory *iface, IUnknown *outer, REFIID riid, void **ppv)
> +{
> - ok(!outer, "outer = %p\n", outer);
> - ok(IsEqualGUID(&IID_IActiveScript, riid), "unexpected riid %s\n", wine_dbgstr_guid(riid));
> - *ppv = &ActiveScript;
> - return S_OK;
>
> +}
> -
>
> +static HRESULT WINAPI ClassFactory_LockServer(IClassFactory *iface, BOOL dolock)
> +{
> - ok(0, "unexpected call\n");
> - return S_OK;
>
> +}
> -
>
> +static const IClassFactoryVtbl ClassFactoryVtbl = {
> - ClassFactory_QueryInterface,
> - ClassFactory_AddRef,
> - ClassFactory_Release,
> - ClassFactory_CreateInstance,
> - ClassFactory_LockServer
>
> +};
> -
>
> +static IClassFactory script_cf = { &ClassFactoryVtbl };
> -
>
> +static BOOL init_key(const char *key_name, const char *def_value, BOOL init)
> +{
> - HKEY hkey;
> - DWORD res;
> -
> - if(!init) {
> -
>    RegDeleteKeyA(HKEY_CLASSES_ROOT, key_name);
> -
>    return TRUE;
> - }
> -
> - res = RegCreateKeyA(HKEY_CLASSES_ROOT, key_name, &hkey);
> - if(res != ERROR_SUCCESS)
> -
>    return FALSE;
> -
> - if(def_value)
> -
>    res = RegSetValueA(hkey, NULL, REG_SZ, def_value, strlen(def_value));
> -
> - RegCloseKey(hkey);
> -
> - return res == ERROR_SUCCESS;
>
> +}
> -
>
> +static BOOL init_registry(BOOL init)
> +{
> - return init_key("TestScript\CLSID", TESTSCRIPT_CLSID, init)
> -
>    && init_key("CLSID\\"TESTSCRIPT_CLSID"\\Implemented Categories\\{F0B7A1A1-9847-11CF-8F20-00805F2CD064}",
> -
>                NULL, init)
> -
>    && init_key("CLSID\\"TESTSCRIPT_CLSID"\\Implemented Categories\\{F0B7A1A2-9847-11CF-8F20-00805F2CD064}",
> -
>                NULL, init);
>
> +}
> -
>
> +static BOOL register_script_engine(void)
> +{
> - DWORD regid;
> - HRESULT hres;
> -
> - if(!init_registry(TRUE)) {
> -
>    init_registry(FALSE);
> -
>    return FALSE;
> - }
> -
> - hres = CoRegisterClassObject(&CLSID_TestScript, (IUnknown *)&script_cf,
> -
>                             CLSCTX_INPROC_SERVER, REGCLS_MULTIPLEUSE, &regid);
> - ok(hres == S_OK, "Could not register script engine: %08x\n", hres);
> -
> - return TRUE;
>
> +}
> -
>
> static HRESULT WINAPI OleClientSite_QueryInterface(IOleClientSite *iface, REFIID riid, void **obj)
>  {
>      if (IsEqualIID(riid, &IID_IOleClientSite) || IsEqualIID(riid, &IID_IUnknown))
> @@ -213,6 +614,92 @@ static void test_olecontrol(void)
>      IOleControl_Release(olecontrol);
>  }
>
> +static void test_Language(void)
> +{
> - static const WCHAR vbW[] = {'V','B','S','c','r','i','p','t',0};
> - static const WCHAR jsW[] = {'J','S','c','r','i','p','t',0};
> - static const WCHAR vb2W[] = {'v','B','s','c','r','i','p','t',0};
> - static const WCHAR dummyW[] = {'d','u','m','m','y',0};
> - IScriptControl *sc;
> - HRESULT hr;
> - BSTR str;
> -
> - hr = CoCreateInstance(&CLSID_ScriptControl, NULL, CLSCTX_INPROC_SERVER|CLSCTX_INPROC_HANDLER,
> -
>        &IID_IScriptControl, (void**)&sc);
> - ok(hr == S_OK, "got 0x%08x\n", hr);
> -
> - hr = IScriptControl_get_Language(sc, NULL);
> - ok(hr == E_POINTER, "got 0x%08x\n", hr);
> -
> - str = (BSTR)0xdeadbeef;
> - hr = IScriptControl_get_Language(sc, &str);
> - ok(hr == S_OK, "got 0x%08x\n", hr);
> - ok(str == NULL, "got %s\n", wine_dbgstr_w(str));
> -
> - str = SysAllocString(vbW);
> - hr = IScriptControl_put_Language(sc, str);
> - ok(hr == S_OK, "got 0x%08x\n", hr);
> - SysFreeString(str);
> -
> - str = SysAllocString(vb2W);
> - hr = IScriptControl_put_Language(sc, str);
> - ok(hr == S_OK, "got 0x%08x\n", hr);
> - SysFreeString(str);
> -
> - hr = IScriptControl_get_Language(sc, &str);
> - ok(hr == S_OK, "got 0x%08x\n", hr);
> - ok(!lstrcmpW(str, vbW), "got %s\n", wine_dbgstr_w(str));
> - SysFreeString(str);
> -
> - str = SysAllocString(dummyW);
> - hr = IScriptControl_put_Language(sc, str);
> - ok(hr == CTL_E_INVALIDPROPERTYVALUE, "got 0x%08x\n", hr);
> - SysFreeString(str);
> -
> - hr = IScriptControl_get_Language(sc, &str);
> - ok(hr == S_OK, "got 0x%08x\n", hr);
> - ok(!lstrcmpW(str, vbW), "got %s\n", wine_dbgstr_w(str));
> - SysFreeString(str);
> -
> - str = SysAllocString(jsW);
> - hr = IScriptControl_put_Language(sc, str);
> - ok(hr == S_OK, "got 0x%08x\n", hr);
> - SysFreeString(str);
> -
> - hr = IScriptControl_get_Language(sc, &str);
> - ok(hr == S_OK, "got 0x%08x\n", hr);
> - ok(!lstrcmpW(str, jsW), "got %s\n", wine_dbgstr_w(str));
> - SysFreeString(str);
> -
> - hr = IScriptControl_put_Language(sc, NULL);
> - ok(hr == S_OK, "got 0x%08x\n", hr);
> -
> - hr = IScriptControl_get_Language(sc, &str);
> - ok(hr == S_OK, "got 0x%08x\n", hr);
> - ok(str == NULL, "got %s\n", wine_dbgstr_w(str));
> -
> - /* custom script engine */
> - if (register_script_engine()) {
> -
>    static const WCHAR testscriptW[] = {'t','e','s','t','s','c','r','i','p','t',0};
> -
> -
>    str = SysAllocString(testscriptW);
> -
>    hr = IScriptControl_put_Language(sc, str);
> -
>    ok(hr == S_OK, "got 0x%08x\n", hr);
> -
>    SysFreeString(str);

It's nice to have tests with emulated script engine, but please also add
test of called callbacks. Please add something like
CHECK_EXPECT()/CHECK_CALLED() macros or alike. Otherwise important
informations that your tests already gather are hidden and not tested.
> -
> -
>    hr = IScriptControl_get_Language(sc, &str);
> -
>    ok(hr == S_OK, "got 0x%08x\n", hr);
> -
>    ok(!lstrcmpW(testscriptW, str), "%s\n", wine_dbgstr_w(str));
> -
>    SysFreeString(str);
> -
> -
>    init_registry(FALSE);
> - }
> - else
> -
>    skip("Could not register TestScript engine\n");
> -
> - IScriptControl_Release(sc);
>
> +}
> -
>
> START_TEST(msscript)
>  {
>      IUnknown *unk;
> @@ -231,6 +718,7 @@ START_TEST(msscript)
>      test_oleobject();
>      test_persiststreaminit();
>      test_olecontrol();
> -
>
> test_Language();
>
> CoUninitialize();
>
> }
> diff --git a/include/activscp.idl b/include/activscp.idl
> index 8a3d75d..e539b04 100644
> --- a/include/activscp.idl
> +++ b/include/activscp.idl
> @@ -71,6 +71,11 @@ typedef enum tagSCRIPTUICHANDLING {
>      SCRIPTUICHANDLING_NOUIDEFAULT = 2
>  } SCRIPTUICHANDLING;
>
> +typedef enum tagSCRIPTGCTYPE {
> - SCRIPTGCTYPE_NORMAL     = 0,
> - SCRIPTGCTYPE_EXHAUSTIVE = 1
>
> +} SCRIPTGCTYPE;
> -
>
> typedef DWORD SCRIPTTHREADID;
>  cpp_quote("#define SCRIPTTHREADID_CURRENT ((SCRIPTTHREADID)-1)")
>  cpp_quote("#define SCRIPTTHREADID_BASE ((SCRIPTTHREADID)-2)")
> @@ -551,3 +556,14 @@ interface IActiveScriptProperty : IUnknown
>              [in] VARIANT *pvarIndex,
>              [in] VARIANT *pvarValue);
>  }
> -
>
> +[
> - object,
> - uuid(6aa2c4a0-2b53-11d4-a2a0-00104bd35090),
> - pointer_default(unique)
>
> +]
> +interface IActiveScriptGarbageCollector : IUnknown
> +{
> - HRESULT CollectGarbage(
> -
>        [in] SCRIPTGCTYPE gctype);
>
> +}

It's not too important, but technically it deserves a separated patch.

Thanks,
Jacek

[PATCH 2/3] msscript: Added basic script hosting stub support

[Nikolay Sivov]

On 13.06.2016 15:55, Jacek Caban wrote:
> Hi Nikolay,
>
> On 06/11/2016 12:02 PM, Nikolay Sivov wrote:
> > Signed-off-by: Nikolay Sivov
> >
> > This deprecates patch 123111.
> >
> > dlls/msscript.ocx/msscript.c        | 237 ++++++++++++++++-
> >  dlls/msscript.ocx/tests/Makefile.in |   2 +-
> >  dlls/msscript.ocx/tests/msscript.c  | 488 ++++++++++++++++++++++++++++++++++++
> >  include/activscp.idl                |  16 ++
> >  4 files changed, 737 insertions(+), 6 deletions(-)
> >
> > diff --git a/dlls/msscript.ocx/msscript.c b/dlls/msscript.ocx/msscript.c
> > index 5a03f0c..b6764f4 100644
> > --- a/dlls/msscript.ocx/msscript.c
> > +++ b/dlls/msscript.ocx/msscript.c
> > @@ -22,6 +22,7 @@
> >  #include "initguid.h"
> >  #include "ole2.h"
> >  #include "olectl.h"
> > +#include "activscp.h"
> >  #include "rpcproxy.h"
> >  #include "msscript.h"
> >
> > @@ -29,14 +30,21 @@
> >
> > WINE_DEFAULT_DEBUG_CHANNEL(msscript);
> >
> > +static const WCHAR vbscriptW[] = {'V','B','S','c','r','i','p','t',0};
> > +static const WCHAR jscriptW[] = {'J','S','c','r','i','p','t',0};
> > -
> >
> > struct ScriptControl {
> >      IScriptControl IScriptControl_iface;
> >      IPersistStreamInit IPersistStreamInit_iface;
> >      IOleObject IOleObject_iface;
> >      IOleControl IOleControl_iface;
> > - IActiveScriptSite IActiveScriptSite_iface;
> > LONG ref;
> > IOleClientSite *site;
> > SIZEL extent;
> > - BSTR lang;
> > - IActiveScript *script;
> > - SCRIPTSTATE script_state;
> >
> > };
>
> It's a good moment to think a bit about the design here. I don't think
> you want to reuse IActiveScriptSite for all script engines you load. A
> separated object would allow better ref counting, would be cleaner and
> safer in my opinion. A quick test on Windows suggests that native does that.

Do you mean that engines ever used in IScriptControl lifetime should be
kept around even though only one can be used at a time? I see that
mshtml does that, keeping a list of hosts per window, but for mshtml the
reason for that is not simply to reuse engine instances. Or should I
simply create a single site per script control object?
> > static HINSTANCE msscript_instance;
> > @@ -120,6 +128,29 @@ static void release_typelib(void)
> >      ITypeLib_Release(typelib);
> >  }
> >
> > +static void release_script_engine(ScriptControl *sc)
> > +{
> > - if (!sc->script)
> > -
> >    return;
> > -
> > - switch (sc->script_state) {
> > - case SCRIPTSTATE_CONNECTED:
> > -
> >    IActiveScript_SetScriptState(sc->script, SCRIPTSTATE_DISCONNECTED);
> > -
> > - case SCRIPTSTATE_STARTED:
> > - case SCRIPTSTATE_DISCONNECTED:
> > - case SCRIPTSTATE_INITIALIZED:
> > -
> >    IActiveScript_Close(sc->script);
> > -
> > - default:
> > -
> >    ;
> > - }
>
> Some "fall through" comments to avoid static analyser fixes follow ups
> would be nice.

Ok.
> > -static HRESULT WINAPI ScriptControl_put_Language(IScriptControl *iface, BSTR language)
> > +static HRESULT WINAPI ScriptControl_put_Language(IScriptControl *iface, BSTR lang)
> >  {
> >      ScriptControl *This = impl_from_IScriptControl(iface);
> > - FIXME("(%p)->(%s)\n", This, debugstr_w(language));
> > - return E_NOTIMPL;
> > - HRESULT hr;
> > - GUID guid;
> > -
> > - TRACE("(%p)->(%s)\n", This, debugstr_w(lang));
> > -
> > - if (!lang) {
> > -
> >    release_script_engine(This);
> > -
> >    set_script_name(This, NULL);
> > -
> >    return S_OK;
> > - }
> > -
> > - hr = CLSIDFromProgID(lang, &guid);
> > - if (FAILED(hr))
> > -
> >    return CTL_E_INVALIDPROPERTYVALUE;
> > -
> > - hr = CoCreateInstance(&guid, NULL, CLSCTX_INPROC_SERVER|CLSCTX_INPROC_HANDLER,
> > -
> >        &IID_IActiveScript, (void**)&This->script);
> > - if (FAILED(hr))
> > -
> >    return CTL_E_INVALIDPROPERTYVALUE;
> > - else if (!init_script_engine(This)) {
> > -
> >    release_script_engine(This);
> > -
> >    return CTL_E_INVALIDPROPERTYVALUE;
> > - }
> > -
> > - /* store engine name */
> > - if (!lstrcmpiW(lang, vbscriptW))
> > -
> >    This->lang = SysAllocString(vbscriptW);
> > - else if (!lstrcmpiW(lang, jscriptW))
> > -
> >    This->lang = SysAllocString(jscriptW);
> > - else
>
> Why do you need those? My guess is that returned script may have
> different upper/lower cases, but if that the case, then we should
> probably use ProgIDFromCLSID in getter and just store CLSID.

There's a test for that. For VBScript/JScript names are returned
capitalized exactly like that. While for custom engines returned name
matches exactly what was set, see in a test it's testscript vs
TestScript registry key.
> It's nice to have tests with emulated script engine, but please also add
> test of called callbacks. Please add something like
> CHECK_EXPECT()/CHECK_CALLED() macros or alike. Otherwise important
> informations that your tests already gather are hidden and not tested.

Will do.
> > diff --git a/include/activscp.idl b/include/activscp.idl
> > index 8a3d75d..e539b04 100644
> > --- a/include/activscp.idl
> > +++ b/include/activscp.idl
> > @@ -71,6 +71,11 @@ typedef enum tagSCRIPTUICHANDLING {
> >      SCRIPTUICHANDLING_NOUIDEFAULT = 2
> >  } SCRIPTUICHANDLING;
> >
> > +typedef enum tagSCRIPTGCTYPE {
> > - SCRIPTGCTYPE_NORMAL     = 0,
> > - SCRIPTGCTYPE_EXHAUSTIVE = 1
> >
> > +} SCRIPTGCTYPE;
> > -
> >
> > typedef DWORD SCRIPTTHREADID;
> >  cpp_quote("#define SCRIPTTHREADID_CURRENT ((SCRIPTTHREADID)-1)")
> >  cpp_quote("#define SCRIPTTHREADID_BASE ((SCRIPTTHREADID)-2)")
> > @@ -551,3 +556,14 @@ interface IActiveScriptProperty : IUnknown
> >              [in] VARIANT *pvarIndex,
> >              [in] VARIANT *pvarValue);
> >  }
> > -
> >
> > +[
> > - object,
> > - uuid(6aa2c4a0-2b53-11d4-a2a0-00104bd35090),
> > - pointer_default(unique)
> >
> > +]
> > +interface IActiveScriptGarbageCollector : IUnknown
> > +{
> > - HRESULT CollectGarbage(
> > -
> >        [in] SCRIPTGCTYPE gctype);
> >
> > +}
>
> It's not too important, but technically it deserves a separated patch.

I'll make it separate.
> Thanks,
> Jacek

[PATCH 2/3] msscript: Added basic script hosting stub support

[Jacek Caban]

Hi Nikolay,

On 06/13/2016 01:25 PM, Nikolay Sivov wrote:
> > It's a good moment to think a bit about the design here. I don't think
> > > you want to reuse IActiveScriptSite for all script engines you load. A
> > > separated object would allow better ref counting, would be cleaner and
> > > safer in my opinion. A quick test on Windows suggests that native does that.
>
> Do you mean that engines ever used in IScriptControl lifetime should be
> kept around even though only one can be used at a time? I see that
> mshtml does that, keeping a list of hosts per window, but for mshtml the
> reason for that is not simply to reuse engine instances. Or should I
> simply create a single site per script control object?

See the attached test for why it needs to be a separated object. The
reason for not reusing this object is that, although script engines
should not use script site after being closed, there is no guarantee
they won't keep references that would be released later. That said, you
should keep reference only to current script site object in script
control, but when you close a script engine, simply detach script site
from script control and let usual COM rules control when it's freed.

Jacek