Re: [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