February 2017 - Wine-devel - list.winehq.org

Proposal: winetest 'context'
by Mark Jansen 26 Feb '17

26 Feb '17

Hello, Often when testing something, helper functions are introduced that are called with different data (for example when iterating over tables). Attached patch (winetest_set_context.diff) is a proposal that allows tests to specify a 'context', that will be printed in the failure message. This way, the actual test code does not have to prefix every case with a trace, or include the data in every test macro. The patch is written so that tests do not have to use this mechanism, so there is no need to convert all tests at once. Second attachment (winetest_ctx_example.diff) shows how an existing test could be converted to use this new technique. Regards, Mark Jansen

3 2

Re: [PATCH] rasapi32: Ensure device count matches data returned
by Sebastian Lackner 26 Feb '17

26 Feb '17

On 26.02.2017 20:41, Bruno Jesus wrote: > From: Bruno Jesus <bjesus(a)codeweavers.com> > > Based on original patch by Qian Hong. > > Fixes bug https://bugs.winehq.org/show_bug.cgi?id=30378 > > Signed-off-by: Bruno Jesus <bjesus(a)codeweavers.com> > --- > dlls/rasapi32/rasapi.c | 1 + > dlls/rasapi32/tests/rasapi.c | 24 ++++++++++++++++++++++++ > 2 files changed, 25 insertions(+) > > diff --git a/dlls/rasapi32/rasapi.c b/dlls/rasapi32/rasapi.c > index fcd0cb8..f97d4c1 100644 > --- a/dlls/rasapi32/rasapi.c > +++ b/dlls/rasapi32/rasapi.c > @@ -254,6 +254,7 @@ DWORD WINAPI RasEnumDevicesA(LPRASDEVINFOA lpRasDevinfo, LPDWORD lpcb, LPDWORD l > if(lpRasDevinfo && lpRasDevinfo->dwSize != sizeof(RASDEVINFOA)) > return ERROR_INVALID_SIZE; > > + *lpcDevices = 1; > if (!lpRasDevinfo || (*lpcb < sizeof(RASDEVINFOA))) { > *lpcb = sizeof(RASDEVINFOA); > return ERROR_BUFFER_TOO_SMALL; > diff --git a/dlls/rasapi32/tests/rasapi.c b/dlls/rasapi32/tests/rasapi.c > index e0ff25f..e982f6c 100644 > --- a/dlls/rasapi32/tests/rasapi.c > +++ b/dlls/rasapi32/tests/rasapi.c > @@ -77,80 +77,104 @@ static void test_rasenum(void) > > /* test first parameter */ > cb = bufsize; > + cDevices = 0xdead; > result = pRasEnumDevicesA(NULL, &cb, &cDevices); > ok(result == ERROR_BUFFER_TOO_SMALL || > result == ERROR_INVALID_USER_BUFFER, /* win98 */ > "Expected ERROR_BUFFER_TOO_SMALL, got %08d\n", result); > + ok(cDevices > 0 && cDevices != 0xdead, "Unexpected device count %d\n", cDevices); > > rasDevInfo[0].dwSize = 0; > cb = bufsize; > + cDevices = 0xdead; > result = pRasEnumDevicesA(rasDevInfo, &cb, &cDevices); > ok(result == ERROR_INVALID_SIZE || > result == ERROR_INVALID_USER_BUFFER, /* win98 */ > "Expected ERROR_INVALID_SIZE, got %08d\n", result); > + ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices); > > rasDevInfo[0].dwSize = sizeof(RASDEVINFOA) -1; > cb = bufsize; > + cDevices = 0xdead; > result = pRasEnumDevicesA(rasDevInfo, &cb, &cDevices); > ok(result == ERROR_INVALID_SIZE || > result == ERROR_INVALID_USER_BUFFER, /* win98 */ > "Expected ERROR_INVALID_SIZE, got %08d\n", result); > + ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices); > > rasDevInfo[0].dwSize = sizeof(RASDEVINFOA) +1; > cb = bufsize; > + cDevices = 0xdead; > result = pRasEnumDevicesA(rasDevInfo, &cb, &cDevices); > ok(result == ERROR_INVALID_SIZE || > result == ERROR_INVALID_USER_BUFFER, /* win98 */ > "Expected ERROR_INVALID_SIZE, got %08d\n", result); > + ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices); > > /* test second parameter */ > rasDevInfo[0].dwSize = sizeof(RASDEVINFOA); > result = pRasEnumDevicesA(rasDevInfo, NULL, &cDevices); > + cDevices = 0xdead; Whats the point to set this after the function call? And also, how is this better than Qian Hongs original patch? Using %u is more correct for unsigned values, and the empty line after the assignment of *lpcDevices also didn't hurt. > ok(result == ERROR_INVALID_PARAMETER, > "Expected ERROR_INVALID_PARAMETER, got %08d\n", result); > + ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices); > > rasDevInfo[0].dwSize = sizeof(RASDEVINFOA); > cb = 0; > result = pRasEnumDevicesA(rasDevInfo, &cb, &cDevices); > + cDevices = 0xdead; > ok(result == ERROR_BUFFER_TOO_SMALL || > result == ERROR_INVALID_SIZE, /* vista, 2k8 */ > "Expected ERROR_BUFFER_TOO_SMALL/ERROR_INVALID_SIZE, got %08d\n", result); > + ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices); > > rasDevInfo[0].dwSize = sizeof(RASDEVINFOA); > cb = bufsize -1; > result = pRasEnumDevicesA(rasDevInfo, &cb, &cDevices); > + cDevices = 0xdead; > ok(result == ERROR_BUFFER_TOO_SMALL, > "Expected ERROR_BUFFER_TOO_SMALL, got %08d\n", result); > + ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices); > > rasDevInfo[0].dwSize = sizeof(RASDEVINFOA); > cb = bufsize +1; > + cDevices = 0xdead; > result = pRasEnumDevicesA(rasDevInfo, &cb, &cDevices); > ok(result == ERROR_SUCCESS, > "Expected ERROR_SUCCESS, got %08d\n", result); > + ok(cDevices > 0 && cDevices != 0xdead, "Unexpected device count %d\n", cDevices); > > /* test third parameter */ > rasDevInfo[0].dwSize = sizeof(RASDEVINFOA); > cb = bufsize; > + cDevices = 0xdead; > result = pRasEnumDevicesA(rasDevInfo, &cb, NULL); > ok(result == ERROR_INVALID_PARAMETER, > "Expected ERROR_INVALID_PARAMETER, got %08d\n", result); > + ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices); > > /* test combinations of invalid parameters */ > + cDevices = 0xdead; > result = pRasEnumDevicesA(NULL, NULL, &cDevices); > ok(result == ERROR_INVALID_PARAMETER, > "Expected ERROR_INVALID_PARAMETER, got %08d\n", result); > + ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices); > > + cDevices = 0xdead; > result = pRasEnumDevicesA(NULL, &cb, NULL); > ok(result == ERROR_INVALID_PARAMETER || > result == ERROR_INVALID_USER_BUFFER, /* win98 */ > "Expected ERROR_INVALID_PARAMETER, got %08d\n", result); > + ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices); > > cb = 0; > rasDevInfo[0].dwSize = 0; > + cDevices = 0xdead; > result = pRasEnumDevicesA(rasDevInfo, &cb, &cDevices); > ok(result == ERROR_INVALID_SIZE || > broken(result == ERROR_BUFFER_TOO_SMALL), /* win98 */ > "Expected ERROR_INVALID_SIZE, got %08d\n", result); > + ok(cDevices == 0xdead, "Unexpected device count %d\n", cDevices); > > HeapFree(GetProcessHeap(), 0, rasDevInfo); > } > -- 2.9.3 >

