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