Resubmit user32: TrackPopupMenu(Ex) fixes

[Reece Dunn]

2009/3/6 Rein Klazes <wijn at 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;h=66465b84a4ed90c3c1a5987398e8859a1959163d;hb=3db77ce50b9dfa811966afe15604ce2ee3e20c8e).

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.c;h=cccb1b9ea0b680fe5c55c1d582e08b37b0d25d7e;hb=3db77ce50b9dfa811966afe15604ce2ee3e20c8e
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