2 1

Re: include: Avoid overflow (warning) in RGBA_MAKE.
by Dmitry Timoshkov 26 Feb '17

26 Feb '17

Gerald Pfeifer <gerald(a)pfeifer.com> wrote: > --- a/include/d3dtypes.h > +++ b/include/d3dtypes.h > @@ -53,7 +53,7 @@ typedef LONG D3DFIXED; > #define RGBA_GETRED(rgb) (((rgb) >> 16) & 0xff) > #define RGBA_GETGREEN(rgb) (((rgb) >> 8) & 0xff) > #define RGBA_GETBLUE(rgb) ((rgb) & 0xff) > -#define RGBA_MAKE(r, g, b, a) ((D3DCOLOR) (((a) << 24) | ((r) << 16) | ((g) << 8) | (b))) > +#define RGBA_MAKE(r, g, b, a) ((D3DCOLOR) (((unsigned)(a) << 24) | ((r) << 16) | ((g) << 8) | (b))) This is a public header, probably you need to fix the callers of this macro instead. -- Dmitry.

3 4

Re: [PATCH 03/18] comctl32: Added basic implementation for task dialogs and add tests
by Fabian Maurer 26 Feb '17

26 Feb '17

Thanks for the reply, > If this will stay, please use functions, not macros. STR_SIZE does not > look useful. Well, I figured it makes the code a bit easier. Should I should just write it out everywhere or make it a function? > Usually there's no need to allocate 'struct list' itself. You're right, I'll rewrite that. > You can't free list node like that. What do you mean? I allocated the list and it's contents with HeapAlloc, so I need to free it that way. Or am I missing something? > This one is already defined in a header. Thanks, I'll use the headers instead of adding another define for that. > Static string for that doesn't look right. Why is that? I've looked into the other code, and strings are often defined as static const WCHAR, that was the recommended method, too. Of course that needs to be changed later, since we (probably) want to take localization into account. Regards, Fabian Maurer

