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