2009/3/6 Rein Klazes wijn@online.nl:
Hi,
Several rounds of commits have gone by without this patch. I thought it was rather noncontroversial, there must be something that I am overlooking. Can I get a hint please?
(cleaned up some white space errors, otherwise patch is unchanged. )
Fix another bug in the application of bug#9615. TrackMenuPopupEx is called with a NULL menu handle, and the application crashes on the returned NULL value in the following INITMENU message.
for the Changelog: Check for invalid menu handle passed to TrackPopupMenu and TrackPopupMenuEx. Add conformance tests for the correct handling of those invalid handles. TrackPopupMenu should call TrackPopupMenuEx, not the other way.
NOTE: I am going to refer to TrackPopupMenu as TPM and TrackPopupMenuEx as TPMEx in the rest of this reply.
You probably want to split this up into 3 patches. Add the tests first - with appropriate todo_wine entries, then the fix, then the TPM -> TPMEx change.
= Implementation Comments:
+ /* FIXME: this check is performed several times, here and in the called + functions. That could be optimized */ + if( !hMenu || !MENU_GetMenu( hMenu )) { + SetLastError( ERROR_INVALID_MENU_HANDLE) ; + return FALSE; + }
Which called functions are doing this check? You should do the SetLastError call there instead (in MENU_TrackMenu?).
-BOOL WINAPI TrackPopupMenuEx( HMENU hMenu, UINT wFlags, INT x, INT y, - HWND hWnd, LPTPMPARAMS lpTpm ) +BOOL WINAPI TrackPopupMenu( HMENU hMenu, UINT wFlags, INT x, INT y, + INT nReserved, HWND hWnd, const RECT *lpRect ) { - FIXME("not fully implemented\n" );
Why are you removing this FIXME? Do you know that your changes fully implement TPMEx (which does not have the FIXME in your patch)?
- return TrackPopupMenu( hMenu, wFlags, x, y, 0, hWnd, - lpTpm ? &lpTpm->rcExclude : NULL ); + return TrackPopupMenuEx( hMenu, wFlags, x, y, hWnd, NULL);
What is the rationale behind calling TPMEx from TPM? Is it so you can properly implement something in TPMEx that is not provided in TPM? If so, are there any tests to show that there are differences that require this change?
You have two changes in this patch: fixing the last error value and calling TPMEx from TPM. These need to be separate patches as it is difficult to see what the changes are.
+static int gflag_initmenupopup,
Consider calling this something like gcalled_initmenupopup.
+/* some TrackPopupMenu and TrackPopupMenuEx tests */ +/* the LastError values differ between NO_ERROR and invalid handle */ +/* between all windows versions tested. The first value is that valid on XP */ +/* Vista was the only that made returned different error values */ +/* between the TrackPopupMenu and TrackPopupMenuEx functions */
This is not needed - the code tells us this.
+ SetWindowLongPtr( hwnd, GWLP_WNDPROC, (LONG_PTR)menu_ownerdraw_wnd_proc);
Why are you using the ownerdraw class? Create one for the TPM/TPMEx tests.
+ for( Ex = 0; Ex < 2; Ex++) {
Reading the tests and trying to understand what they are doing is confusing. It is better to split it out into a TMP and a TPMEx test function (no MyTrackPopupMenu) like the SHCreateStreamOnFileA/W/Ex tests are doing (http://source.winehq.org/git/wine.git/?a=blob;f=dlls/shlwapi/tests/istream.c...).
You can add a trace at the top of each function so you don't need to write out which TPM/TPMEx function you are testing in every call (the Ex ? "Ex" : "" looks ugly - expecially in every trace call). Splitting it out also allows you later to do TPM or TPMEx specific tests that show the differences between these functions.
+ /* display the menu */
This comment is redundant.
+ /* start with an invalid menu handle */
Consider removing the "start with", e.g. "invalid argument tests".
+ ok( !(gflag_initmenupopup || gflag_entermenuloop || gflag_initmenu), + "got unexpected message(s)%s%s%s\n", + gflag_initmenupopup ? " WM_INITMENUPOPUP ": " ", + gflag_entermenuloop ? "WM_INITMENULOOP ": "", + gflag_initmenu ? "WM_INITMENU": "");
Break this out into 3 ok checks -- ok(!gflag_initmenupopup, "WM_INITMENUPOPUP should not be called on invalid arguments.").
+ ok( gle == NO_ERROR + || gle == ERROR_INVALID_MENU_HANDLE /* NT4, win2k and Vista in the TrackPopupMenuEx case */
In one of the above calls you have marked the NO_ERROR as broken. Why the difference?
= General
Use blank lines to separate out the blocks of functionality (see other tests, like the istream.c or ordinal.c (also in shlwapi) tests for examples) -- we are not constrained by the size of the source code, especially in tests. Readability is paramount in the tests.
Since you are noting message sequences, see http://source.winehq.org/git/wine.git/?a=blob;f=dlls/comctl32/tests/monthcal... for an example of how to do message sequence tests.
= Minor Style / Grammar Nits:
+ /* FIXME: this check is performed several times, here and in the called + functions. That could be optimized */
You are missing a '.' at the end of the sentence.
+ SetLastError( ERROR_INVALID_MENU_HANDLE) ;
The spacing is inconsistent here. The space at the end should be before the ), not after it.
+ ok( ret, "AppendMenA has failed!\n");
Typo: should be AppendMenuA.
- Reece
Reece Dunn schreef:
= Implementation Comments:
- /* FIXME: this check is performed several times, here and in the called
functions. That could be optimized */- if( !hMenu || !MENU_GetMenu( hMenu )) {
SetLastError( ERROR_INVALID_MENU_HANDLE) ;return FALSE;- }
Which called functions are doing this check? You should do the SetLastError call there instead (in MENU_TrackMenu?).
No, the check must be here, before any message's have been sent.
-BOOL WINAPI TrackPopupMenuEx( HMENU hMenu, UINT wFlags, INT x, INT y,
HWND hWnd, LPTPMPARAMS lpTpm )+BOOL WINAPI TrackPopupMenu( HMENU hMenu, UINT wFlags, INT x, INT y,
INT nReserved, HWND hWnd, const RECT *lpRect ){
- FIXME("not fully implemented\n" );
Why are you removing this FIXME? Do you know that your changes fully implement TPMEx (which does not have the FIXME in your patch)?
No. But the FIXME is referring to what I consider a cosmetic feature. Since I noticed that it is confusing users, that link these messages to the crashes and other nasties that I am trying to fix, I decided to not to bother them with it any more. But a WARN might be considered if you realy realy want it.
- return TrackPopupMenu( hMenu, wFlags, x, y, 0, hWnd,
lpTpm ? &lpTpm->rcExclude : NULL );
- return TrackPopupMenuEx( hMenu, wFlags, x, y, hWnd, NULL);
What is the rationale behind calling TPMEx from TPM? Is it so you can properly implement something in TPMEx that is not provided in TPM? If so, are there any tests to show that there are differences that require this change?
I think here the MSDN documentation should suffice for now. the Ex version does implement the excluded rectangle, the plain version ignores it.
Rein.
2009/3/6 Rein Klazes wijn@online.nl:
Reece Dunn schreef:
= Implementation Comments:
- /* FIXME: this check is performed several times, here and in the
called
- functions. That could be optimized */
- if( !hMenu || !MENU_GetMenu( hMenu )) {
- SetLastError( ERROR_INVALID_MENU_HANDLE) ;
- return FALSE;
- }
Which called functions are doing this check? You should do the SetLastError call there instead (in MENU_TrackMenu?).
No, the check must be here, before any message's have been sent.
So do the checks in MENU_InitTracking instead.
-BOOL WINAPI TrackPopupMenuEx( HMENU hMenu, UINT wFlags, INT x, INT y,
- HWND hWnd, LPTPMPARAMS lpTpm )
+BOOL WINAPI TrackPopupMenu( HMENU hMenu, UINT wFlags, INT x, INT y,
- INT nReserved, HWND hWnd, const RECT *lpRect )
{
- FIXME("not fully implemented\n" );
Why are you removing this FIXME? Do you know that your changes fully implement TPMEx (which does not have the FIXME in your patch)?
No. But the FIXME is referring to what I consider a cosmetic feature. Since I noticed that it is confusing users, that link these messages to the crashes and other nasties that I am trying to fix, I decided to not to bother them with it any more. But a WARN might be considered if you realy realy want it.
Ok. So remove the FIXME in a separate patch.
- return TrackPopupMenu( hMenu, wFlags, x, y, 0, hWnd,
- lpTpm ? &lpTpm->rcExclude : NULL );
- return TrackPopupMenuEx( hMenu, wFlags, x, y, hWnd, NULL);
What is the rationale behind calling TPMEx from TPM? Is it so you can properly implement something in TPMEx that is not provided in TPM? If so, are there any tests to show that there are differences that require this change?
I think here the MSDN documentation should suffice for now. the Ex version does implement the excluded rectangle, the plain version ignores it.
MSDN states that for TrackPopupMenu (http://msdn.microsoft.com/en-us/library/ms648002%28VS.85%29.aspx) nReserved must be 0 and lpRect is ignored. TrackPopupMenuEx has an optional (LP)TPMPARAMS structure that just passes an exclude RECT.
Calling TrackPopupMenuEx from TrackPopupMenu with LPTPMPARAMS being NULL is a valid option. But so is passing the current Wine implementation (possibly using a value of 1 or something in the reserved value to signal that this is internal Wine code, not external application code).
You need tests to prove this, though. Until you do, the existing Wine implementation is valid. So: 1. does TrackPopupMenu really ignore the RECT argument? 2. what happens if you pass a nReserved value that is not 0?
- Reece
-
Reece Dunn -
Rein Klazes