Re: msi: Add full JScript/VBScript support and partial expandable OLE automation support. [PATCH 3/3]

[Robert Shearman]

Misha Koshelev wrote:
> +/* Macros to get pointer to AutomationObject) from the other VTables. */

Typo with extra ")" here.
> +HRESULT WINAPI SessionImpl_Invoke(
> -
>    AutomationObject* This,
> -
>    DISPID dispIdMember,
> -
>    REFIID riid,
> -
>    LCID lcid,
> -
>    WORD wFlags,
> -
>    DISPPARAMS* pDispParams,
> -
>    VARIANT* pVarResult,
> -
>    EXCEPINFO* pExcepInfo,
> -
>    UINT* puArgErr)
>
> +{
> - WCHAR szString[MAX_MSI_STRING];
> - DWORD dwLen = MAX_MSI_STRING;
> - IDispatch *iDispatch = NULL;
> - MSIHANDLE msiHandle;
> - LANGID langId;
> - UINT ret;
> - INSTALLSTATE iInstalled, iAction;

Some error checking here would be good, like you do in 
AutomationObject_Invoke.
> -
> - switch (dispIdMember)
> - {

...
> +/*
> -
> - AutomationObject - "base" class for all automation objects so we don't have to repeat functions. Just
> -
> -
>                need to implement Invoke function for each dispinterface and pass the new function
> -
> -
>                to create_automation_object.
> - */
> -
>
> +typedef struct {
> - /*
> -
> * VTables - We provide IDispatch, IProvideClassInfo, IProvideClassInfo2, IProvideMultipleClassInfo
> -
> */
> - const IDispatchVtbl *lpVtbl;
> - const IProvideClassInfoVtbl *lpvtblIProvideClassInfo;
> - const IProvideClassInfo2Vtbl *lpvtblIProvideClassInfo2;
> - const IProvideMultipleClassInfoVtbl *lpvtblIProvideMultipleClassInfo;
> -
> - /* Object reference count */
> - LONG ref;
> -
> - /* Clsid for this class and it's appropriate ITypeInfo object */
> - LPCLSID clsid;
> - ITypeInfo *iTypeInfo;
> -
> - /* The MSI handle of the current object */
> - MSIHANDLE msiHandle;;
> -
> - /* A function that is called from IDispatch::Invoke, specific to this type of object */
> - LPVOID funcInvoke;
>
> +} AutomationObject;

You shouldn't need to expose the implementation details of 
AutomationObject outside of automation.c.
> -
>
> +/* This is the function that one needs to call to create an automation object. */
> +extern HRESULT create_automation_object(MSIHANDLE msiHandle, IUnknown *pUnkOuter, LPVOID *ppObj, REFIID clsid, LPVOID funcInvoke);

It is dangerous to use LPVOID as the type to pass a function in.
> -
>
> +/* We need to expose these functions because our IActiveScriptSite calls it */
> +extern HRESULT WINAPI LoadTypeInfo(IDispatch *iface, ITypeInfo **pptinfo, REFIID clsid, LCID lcid);
> +extern HRESULT WINAPI SessionImpl_Invoke(AutomationObject*,DISPID,REFIID,LCID,WORD,DISPPARAMS*,VARIANT*,EXCEPINFO*,UINT*);
> -
>
> +/* Disp Ids
> -
> - (not complete, look in msiserver.idl to add more) */
>
> +typedef enum {
> - RecordDispId_StringData=1,
> - RecordDispId_IntegerData,
> - RecordDispId_SetStream,
> - RecordDispId_ReadStream,
> - RecordDispId_FieldCount,
> - RecordDispId_IsNull,
> - RecordDispId_DataSize,
> - RecordDispId_ClearData,
> - RecordDispId_FormatText
>
> +} RecordDispId;
> -
>
> +typedef enum {
> - ViewDispId_Execute=1,
> - ViewDispId_Fetch,
> - ViewDispId_Modify,
> - ViewDispId_Close,
> - ViewDispId_ColumnInfo,
> - ViewDispId_GetError
>
> +} ViewDispId;
> -
>
> +typedef enum {
> - DatabaseDispId_DatabaseState=1,
> - DatabaseDispId_SummaryInformation,
> - DatabaseDispId_OpenView,
> - DatabaseDispId_Commit,
> - DatabaseDispId_PrimaryKeys,
> - DatabaseDispId_Import,
> - DatabaseDispId_Export,
> - DatabaseDispId_Merge,
> - DatabaseDispId_GenerateTransform,
> - DatabaseDispId_ApplyTransform,
> - DatabaseDispId_EnableUIPreview,
> - DatabaseDispId_TablePersistent,
> - DatabaseDispId_CreateTransformSummaryInfo
>
> +} DatabaseDispId;
> -
>
> +typedef enum {
> - SessionDispId_Installer=1,
> - SessionDispId_Property,
> - SessionDispId_Language,
> - SessionDispId_Mode,
> - SessionDispId_Database,
> - SessionDispId_SourcePath,
> - SessionDispId_TargetPath,
> - SessionDispId_DoAction,
> - SessionDispId_Sequence,
> - SessionDispId_EvaluateCondition,
> - SessionDispId_FormatRecord,
> - SessionDispId_Message,
> - SessionDispId_FeatureCurrentState,
> - SessionDispId_FeatureRequestState,
> - SessionDispId_FeatureValidStates,
> - SessionDispId_FeatureCost,
> - SessionDispId_ComponentCurrentState,
> - SessionDispId_ComponentRequestState,
> - SessionDispId_SetInstallLevel,
> - SessionDispId_VerifyDiskSpace,
> - SessionDispId_ProductProperty,
> - SessionDispId_FeatureInfo,
> - SessionDispId_ComponentCost
>
> +} SessionDispId;

See dlls/oleaut32/tests/tmarshal_dispids.h and 
dlls/oleaut32/tests/tmarshal.idl for how to use DISPIDs properly.
> +typedef struct {
> - IActiveScriptSite lpVtbl;
> - AutomationObject *session;

You don't actually access this as anything other than an IDispatch 
object, so you might as well change to "IDispatch *session".
> +LPCWSTR read_script_from_file(LPCWSTR szFile, INT type)

Should be static.
> +/* JScript or VBScript? */
> +LPCWSTR progid_from_type(INT type)

Should also be static.
> -
>
> +/*
> -
> - Call a script. This is our meat and potatoes.
> -
> -
> - Currently, since the function is relatively new, it will always end up returning S_OK.
> -
> -
>   Think of it like a bonus feature, we can run the script - great. If we have a problem,
> -
> -
>   we are no worse off than if this function had not been called.
> - */
>
> +DWORD call_script(MSIHANDLE hPackage, INT type, LPCWSTR filename, LPCWSTR function, LPCWSTR action)
> +{
> - LPCWSTR script = NULL, progId = NULL;
> - HRESULT hr;
> - IActiveScript *iActiveScript = NULL;
> - IActiveScriptParse *iActiveScriptParse = NULL;

I haven't seen this variable notation style before. In Hungarian 
notation "p" is used instead of "i" to show a pointer to an object 
exposing the interface declared.

msi: Add full JScript/VBScript support and partial expandable OLE automation support. [PATCH 3/3]

[Misha Koshelev]

On Sun, 2007-02-25 at 13:00 +0000, Robert Shearman wrote:
> Misha Koshelev wrote:
> > +/* Macros to get pointer to AutomationObject) from the other VTables. */
>
> Typo with extra ")" here.
> > +HRESULT WINAPI SessionImpl_Invoke(
> > -
> >    AutomationObject* This,
> > -
> >    DISPID dispIdMember,
> > -
> >    REFIID riid,
> > -
> >    LCID lcid,
> > -
> >    WORD wFlags,
> > -
> >    DISPPARAMS* pDispParams,
> > -
> >    VARIANT* pVarResult,
> > -
> >    EXCEPINFO* pExcepInfo,
> > -
> >    UINT* puArgErr)
> >
> > +{
> > - WCHAR szString[MAX_MSI_STRING];
> > - DWORD dwLen = MAX_MSI_STRING;
> > - IDispatch *iDispatch = NULL;
> > - MSIHANDLE msiHandle;
> > - LANGID langId;
> > - UINT ret;
> > - INSTALLSTATE iInstalled, iAction;
>
> Some error checking here would be good, like you do in 
> AutomationObject_Invoke.

Well perhaps there is a little confusion here as these functions are
actually always called from _within_ AutomationObject_Invoke, hence the
error checking is already done. I did this as there is quite a few
objects to implement and to save writing the same code over and over I
thought this would be a good idea. Do you think I should rename these
functions to something else so it is more clear that they are not
actually called directly through an IDispatch interface but rather from
an AutomationObject?
> > -
> > - switch (dispIdMember)
> > - {
>
> ...
> > +/*
> > -
> > - AutomationObject - "base" class for all automation objects so we don't have to repeat functions. Just
> > -
> > -
> >                need to implement Invoke function for each dispinterface and pass the new function
> > -
> > -
> >                to create_automation_object.
> > - */
> > -
> >
> > +typedef struct {
> > - /*
> > -
> > * VTables - We provide IDispatch, IProvideClassInfo, IProvideClassInfo2, IProvideMultipleClassInfo
> > -
> > */
> > - const IDispatchVtbl *lpVtbl;
> > - const IProvideClassInfoVtbl *lpvtblIProvideClassInfo;
> > - const IProvideClassInfo2Vtbl *lpvtblIProvideClassInfo2;
> > - const IProvideMultipleClassInfoVtbl *lpvtblIProvideMultipleClassInfo;
> > -
> > - /* Object reference count */
> > - LONG ref;
> > -
> > - /* Clsid for this class and it's appropriate ITypeInfo object */
> > - LPCLSID clsid;
> > - ITypeInfo *iTypeInfo;
> > -
> > - /* The MSI handle of the current object */
> > - MSIHANDLE msiHandle;;
> > -
> > - /* A function that is called from IDispatch::Invoke, specific to this type of object */
> > - LPVOID funcInvoke;
> >
> > +} AutomationObject;
>
> You shouldn't need to expose the implementation details of 
> AutomationObject outside of automation.c.
> > -
> >
> > +/* This is the function that one needs to call to create an automation object. */
> > +extern HRESULT create_automation_object(MSIHANDLE msiHandle, IUnknown *pUnkOuter, LPVOID *ppObj, REFIID clsid, LPVOID funcInvoke);
>
> It is dangerous to use LPVOID as the type to pass a function in.
> > -
> >
> > +/* We need to expose these functions because our IActiveScriptSite calls it */
> > +extern HRESULT WINAPI LoadTypeInfo(IDispatch *iface, ITypeInfo **pptinfo, REFIID clsid, LCID lcid);
> > +extern HRESULT WINAPI SessionImpl_Invoke(AutomationObject*,DISPID,REFIID,LCID,WORD,DISPPARAMS*,VARIANT*,EXCEPINFO*,UINT*);
> > -
> >
> > +/* Disp Ids
> > -
> > - (not complete, look in msiserver.idl to add more) */
> >
> > +typedef enum {
> > - RecordDispId_StringData=1,
> > - RecordDispId_IntegerData,
> > - RecordDispId_SetStream,
> > - RecordDispId_ReadStream,
> > - RecordDispId_FieldCount,
> > - RecordDispId_IsNull,
> > - RecordDispId_DataSize,
> > - RecordDispId_ClearData,
> > - RecordDispId_FormatText
> >
> > +} RecordDispId;
> > -
> >
> > +typedef enum {
> > - ViewDispId_Execute=1,
> > - ViewDispId_Fetch,
> > - ViewDispId_Modify,
> > - ViewDispId_Close,
> > - ViewDispId_ColumnInfo,
> > - ViewDispId_GetError
> >
> > +} ViewDispId;
> > -
> >
> > +typedef enum {
> > - DatabaseDispId_DatabaseState=1,
> > - DatabaseDispId_SummaryInformation,
> > - DatabaseDispId_OpenView,
> > - DatabaseDispId_Commit,
> > - DatabaseDispId_PrimaryKeys,
> > - DatabaseDispId_Import,
> > - DatabaseDispId_Export,
> > - DatabaseDispId_Merge,
> > - DatabaseDispId_GenerateTransform,
> > - DatabaseDispId_ApplyTransform,
> > - DatabaseDispId_EnableUIPreview,
> > - DatabaseDispId_TablePersistent,
> > - DatabaseDispId_CreateTransformSummaryInfo
> >
> > +} DatabaseDispId;
> > -
> >
> > +typedef enum {
> > - SessionDispId_Installer=1,
> > - SessionDispId_Property,
> > - SessionDispId_Language,
> > - SessionDispId_Mode,
> > - SessionDispId_Database,
> > - SessionDispId_SourcePath,
> > - SessionDispId_TargetPath,
> > - SessionDispId_DoAction,
> > - SessionDispId_Sequence,
> > - SessionDispId_EvaluateCondition,
> > - SessionDispId_FormatRecord,
> > - SessionDispId_Message,
> > - SessionDispId_FeatureCurrentState,
> > - SessionDispId_FeatureRequestState,
> > - SessionDispId_FeatureValidStates,
> > - SessionDispId_FeatureCost,
> > - SessionDispId_ComponentCurrentState,
> > - SessionDispId_ComponentRequestState,
> > - SessionDispId_SetInstallLevel,
> > - SessionDispId_VerifyDiskSpace,
> > - SessionDispId_ProductProperty,
> > - SessionDispId_FeatureInfo,
> > - SessionDispId_ComponentCost
> >
> > +} SessionDispId;
>
> See dlls/oleaut32/tests/tmarshal_dispids.h and 
> dlls/oleaut32/tests/tmarshal.idl for how to use DISPIDs properly.
> > +typedef struct {
> > - IActiveScriptSite lpVtbl;
> > - AutomationObject *session;
>
> You don't actually access this as anything other than an IDispatch 
> object, so you might as well change to "IDispatch *session".
> > +LPCWSTR read_script_from_file(LPCWSTR szFile, INT type)
>
> Should be static.
> > +/* JScript or VBScript? */
> > +LPCWSTR progid_from_type(INT type)
>
> Should also be static.
> > -
> >
> > +/*
> > -
> > - Call a script. This is our meat and potatoes.
> > -
> > -
> > - Currently, since the function is relatively new, it will always end up returning S_OK.
> > -
> > -
> >   Think of it like a bonus feature, we can run the script - great. If we have a problem,
> > -
> > -
> >   we are no worse off than if this function had not been called.
> > - */
> >
> > +DWORD call_script(MSIHANDLE hPackage, INT type, LPCWSTR filename, LPCWSTR function, LPCWSTR action)
> > +{
> > - LPCWSTR script = NULL, progId = NULL;
> > - HRESULT hr;
> > - IActiveScript *iActiveScript = NULL;
> > - IActiveScriptParse *iActiveScriptParse = NULL;
>
> I haven't seen this variable notation style before. In Hungarian 
> notation "p" is used instead of "i" to show a pointer to an object 
> exposing the interface declared.

Other comments sound good, I will make appropriate changes. I think I
will make patch 2 the scripting functions with a dummy call_script that
just does a TRACE, and then patch 3 will apply automation and scripting.

Misha

msi: Add full JScript/VBScript support and partial expandable OLE automation support. [PATCH 3/3]

[Misha Koshelev]

On Sun, 2007-02-25 at 13:00 +0000, Robert Shearman wrote:
> Misha Koshelev wrote:
> > +/* Macros to get pointer to AutomationObject) from the other VTables. */
>
> Typo with extra ")" here.
> > +HRESULT WINAPI SessionImpl_Invoke(
> > -
> >    AutomationObject* This,
> > -
> >    DISPID dispIdMember,
> > -
> >    REFIID riid,
> > -
> >    LCID lcid,
> > -
> >    WORD wFlags,
> > -
> >    DISPPARAMS* pDispParams,
> > -
> >    VARIANT* pVarResult,
> > -
> >    EXCEPINFO* pExcepInfo,
> > -
> >    UINT* puArgErr)
> >
> > +{
> > - WCHAR szString[MAX_MSI_STRING];
> > - DWORD dwLen = MAX_MSI_STRING;
> > - IDispatch *iDispatch = NULL;
> > - MSIHANDLE msiHandle;
> > - LANGID langId;
> > - UINT ret;
> > - INSTALLSTATE iInstalled, iAction;
>
> Some error checking here would be good, like you do in 
> AutomationObject_Invoke.
> > -
> > - switch (dispIdMember)
> > - {
>
> ...
> > +/*
> > -
> > - AutomationObject - "base" class for all automation objects so we don't have to repeat functions. Just
> > -
> > -
> >                need to implement Invoke function for each dispinterface and pass the new function
> > -
> > -
> >                to create_automation_object.
> > - */
> > -
> >
> > +typedef struct {
> > - /*
> > -
> > * VTables - We provide IDispatch, IProvideClassInfo, IProvideClassInfo2, IProvideMultipleClassInfo
> > -
> > */
> > - const IDispatchVtbl *lpVtbl;
> > - const IProvideClassInfoVtbl *lpvtblIProvideClassInfo;
> > - const IProvideClassInfo2Vtbl *lpvtblIProvideClassInfo2;
> > - const IProvideMultipleClassInfoVtbl *lpvtblIProvideMultipleClassInfo;
> > -
> > - /* Object reference count */
> > - LONG ref;
> > -
> > - /* Clsid for this class and it's appropriate ITypeInfo object */
> > - LPCLSID clsid;
> > - ITypeInfo *iTypeInfo;
> > -
> > - /* The MSI handle of the current object */
> > - MSIHANDLE msiHandle;;
> > -
> > - /* A function that is called from IDispatch::Invoke, specific to this type of object */
> > - LPVOID funcInvoke;
> >
> > +} AutomationObject;
>
> You shouldn't need to expose the implementation details of 
> AutomationObject outside of automation.c.
> > -
> >
> > +/* This is the function that one needs to call to create an automation object. */
> > +extern HRESULT create_automation_object(MSIHANDLE msiHandle, IUnknown *pUnkOuter, LPVOID *ppObj, REFIID clsid, LPVOID funcInvoke);
>
> It is dangerous to use LPVOID as the type to pass a function in.
> > -
> >
> > +/* We need to expose these functions because our IActiveScriptSite calls it */
> > +extern HRESULT WINAPI LoadTypeInfo(IDispatch *iface, ITypeInfo **pptinfo, REFIID clsid, LCID lcid);
> > +extern HRESULT WINAPI SessionImpl_Invoke(AutomationObject*,DISPID,REFIID,LCID,WORD,DISPPARAMS*,VARIANT*,EXCEPINFO*,UINT*);
> > -
> >
> > +/* Disp Ids
> > -
> > - (not complete, look in msiserver.idl to add more) */
> >
> > +typedef enum {
> > - RecordDispId_StringData=1,
> > - RecordDispId_IntegerData,
> > - RecordDispId_SetStream,
> > - RecordDispId_ReadStream,
> > - RecordDispId_FieldCount,
> > - RecordDispId_IsNull,
> > - RecordDispId_DataSize,
> > - RecordDispId_ClearData,
> > - RecordDispId_FormatText
> >
> > +} RecordDispId;
> > -
> >
> > +typedef enum {
> > - ViewDispId_Execute=1,
> > - ViewDispId_Fetch,
> > - ViewDispId_Modify,
> > - ViewDispId_Close,
> > - ViewDispId_ColumnInfo,
> > - ViewDispId_GetError
> >
> > +} ViewDispId;
> > -
> >
> > +typedef enum {
> > - DatabaseDispId_DatabaseState=1,
> > - DatabaseDispId_SummaryInformation,
> > - DatabaseDispId_OpenView,
> > - DatabaseDispId_Commit,
> > - DatabaseDispId_PrimaryKeys,
> > - DatabaseDispId_Import,
> > - DatabaseDispId_Export,
> > - DatabaseDispId_Merge,
> > - DatabaseDispId_GenerateTransform,
> > - DatabaseDispId_ApplyTransform,
> > - DatabaseDispId_EnableUIPreview,
> > - DatabaseDispId_TablePersistent,
> > - DatabaseDispId_CreateTransformSummaryInfo
> >
> > +} DatabaseDispId;
> > -
> >
> > +typedef enum {
> > - SessionDispId_Installer=1,
> > - SessionDispId_Property,
> > - SessionDispId_Language,
> > - SessionDispId_Mode,
> > - SessionDispId_Database,
> > - SessionDispId_SourcePath,
> > - SessionDispId_TargetPath,
> > - SessionDispId_DoAction,
> > - SessionDispId_Sequence,
> > - SessionDispId_EvaluateCondition,
> > - SessionDispId_FormatRecord,
> > - SessionDispId_Message,
> > - SessionDispId_FeatureCurrentState,
> > - SessionDispId_FeatureRequestState,
> > - SessionDispId_FeatureValidStates,
> > - SessionDispId_FeatureCost,
> > - SessionDispId_ComponentCurrentState,
> > - SessionDispId_ComponentRequestState,
> > - SessionDispId_SetInstallLevel,
> > - SessionDispId_VerifyDiskSpace,
> > - SessionDispId_ProductProperty,
> > - SessionDispId_FeatureInfo,
> > - SessionDispId_ComponentCost
> >
> > +} SessionDispId;
>
> See dlls/oleaut32/tests/tmarshal_dispids.h and 
> dlls/oleaut32/tests/tmarshal.idl for how to use DISPIDs properly.
> > +typedef struct {
> > - IActiveScriptSite lpVtbl;
> > - AutomationObject *session;
>
> You don't actually access this as anything other than an IDispatch 
> object, so you might as well change to "IDispatch *session".
> > +LPCWSTR read_script_from_file(LPCWSTR szFile, INT type)
>
> Should be static.
> > +/* JScript or VBScript? */
> > +LPCWSTR progid_from_type(INT type)
>
> Should also be static.
> > -
> >
> > +/*
> > -
> > - Call a script. This is our meat and potatoes.
> > -
> > -
> > - Currently, since the function is relatively new, it will always end up returning S_OK.
> > -
> > -
> >   Think of it like a bonus feature, we can run the script - great. If we have a problem,
> > -
> > -
> >   we are no worse off than if this function had not been called.
> > - */
> >
> > +DWORD call_script(MSIHANDLE hPackage, INT type, LPCWSTR filename, LPCWSTR function, LPCWSTR action)
> > +{
> > - LPCWSTR script = NULL, progId = NULL;
> > - HRESULT hr;
> > - IActiveScript *iActiveScript = NULL;
> > - IActiveScriptParse *iActiveScriptParse = NULL;
>
> I haven't seen this variable notation style before. In Hungarian 
> notation "p" is used instead of "i" to show a pointer to an object 
> exposing the interface declared.

I have implemented all the changes you outlined (thanks a lot) in this
and your other email, with the exception of the extra error checking in
each of the individual Invoke functions as they are called from within
AutomationObject::Invoke after the error checking already occurs. What
do you think?

Thanks
Misha

p.s. The 5th patch is not a part of my submission, but is just a hack I
used to make the IDL file with the dispids using oleview. This patch
will make oleview use the DISPID_PARENT_MEMBER notation for ids and will
output the include file to standard output. Thought maybe it would be
useful for someone.