3 4

Re: [PATCH 04/18] comctl32: Extend TaskDialog, add simple callback and tests
by Nikolay Sivov 26 Feb '17

26 Feb '17

On 25.02.2017 19:01, Fabian Maurer wrote: > +/* FIXME: Make thread safe? */ > +static const TASKDIALOGCONFIG *task_config = 0; > +static HRESULT callback(HWND hwnd, UINT uNotification, WPARAM wParam, LPARAM lParam) > +{ > + if(task_config->pfCallback) > + return task_config->pfCallback(hwnd, uNotification, wParam, lParam, task_config->lpCallbackData); > + return S_OK; > +} > + > static INT_PTR CALLBACK DialogProc(HWND hwndDlg, UINT uMsg, WPARAM wParam, LPARAM lParam) > { > + HRESULT ret_callback; > + > switch (uMsg) > { > + case WM_INITDIALOG: > + callback(hwndDlg, TDN_DIALOG_CONSTRUCTED, 0, 0); > + callback(hwndDlg, TDN_CREATED, 0, 0); > + return TRUE; How is this going to work with global variable? > +/* For message checking we use a simplified version from the code used for user32 messages */ > + > +#define TDN_NO_MORE_MESSAGES 0xffffffff > + > +typedef struct > +{ > + UINT msg; > + WPARAM wparam; > + LPARAM lparam; > + > + struct list entry; > +}message_data; > + > +static struct list messages = LIST_INIT(messages); > + > +/* Message lists to test against */ > + > +static const message_data mes_simple_show[] = { > + { TDN_DIALOG_CONSTRUCTED, 0, 0 }, > + { TDN_CREATED, 0, 0 }, > + { TDN_BUTTON_CLICKED, IDOK, 0 }, > + { TDN_DESTROYED, 0, 0 }, > + { TDN_NO_MORE_MESSAGES } > +}; > + > +/* Functions handling message processing */ > + > +static void message_add(UINT msg, WPARAM wparam, LPARAM lparam) > +{ > + message_data *data = HeapAlloc(GetProcessHeap(), 0, sizeof(message_data)); > + > + data->msg = msg; > + data->wparam = wparam; > + data->lparam = lparam; > + > + list_add_tail(&messages, &data->entry); > +} > + > +#define ok_sequence(exp ) \ > + ok_sequence_((exp), __FILE__, __LINE__) > + > +static void ok_sequence_(const message_data *expected_list, const char *file, int line) > +{ > + message_data *message; > + BOOL dump = FALSE; > + UINT count; > + For that please use msg.h from comctl32/tests.

2 3

Re: [PATCH 06/18] comctl32: TaskDialog - Implement main and content text controls
by Nikolay Sivov 26 Feb '17

26 Feb '17

On 25.02.2017 19:01, Fabian Maurer wrote: > + PostMessageW(GetDlgItem(hwndDlg, ID_TEXTMAIN), WM_SETFONT, (WPARAM)font_main, TRUE); > + PostMessageW(GetDlgItem(hwndDlg, ID_TEXTCONTENT), WM_SETFONT, (WPARAM)font_default, TRUE); Why Post*? Dialog handling in user32 has an ability to do this for you. > + case WM_CTLCOLORSTATIC: > + if((HWND)lParam == GetDlgItem(hwndDlg, ID_TEXTMAIN)) > + { > + HDC hdc = (HDC) wParam; > + > + SetTextColor(hdc, RGB(50,50,220)); > + SetBkColor(hdc, GetSysColor(COLOR_3DFACE)); > + > + return (INT_PTR)GetSysColorBrush(COLOR_3DFACE); > + } > + break; Where does this come from? > + font_default = CreateFontW (16, 0, 0, 0, FW_DONTCARE, FALSE, FALSE, FALSE, > + DEFAULT_CHARSET, 0, 0, CLEARTYPE_QUALITY, FF_DONTCARE, font_name); > + font_main = CreateFontW (19, 0, 0, 0, FW_DONTCARE, FALSE, FALSE, FALSE, > + DEFAULT_CHARSET, 0, 0, CLEARTYPE_QUALITY, FF_DONTCARE, font_name); > + Why Tahoma is hardcoded? If you're using dialog code this should be handled automatically using template data. > + if(!IS_INTRESOURCE(pTaskConfig->pszMainInstruction) && !STR_EMPTY(pTaskConfig->pszMainInstruction)) > + { > + RECT rect; > + > + SelectObject(dc_dummy, font_main); > + rect = text_get_rect(dc_dummy, pTaskConfig->pszMainInstruction, dialog_width); > + > + controls_add(controls, ID_TEXTMAIN, class_static, pTaskConfig->pszMainInstruction, > + WS_CHILD | WS_VISIBLE, 5, dialog_height + 5, rect.right, rect.bottom); > + > + dialog_height += rect.bottom + 10; > + SelectObject(dc_dummy, font_default); > + } > + > + if(!IS_INTRESOURCE(pTaskConfig->pszContent) && !STR_EMPTY(pTaskConfig->pszContent)) > + { > + RECT rect = text_get_rect(dc_dummy, pTaskConfig->pszContent, dialog_width); > + > + controls_add(controls, ID_TEXTCONTENT, class_static, pTaskConfig->pszContent, > + WS_CHILD | WS_VISIBLE, 5, dialog_height + 5, rect.right, rect.bottom); > + > + dialog_height += rect.bottom + 10; > + } > + > + controls_add(controls, IDOK, class_button, text_ok, > + WS_CHILD | WS_VISIBLE | BS_DEFPUSHBUTTON, dialog_width - 40 - 10, dialog_height + 5, 40, 10); > + dialog_height += 10 + 10; There's a lot of magic offsets here that should be grouped together and probably have some defined names too.

2 3

Re: [PATCH v2 4/6] ddraw: Keep a ddraw object reference in d3d7 vertex buffers.
by Henri Verbeet 26 Feb '17

26 Feb '17

On 24 February 2017 at 19:37, Matteo Bruni <mbruni(a)codeweavers.com> wrote: > @@ -4257,6 +4257,7 @@ static HRESULT WINAPI d3d7_CreateVertexBuffer(IDirect3D7 *iface, D3DVERTEXBUFFER > { > TRACE("Created vertex buffer %p.\n", object); > *vertex_buffer = &object->IDirect3DVertexBuffer7_iface; > + IDirectDraw7_AddRef(&ddraw->IDirectDraw7_iface); > } It seems more appropriate to do this in d3d_vertex_buffer_create() when storing the ddraw pointer.

1 0

Re: [PATCH 14/18] comctl32: TaskDialog - Changed tests to make them more reliable
by Marvin 25 Feb '17

25 Feb '17

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=28901 Your paranoid android. === w7pro64 (64 bit taskdialog) === 0824:taskdialog: unhandled exception c0000005 at 00000000004620FE

2 1

Re: [09/18] comctl32: TaskDialog - make thread-safe
by Nikolay Sivov 25 Feb '17

25 Feb '17

On 24.02.2017 23:04, Fabian Maurer wrote: > Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de> > --- > dlls/comctl32/taskdialog.c | 137 ++++++++++++++++++++++++++++++++++----------- > 1 file changed, 105 insertions(+), 32 deletions(-) > > diff --git a/dlls/comctl32/taskdialog.c b/dlls/comctl32/taskdialog.c > index 3d6b99000c..63848303f5 100644 > --- a/dlls/comctl32/taskdialog.c > +++ b/dlls/comctl32/taskdialog.c > @@ -69,6 +69,26 @@ typedef struct > UINT y; > }button_info; > > +typedef struct > +{ > + struct list entry; > + const TASKDIALOGCONFIG *task_config; > + HFONT font_default; > + HFONT font_main; > + HWND hwnd; > +}taskdialog_info; > + > +struct list taskdialogs = LIST_INIT(taskdialogs); > + > +static CRITICAL_SECTION taskdialog_section; > +static CRITICAL_SECTION_DEBUG taskdialog_section_debug = > +{ > + 0, 0, &taskdialog_section, > + { &taskdialog_section_debug.ProcessLocksList, &taskdialog_section_debug.ProcessLocksList }, > + 0, 0, { (DWORD_PTR)(__FILE__ ": taskdialog_section") } > +}; > +static CRITICAL_SECTION taskdialog_section = { &taskdialog_section_debug, -1, 0, 0, 0, 0 }; > + Why would you need something like that? What is unsafe about regular dialog window?

2 5

Re: [PATCH 18/18] comctl32: TaskDialog - Add ability to cancel dialog
by Marvin 25 Feb '17

25 Feb '17

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at https://testbot.winehq.org/JobDetails.pl?Key=28906 Your paranoid android. === w8 (32 bit taskdialog) === 02f0:taskdialog: unhandled exception c0000005 at 00472B00 === w7pro64 (64 bit taskdialog) === 0828:taskdialog: unhandled exception c0000005 at 000000000046211E === w864 (64 bit taskdialog) === 0fb0:taskdialog: unhandled exception c0000005 at 000000000046211E

1 0