Re: advpack: Only do_ocx_reg (and thus DllRegisterServer) from RegisterOCX when 'N' is passed as a flag. Clarify documentation.
I'm very hesitant about this. MSDN has no documentation about RegisterOCX, so I'm not sure how you're justifying this change. It's been a long time since I worked on this, so I don't remember much, but I do remember testing this method and documenting the parameters correctly. Where are you getting information that 'I' is required when using 'N'? James On Tue, May 11, 2010 at 1:24 PM, Gerald Pfeifer <gerald(a)pfeifer.com> wrote:
This is my humble attempt of addressing Alexandre's feeback at
http://www.winehq.org/pipermail/wine-devel/2010-May/083518.html
It does pass testing for me, even on FreeBSD/i386, but I will say I did not find the MSDN documentation I located too helpful/clear, and may have been misled.
As a next step we'd need to look into DllInstall. This change, hopefully, is a move in the right direction, though. If not, some guidance or one of the pros looking into it will be appreciated.
Gerald
--- dlls/advpack/advpack.c | 14 ++++++++++---- 1 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/dlls/advpack/advpack.c b/dlls/advpack/advpack.c index 112d38a..f040ec6 100644 --- a/dlls/advpack/advpack.c +++ b/dlls/advpack/advpack.c @@ -486,8 +486,10 @@ HRESULT do_ocx_reg(HMODULE hocx, BOOL do_reg) * NOTES * OCX - Filename of the OCX to register. * flags - Controls the operation of RegisterOCX. - * 'I' Call DllRegisterServer and DllInstall. - * 'N' Only call DllInstall. + * 'I' Call DllInstall and, unless 'N' is specified as well, + * DllRegisterServer. + * 'N' Do not call DllRegisterServer; only valid if 'I' is + * specified too. * param - Command line passed to DllInstall. */ HRESULT WINAPI RegisterOCX(HWND hWnd, HINSTANCE hInst, LPCSTR cmdline, INT show) @@ -519,8 +521,12 @@ HRESULT WINAPI RegisterOCX(HWND hWnd, HINSTANCE hInst, LPCSTR cmdline, INT show) if (!hm) goto done;
- hr = do_ocx_reg(hm, TRUE); - + if(strchrW(str_flags,'I')) + { + if (!strchrW(str_flags,'N')) + hr = do_ocx_reg(hm, TRUE); + } + done: FreeLibrary(hm); HeapFree(GetProcessHeap(), 0, cmdline_copy); -- 1.6.6.2
On Tue, 11 May 2010, James Hawkins wrote:
I'm very hesitant about this. MSDN has no documentation about RegisterOCX, so I'm not sure how you're justifying this change. It's been a long time since I worked on this, so I don't remember much, but I do remember testing this method and documenting the parameters correctly. Where are you getting information that 'I' is required when using 'N'?
I had to look again, and it seems the best I managed to find is the following: http://msdn.microsoft.com/en-us/library/bb759846%28VS.85%29.aspx If you experimentally verified something differently I will be happy to follow that. However, I did not found the original documentation sufficiently clear to really use it as a specification to base the implementation on. Specific questions I ran into were: - what happens if none of these are specified? - can the string contain more than one character? (pressumably yes?) - what happens if both are specified? Would you feel more comfortable leaving the documentation as is and me just suggesting the following? if(strchrW(str_flags,'I')) hr = do_ocx_reg(hm, TRUE); to replace hr = do_ocx_reg(hm, TRUE); ? Or would you prefer to submit a patch to clarify the documentation (copying me) and based on that I hack the code? That way we'd ensure that the former is sufficiently clear and I assume you'll verify whether the code matches that? Whatever works for you as long as we make progress. :-) Gerald
Hi James, On Thu, 13 May 2010, Gerald Pfeifer wrote:
Would you feel more comfortable leaving the documentation as is and me just suggesting the following?
if(strchrW(str_flags,'I')) hr = do_ocx_reg(hm, TRUE);
to replace
hr = do_ocx_reg(hm, TRUE);
?
Or would you prefer to submit a patch to clarify the documentation (copying me) and based on that I hack the code? That way we'd ensure that the former is sufficiently clear and I assume you'll verify whether the code matches that?
I just realized I did not see a response to this. How about the patch below that actually matches current documentation (which the current code does not seem to)? Gerald ChangeLog: Only register if I has been passed as a flag. --- dlls/advpack/advpack.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/dlls/advpack/advpack.c b/dlls/advpack/advpack.c index 112d38a..c57933b 100644 --- a/dlls/advpack/advpack.c +++ b/dlls/advpack/advpack.c @@ -519,7 +519,8 @@ HRESULT WINAPI RegisterOCX(HWND hWnd, HINSTANCE hInst, LPCSTR cmdline, INT show) if (!hm) goto done; - hr = do_ocx_reg(hm, TRUE); + if(strchrW(str_flags,'I')) + hr = do_ocx_reg(hm, TRUE); done: FreeLibrary(hm); -- 1.7.6
I recommend writing unit tests to answer the open questions. Thanks, James On Sat, Sep 24, 2011 at 2:46 PM, Gerald Pfeifer <gerald(a)pfeifer.com> wrote:
Hi James,
On Thu, 13 May 2010, Gerald Pfeifer wrote:
Would you feel more comfortable leaving the documentation as is and me just suggesting the following?
if(strchrW(str_flags,'I')) hr = do_ocx_reg(hm, TRUE);
to replace
hr = do_ocx_reg(hm, TRUE);
?
Or would you prefer to submit a patch to clarify the documentation (copying me) and based on that I hack the code? That way we'd ensure that the former is sufficiently clear and I assume you'll verify whether the code matches that?
I just realized I did not see a response to this. How about the patch below that actually matches current documentation (which the current code does not seem to)?
Gerald
ChangeLog: Only register if I has been passed as a flag.
--- dlls/advpack/advpack.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/dlls/advpack/advpack.c b/dlls/advpack/advpack.c index 112d38a..c57933b 100644 --- a/dlls/advpack/advpack.c +++ b/dlls/advpack/advpack.c @@ -519,7 +519,8 @@ HRESULT WINAPI RegisterOCX(HWND hWnd, HINSTANCE hInst, LPCSTR cmdline, INT show) if (!hm) goto done;
- hr = do_ocx_reg(hm, TRUE); + if(strchrW(str_flags,'I')) + hr = do_ocx_reg(hm, TRUE);
done: FreeLibrary(hm); -- 1.7.6
participants (2)
-
Gerald Pfeifer -
James Hawkins