user32: Fix error handling in {Begin,End,}DeferWindowPos() to match Windows behavior (resend)
Rodrigo Rivas
rodrigorivascosta at gmail.com
Tue Aug 25 17:17:30 CDT 2015
On Tue, Aug 25, 2015 at 4:31 PM, Sebastian Lackner
<sebastian at fds-team.de> wrote:
> I also think moving the tests to an existing test file would be better, but here a few
> additional comments:
>
> On 25.08.2015 15:08, Rodrigo Rivas wrote:
>> diff --git a/dlls/user32/tests/Makefile.in b/dlls/user32/tests/Makefile.in
>> index 7149dc8..982b0fe 100644
>> --- a/dlls/user32/tests/Makefile.in
>> +++ b/dlls/user32/tests/Makefile.in
>> @@ -9,6 +9,7 @@ C_SRCS = \
>> cursoricon.c \
>> dce.c \
>> dde.c \
>> + defer.c \
>> dialog.c \
>> edit.c \
>> generated.c \
>> diff --git a/dlls/user32/tests/defer.c b/dlls/user32/tests/defer.c
>> new file mode 100644
>> index 0000000..eb63384
>> --- /dev/null
>> +++ b/dlls/user32/tests/defer.c
>> @@ -0,0 +1,168 @@
>> +/*
>> + * Unit tests for DeferWindowPos
>> + *
>> + * Copyright 2015 Rodrigo Rivas Costa
>> + *
>> + * This library is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * This library is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with this library; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
>> + */
>> +
>> +#include "windows.h"
>> +
>> +#include "wine/test.h"
>> +
>> +
>> +#define WIDTH_CHILDREN 50
>> +#define NCHILDREN 3
>> +#define IDX_FAIL 1
>> +
>> +static HINSTANCE g_hInstance;
>
> This is not a DLL, so theoretically no need to use a static variable for that.
> The overhead of calling GetModuleHandle multiple times should be very small.
>
>> +static HWND hChildren[NCHILDREN];
>> +
>> +static void ExpectY(HWND hwnd, int y)
>> +{
>> + RECT r;
>> + POINT p;
>> + GetWindowRect(hwnd, &r);
>> + p.x = r.left;
>> + p.y = r.top;
>> + MapWindowPoints(NULL, GetParent(hwnd), &p, 1);
>> + ok(p.y == y, "Window has not been properly moved: %d != %d\n", (int)p.y, y);
>
> I didn't test it, but there should be no need to explicitly cast here.
>
>> +}
>
> Such a helper function will make it complicated to track down test failures.
> In this case the code doesn't look too long, so you could directly duplicate it into
> the individual test functions. The alternative would be to pass the line number to
> the function, and then use ok_(...) instead. You can take a look at existing tests how
> to do that.
>
>> +
>> +static void TestDeferWindowPos(int posY)
>
> You could split each test function into a separate patch (failing tests marked with
> "todo_wine"), and then send the fix as a forth patch at the end. It is not strictly
> necessary, but it would probably be easier for reviewing.
>
>> +{
>> + int i;
>> + BOOL res;
>> + HDWP hdwp;
>> +
>> + hdwp = BeginDeferWindowPos(1);
>> + ok(hdwp != NULL, "BeginDeferWindowPos failed\n");
>> + for (i = 0; i < NCHILDREN; ++i)
>> + {
>> + HWND h = hChildren[i];
>> + hdwp = DeferWindowPos(hdwp, h, NULL, i*WIDTH_CHILDREN, posY, 0, 0, SWP_NOZORDER|SWP_NOSIZE|SWP_NOACTIVATE);
>> + ok(hdwp != NULL, "DeferWindowPos failed\n");
>> + }
>> + res = EndDeferWindowPos(hdwp);
>> + ok(res, "EndDeferWindowPos failed\n");
>> + for (i = 0; i < NCHILDREN; ++i)
>> + ExpectY(hChildren[i], posY);
>> +}
>> +
>> +static void TestBogusDeferWindowPos(int posY)
>> +{
>> + int i;
>> + BOOL res;
>> + HDWP hdwp;
>> +
>> + hdwp = BeginDeferWindowPos(1);
>> + ok(hdwp != NULL, "BeginDeferWindowPos failed\n");
>> + for (i = 0; i < NCHILDREN; ++i)
>> + {
>> + HDWP hnext;
>> + HWND h = hChildren[i];
>> + if (i==IDX_FAIL)
>
> The style (missing spaces inbetween) is a bit inconsistent here, and also at a couple of places below.
>
>> + h = (HWND)0; //Insert an error. This is not a valid HWND.
>
> C++ Style comments are not allowed in Wine source. Also, you can use NULL instead of casting 0 to a HWND.
>
>> + hnext = DeferWindowPos(hdwp, h, NULL, i*WIDTH_CHILDREN, posY, 0, 0, SWP_NOZORDER|SWP_NOSIZE|SWP_NOACTIVATE);
>> + if (i==IDX_FAIL)
>> + {
>> + DWORD err = GetLastError();
>> + ok(hnext == NULL, "DeferWindowPos accepted a bogus HWND\n");
>> + ok(err == ERROR_INVALID_WINDOW_HANDLE, "Bogus HWND should return ERROR_INVALID_WINDOW_HANDLE, got %d\n", (int)err);
>
> Also no need to explicitly cast here.
>
>> + }
>> + else
>> + {
>> + ok(hnext != NULL, "DeferWindowPos failed\n");
>> + hdwp = hnext;
>> + }
>> + }
>> + res = EndDeferWindowPos(hdwp);
>> + ok(res, "EndDeferWindowPos failed\n");
>> + for (i = 0; i < NCHILDREN; ++i)
>> + {
>> + if (i != IDX_FAIL)
>> + ExpectY(hChildren[i], posY);
>> + }
>> +}
>> +
>> +static void TestDestroyDeferWindowPos(int posY)
>> +{
>> + int i;
>> + BOOL res;
>> + HDWP hdwp;
>> +
>> + hdwp = BeginDeferWindowPos(1);
>> + ok(hdwp != NULL, "BeginDeferWindowPos failed\n");
>> + for (i = 0; i < NCHILDREN; ++i)
>> + {
>> + HWND h = hChildren[i];
>> + hdwp = DeferWindowPos(hdwp, h, NULL, i*WIDTH_CHILDREN, posY, 0, 0, SWP_NOZORDER|SWP_NOSIZE|SWP_NOACTIVATE);
>> + ok(hdwp != NULL, "DeferWindowPos failed\n");
>> + /* Destroying a window after DeferWindowPos but before EndDeferWindowPos should be silently ignored */
>> + if (i == IDX_FAIL)
>> + DestroyWindow(h);
>> + }
>> + res = EndDeferWindowPos(hdwp);
>> + ok(res, "EndDeferWindowPos failed\n");
>> + for (i = 0; i < NCHILDREN; ++i)
>> + {
>> + if (i != IDX_FAIL)
>> + ExpectY(hChildren[i], posY);
>> + }
>> +}
>> +
>> +static LRESULT CALLBACK TestWndProc(HWND hWnd, UINT msg, WPARAM wParam, LPARAM lParam)
>> +{
>> + switch (msg)
>> + {
>> + case WM_CREATE:
>> + {
>> + int i;
>> + int posY = 0;
>> + for (i=0; i < NCHILDREN; ++i)
>> + {
>> + hChildren[i] = CreateWindowExA(0, "Button", NULL, WS_CHILD|WS_VISIBLE,
>> + i*WIDTH_CHILDREN, posY, WIDTH_CHILDREN, WIDTH_CHILDREN,
>> + hWnd, NULL, g_hInstance, NULL);
>> + }
>> +
>> + TestDeferWindowPos(++posY);
>> + TestBogusDeferWindowPos(++posY);
>> + TestDestroyDeferWindowPos(++posY);
>
> If possible you should move those calls outside of the TestWndProc.
>
>> + }
>> + break;
>> + }
>> + return DefWindowProcA(hWnd, msg, wParam, lParam);
>> +}
>> +
>> +START_TEST(defer)
>> +{
>> + WNDCLASSEXA cls;
>> + ATOM acls;
>> +
>> + g_hInstance = GetModuleHandleA(NULL);
>> + memset(&cls, 0, sizeof(cls));
>> + cls.cbSize = sizeof(cls);
>> + cls.lpfnWndProc = TestWndProc;
>> + cls.hInstance = g_hInstance;
>> + cls.lpszClassName = "TestWnd";
>> +
>> + acls = RegisterClassExA(&cls);
>> +
>> + HWND hWnd = CreateWindowExA(0, MAKEINTRESOURCEA(acls), "TestWnd", WS_OVERLAPPEDWINDOW,
>
> You can't declare a variable in the middle of a function (should give you a warning while compiling).
>
>> + CW_USEDEFAULT, CW_USEDEFAULT, CW_USEDEFAULT, CW_USEDEFAULT,
>> + NULL, NULL, g_hInstance, NULL);
>> + ok(hWnd != NULL, "CreateWindowEx failed\n");
>> +}
>> diff --git a/dlls/user32/winpos.c b/dlls/user32/winpos.c
>> index f92a3dc..7cb3a4f 100644
>> --- a/dlls/user32/winpos.c
>> +++ b/dlls/user32/winpos.c
>> @@ -2349,6 +2349,11 @@ HDWP WINAPI DeferWindowPos( HDWP hdwp, HWND hwnd, HWND hwndAfter,
>>
>> hwnd = WIN_GetFullHandle( hwnd );
>> if (is_desktop_window( hwnd )) return 0;
>> + if (!IsWindow( hwnd ))
>> + {
>> + SetLastError( ERROR_INVALID_WINDOW_HANDLE );
>> + return 0;
>> + }
>>
>> if (!(pDWP = get_user_handle_ptr( hdwp, USER_DWP ))) return 0;
>> if (pDWP == OBJ_OTHER_PROCESS)
>> @@ -2418,7 +2423,6 @@ BOOL WINAPI EndDeferWindowPos( HDWP hdwp )
>> {
>> DWP *pDWP;
>> WINDOWPOS *winpos;
>> - BOOL res = TRUE;
>> int i;
>>
>> TRACE("%p\n", hdwp);
>> @@ -2430,20 +2434,20 @@ BOOL WINAPI EndDeferWindowPos( HDWP hdwp )
>> return FALSE;
>> }
>>
>> - for (i = 0, winpos = pDWP->winPos; res && i < pDWP->actualCount; i++, winpos++)
>> + for (i = 0, winpos = pDWP->winPos; i < pDWP->actualCount; i++, winpos++)
>> {
>> TRACE("hwnd %p, after %p, %d,%d (%dx%d), flags %08x\n",
>> winpos->hwnd, winpos->hwndInsertAfter, winpos->x, winpos->y,
>> winpos->cx, winpos->cy, winpos->flags);
>>
>> if (WIN_IsCurrentThread( winpos->hwnd ))
>> - res = USER_SetWindowPos( winpos );
>> + USER_SetWindowPos( winpos );
>> else
>> - res = SendMessageW( winpos->hwnd, WM_WINE_SETWINDOWPOS, 0, (LPARAM)winpos );
>> + SendMessageW( winpos->hwnd, WM_WINE_SETWINDOWPOS, 0, (LPARAM)winpos );
>> }
>> HeapFree( GetProcessHeap(), 0, pDWP->winPos );
>> HeapFree( GetProcessHeap(), 0, pDWP );
>> - return res;
>> + return TRUE;
>> }
>
Thank you all for the review. I will rewrite the patch and send the
try #2 in a couple of days.
More information about the wine-devel
mailing list