February 2016 - Wine-Devel - list.winehq.org

Re: [PATCH v3] winex11.drv: Make the GL context current even if no drawables were given.

by Matteo Bruni

2016-02-29 12:45 GMT+01:00 Henri Verbeet <hverbeet(a)gmail.com>: > On 29 February 2016 at 04:33, Ken Thomases <ken(a)codeweavers.com> wrote: >> So the reasons to revert are: a) we don't have a real test case, and b) leaving it in causes a success-in-todo_wine test failure specific to the Mac? Seem somewhat tenuous reasons to revert a change that implements functionality from the spec. >> > Those don't help, but the more important reason is that as far as we > know this functionality isn't supported on any Windows OpenGL driver, > supposedly because of architectural reasons. Yeah. FWIW I checked on AMD Win10 and that's still unsupported.

Feb. 29, 2016

Re: [PATCH] winex11: Don't create a temporary context each time X11DRV_wglBindTexImageARB is called.

by Matteo Bruni

2016-02-29 3:59 GMT+01:00 Ken Thomases <ken(a)codeweavers.com>: > On Feb 26, 2016, at 5:29 PM, Miklós Máté <mtmkls(a)gmail.com> wrote: > >> @@ -2320,6 +2322,7 @@ static BOOL X11DRV_wglDestroyPbufferARB( struct wgl_pbuffer *object ) >> TRACE("(%p)\n", object); >> >> pglXDestroyPbuffer(gdi_display, object->drawable); >> + pglXDestroyContext(gdi_display, object->tmp_context); > > I think you need to check if object->tmp_context is non-zero before calling glXDestroyContext() to avoid a GLXBadContext error. > > >> + if (!object->tmp_context) { >> + object->tmp_context = pglXCreateNewContext(gdi_display, object->fmt->fbconfig, object->fmt->render_type, prev_context, True); >> + object->prev_context = prev_context; >> + } else if (object->prev_context != prev_context) { >> + pglXDestroyContext(gdi_display, object->tmp_context); >> + object->tmp_context = pglXCreateNewContext(gdi_display, object->fmt->fbconfig, object->fmt->render_type, prev_context, True); >> + object->prev_context = prev_context; >> + } > > Those two branches are mostly the same. This could be reduced to: > > if (!object->tmp_context || object->prev_context != prev_context) { > if (object->tmp_context) > pglXDestroyContext(gdi_display, object->tmp_context); > object->tmp_context = pglXCreateNewContext(gdi_display, object->fmt->fbconfig, object->fmt->render_type, prev_context, True); > object->prev_context = prev_context; > } > > > Is it necessary, when a context is destroyed, to go through all wgl_pbuffer structs looking for any whose prev_context matches the one being destroyed and clear it? Otherwise, you risk having a stale reference that could be reused for a different context and not realize you need to recreate tmp_context. Of course, enumerating all of them in a thread-safe manner is going to require a lot of added infrastructure and complexity. Good point, that's necessary in principle and it shouldn't be too hard to do by keeping a wgl_pbuffers list (similarly to context_list for wgl_context, you should probably reuse the same context_section critical section for synchronization). The other points brought up by Ken are valid too of course.

Feb. 29, 2016

Re: [PATCH 2/3] user32: Added more dialog owner tests.

by Jacek Caban

Hi Dmitry, On 02/29/16 17:40, Dmitry Timoshkov wrote: > Jacek Caban <jacek(a)codeweavers.com> wrote: > >>>> + parent = CreateWindowExA(0, "TestParentClass", "Test parent", >>>> + WS_OVERLAPPEDWINDOW | WS_VISIBLE, >>>> + 100, 100, 200, 200, 0, 0, 0, NULL); >>>> + ok (parent != 0, "Failed to create parent window\n"); >>>> + >>>> + child = CreateWindowExA(0, "TestWindowClass", "Test child", >>>> + WS_OVERLAPPEDWINDOW | WS_VISIBLE, >>>> + 100, 100, 200, 200, 0, 0, 0, NULL); >>>> + ok (child != 0, "Failed to create child window\n"); >>>> + >>>> + child2 = CreateWindowExA(0, "SimpleWindowClass", "Test child2", >>>> + WS_OVERLAPPEDWINDOW | WS_VISIBLE | WS_CHILD, >>>> + 100, 100, 200, 200, child, 0, 0, NULL); >>>> + ok (child2 != 0, "Failed to create child window\n"); >>>> + >>>> + SetParent(child, parent); >>>> + SetFocus(child); >>> Is that really required to use SetParent() instead of specifying correct >>> parent in the CreateWindowEx call? >> Yes, SetParent and CreateWindowEx's parent are not the same thing. See >> for example d13a44e4aa12fa5ad3498c39934e88a7dcd30916 to understand the >> difference. > For child windows it shouldn't matter. It does, otherwise parent would be the owner. >>>> + flush_sequence(); >>>> + DialogBoxA( 0, "TEST_DIALOG", child2, TestModalDlgProc2 ); >>>> + ok_sequence(WmModalDialogSeq_2, "ModalDialog2", TRUE); >>> Is this test supposed to show that actual dialog owner is parent window? >>> If yes, then this theory is not tested at all. >> No, it's actually supposed to show that owner is not the parent. We know >> that owner is child in this case, but current code will use parent >> anyway, which is not right. My tests show that messages are sent to >> child, not parent. > Adding actual checks with GetParent()/GetWindow(GW_OWNER)/GetAncestor() > would be preferrable IMO instead of not obvious message sequences. If I just wanted to test the owner, then yes. But we already have tests for that (the one I pointed you) and this part already works fine. I want to test that messages are sent to the right window, which is what this series fixes. >>>> + SetForegroundWindow(hdlg); >>>> + >>>> + { >>>> + const struct message WmEndDialogSeq[] = { >>>> + { WM_ENABLE, sent }, >>>> + { WM_WINDOWPOSCHANGING, sent|wparam, SWP_HIDEWINDOW|SWP_NOACTIVATE|SWP_NOSIZE|SWP_NOMOVE }, >>>> + { HCBT_ACTIVATE, hook|wparam, (WPARAM)hchild }, >>>> + { WM_NCACTIVATE, sent|wparam|lparam, WA_INACTIVE, (LPARAM)hchild }, >>>> + { WM_ACTIVATE, sent|wparam|lparam, WA_INACTIVE, (LPARAM)hchild }, >>>> + /* FIXME: Following two are optional because Wine sends WM_QUERYNEWPALETTE instead of WM_WINDOWPOSCHANGING */ >>>> + { WM_WINDOWPOSCHANGING, sent|wparam|optional, SWP_NOSIZE|SWP_NOMOVE }, >>>> + { WM_QUERYNEWPALETTE, sent|optional }, >>>> + { WM_NCACTIVATE, sent|wparam|lparam, WA_ACTIVE, (LPARAM)hdlg }, >>>> + { WM_GETTEXT, sent|optional|defwinproc }, >>>> + { WM_ACTIVATE, sent|wparam|lparam, WA_ACTIVE, (LPARAM)hdlg }, >>>> + { HCBT_SETFOCUS, hook|wparam, (WPARAM)hchild }, >>>> + { WM_KILLFOCUS, sent|wparam, (WPARAM)hchild }, >>>> + { WM_SETFOCUS, sent|defwinproc|wparam, (WPARAM)hdlg }, >>>> + { 0 } >>>> + }; >>>> + >>>> + flush_sequence(); >>>> + EndDialog(hdlg, 0); >>>> + ok_sequence(WmEndDialogSeq, "EndDialog", FALSE); >>>> + } >>> Please don't create fake embedded blocks. >> I need this declaration here, otherwise I wouldn't be able to use >> handles for testing lparam value. > wparam/lparam could be tested directly in the window procedure. It would require some global variables and new window procedure. It's cleaner to do it using existing tools IMO. Cheers, Jacek

Feb. 29, 2016

Re: [PATCH 2/3] user32: Added more dialog owner tests.

by Dmitry Timoshkov

Jacek Caban <jacek(a)codeweavers.com> wrote: > >> + parent = CreateWindowExA(0, "TestParentClass", "Test parent", > >> + WS_OVERLAPPEDWINDOW | WS_VISIBLE, > >> + 100, 100, 200, 200, 0, 0, 0, NULL); > >> + ok (parent != 0, "Failed to create parent window\n"); > >> + > >> + child = CreateWindowExA(0, "TestWindowClass", "Test child", > >> + WS_OVERLAPPEDWINDOW | WS_VISIBLE, > >> + 100, 100, 200, 200, 0, 0, 0, NULL); > >> + ok (child != 0, "Failed to create child window\n"); > >> + > >> + child2 = CreateWindowExA(0, "SimpleWindowClass", "Test child2", > >> + WS_OVERLAPPEDWINDOW | WS_VISIBLE | WS_CHILD, > >> + 100, 100, 200, 200, child, 0, 0, NULL); > >> + ok (child2 != 0, "Failed to create child window\n"); > >> + > >> + SetParent(child, parent); > >> + SetFocus(child); > > Is that really required to use SetParent() instead of specifying correct > > parent in the CreateWindowEx call? > > Yes, SetParent and CreateWindowEx's parent are not the same thing. See > for example d13a44e4aa12fa5ad3498c39934e88a7dcd30916 to understand the > difference. For child windows it shouldn't matter. > >> + flush_sequence(); > >> + DialogBoxA( 0, "TEST_DIALOG", child2, TestModalDlgProc2 ); > >> + ok_sequence(WmModalDialogSeq_2, "ModalDialog2", TRUE); > > Is this test supposed to show that actual dialog owner is parent window? > > If yes, then this theory is not tested at all. > > No, it's actually supposed to show that owner is not the parent. We know > that owner is child in this case, but current code will use parent > anyway, which is not right. My tests show that messages are sent to > child, not parent. Adding actual checks with GetParent()/GetWindow(GW_OWNER)/GetAncestor() would be preferrable IMO instead of not obvious message sequences. > >> + SetForegroundWindow(hdlg); > >> + > >> + { > >> + const struct message WmEndDialogSeq[] = { > >> + { WM_ENABLE, sent }, > >> + { WM_WINDOWPOSCHANGING, sent|wparam, SWP_HIDEWINDOW|SWP_NOACTIVATE|SWP_NOSIZE|SWP_NOMOVE }, > >> + { HCBT_ACTIVATE, hook|wparam, (WPARAM)hchild }, > >> + { WM_NCACTIVATE, sent|wparam|lparam, WA_INACTIVE, (LPARAM)hchild }, > >> + { WM_ACTIVATE, sent|wparam|lparam, WA_INACTIVE, (LPARAM)hchild }, > >> + /* FIXME: Following two are optional because Wine sends WM_QUERYNEWPALETTE instead of WM_WINDOWPOSCHANGING */ > >> + { WM_WINDOWPOSCHANGING, sent|wparam|optional, SWP_NOSIZE|SWP_NOMOVE }, > >> + { WM_QUERYNEWPALETTE, sent|optional }, > >> + { WM_NCACTIVATE, sent|wparam|lparam, WA_ACTIVE, (LPARAM)hdlg }, > >> + { WM_GETTEXT, sent|optional|defwinproc }, > >> + { WM_ACTIVATE, sent|wparam|lparam, WA_ACTIVE, (LPARAM)hdlg }, > >> + { HCBT_SETFOCUS, hook|wparam, (WPARAM)hchild }, > >> + { WM_KILLFOCUS, sent|wparam, (WPARAM)hchild }, > >> + { WM_SETFOCUS, sent|defwinproc|wparam, (WPARAM)hdlg }, > >> + { 0 } > >> + }; > >> + > >> + flush_sequence(); > >> + EndDialog(hdlg, 0); > >> + ok_sequence(WmEndDialogSeq, "EndDialog", FALSE); > >> + } > > Please don't create fake embedded blocks. > > I need this declaration here, otherwise I wouldn't be able to use > handles for testing lparam value. wparam/lparam could be tested directly in the window procedure. > > Also it would be really nice to send the tests first so it would be more clear > > when a todo_wine is removed with a subsequent patch in order to see whether > > the patch is actually related to the previously sent tests. > > I'm not sure I agree. Please don't get me wrong, but when sending tests after the fixes the result can't be easily verified, and therefore this reduces the trust and value of patches. -- Dmitry.

Feb. 29, 2016

Re: [PATCH 2/3] user32: Added more dialog owner tests.

by Jacek Caban

Hi Dmitry, On 02/29/16 17:16, Dmitry Timoshkov wrote: > Hi Jacek, > > Jacek Caban <jacek(a)codeweavers.com> wrote: > >> + parent = CreateWindowExA(0, "TestParentClass", "Test parent", >> + WS_OVERLAPPEDWINDOW | WS_VISIBLE, >> + 100, 100, 200, 200, 0, 0, 0, NULL); >> + ok (parent != 0, "Failed to create parent window\n"); >> + >> + child = CreateWindowExA(0, "TestWindowClass", "Test child", >> + WS_OVERLAPPEDWINDOW | WS_VISIBLE, >> + 100, 100, 200, 200, 0, 0, 0, NULL); >> + ok (child != 0, "Failed to create child window\n"); >> + >> + child2 = CreateWindowExA(0, "SimpleWindowClass", "Test child2", >> + WS_OVERLAPPEDWINDOW | WS_VISIBLE | WS_CHILD, >> + 100, 100, 200, 200, child, 0, 0, NULL); >> + ok (child2 != 0, "Failed to create child window\n"); >> + >> + SetParent(child, parent); >> + SetFocus(child); > Is that really required to use SetParent() instead of specifying correct > parent in the CreateWindowEx call? Yes, SetParent and CreateWindowEx's parent are not the same thing. See for example d13a44e4aa12fa5ad3498c39934e88a7dcd30916 to understand the difference. >> + flush_sequence(); >> + DialogBoxA( 0, "TEST_DIALOG", child2, TestModalDlgProc2 ); >> + ok_sequence(WmModalDialogSeq_2, "ModalDialog2", TRUE); > Is this test supposed to show that actual dialog owner is parent window? > If yes, then this theory is not tested at all. No, it's actually supposed to show that owner is not the parent. We know that owner is child in this case, but current code will use parent anyway, which is not right. My tests show that messages are sent to child, not parent. >> + SetForegroundWindow(hdlg); >> + >> + { >> + const struct message WmEndDialogSeq[] = { >> + { WM_ENABLE, sent }, >> + { WM_WINDOWPOSCHANGING, sent|wparam, SWP_HIDEWINDOW|SWP_NOACTIVATE|SWP_NOSIZE|SWP_NOMOVE }, >> + { HCBT_ACTIVATE, hook|wparam, (WPARAM)hchild }, >> + { WM_NCACTIVATE, sent|wparam|lparam, WA_INACTIVE, (LPARAM)hchild }, >> + { WM_ACTIVATE, sent|wparam|lparam, WA_INACTIVE, (LPARAM)hchild }, >> + /* FIXME: Following two are optional because Wine sends WM_QUERYNEWPALETTE instead of WM_WINDOWPOSCHANGING */ >> + { WM_WINDOWPOSCHANGING, sent|wparam|optional, SWP_NOSIZE|SWP_NOMOVE }, >> + { WM_QUERYNEWPALETTE, sent|optional }, >> + { WM_NCACTIVATE, sent|wparam|lparam, WA_ACTIVE, (LPARAM)hdlg }, >> + { WM_GETTEXT, sent|optional|defwinproc }, >> + { WM_ACTIVATE, sent|wparam|lparam, WA_ACTIVE, (LPARAM)hdlg }, >> + { HCBT_SETFOCUS, hook|wparam, (WPARAM)hchild }, >> + { WM_KILLFOCUS, sent|wparam, (WPARAM)hchild }, >> + { WM_SETFOCUS, sent|defwinproc|wparam, (WPARAM)hdlg }, >> + { 0 } >> + }; >> + >> + flush_sequence(); >> + EndDialog(hdlg, 0); >> + ok_sequence(WmEndDialogSeq, "EndDialog", FALSE); >> + } > Please don't create fake embedded blocks. I need this declaration here, otherwise I wouldn't be able to use handles for testing lparam value. > Also it would be really nice to send the tests first so it would be more clear > when a todo_wine is removed with a subsequent patch in order to see whether > the patch is actually related to the previously sent tests. I'm not sure I agree. Thanks for the review, Jacek

Feb. 29, 2016

Re: [PATCH 2/3] user32: Added more dialog owner tests.

by Dmitry Timoshkov

Hi Jacek, Jacek Caban <jacek(a)codeweavers.com> wrote: > + parent = CreateWindowExA(0, "TestParentClass", "Test parent", > + WS_OVERLAPPEDWINDOW | WS_VISIBLE, > + 100, 100, 200, 200, 0, 0, 0, NULL); > + ok (parent != 0, "Failed to create parent window\n"); > + > + child = CreateWindowExA(0, "TestWindowClass", "Test child", > + WS_OVERLAPPEDWINDOW | WS_VISIBLE, > + 100, 100, 200, 200, 0, 0, 0, NULL); > + ok (child != 0, "Failed to create child window\n"); > + > + child2 = CreateWindowExA(0, "SimpleWindowClass", "Test child2", > + WS_OVERLAPPEDWINDOW | WS_VISIBLE | WS_CHILD, > + 100, 100, 200, 200, child, 0, 0, NULL); > + ok (child2 != 0, "Failed to create child window\n"); > + > + SetParent(child, parent); > + SetFocus(child); Is that really required to use SetParent() instead of specifying correct parent in the CreateWindowEx call? > + flush_sequence(); > + DialogBoxA( 0, "TEST_DIALOG", child2, TestModalDlgProc2 ); > + ok_sequence(WmModalDialogSeq_2, "ModalDialog2", TRUE); Is this test supposed to show that actual dialog owner is parent window? If yes, then this theory is not tested at all. > + SetForegroundWindow(hdlg); > + > + { > + const struct message WmEndDialogSeq[] = { > + { WM_ENABLE, sent }, > + { WM_WINDOWPOSCHANGING, sent|wparam, SWP_HIDEWINDOW|SWP_NOACTIVATE|SWP_NOSIZE|SWP_NOMOVE }, > + { HCBT_ACTIVATE, hook|wparam, (WPARAM)hchild }, > + { WM_NCACTIVATE, sent|wparam|lparam, WA_INACTIVE, (LPARAM)hchild }, > + { WM_ACTIVATE, sent|wparam|lparam, WA_INACTIVE, (LPARAM)hchild }, > + /* FIXME: Following two are optional because Wine sends WM_QUERYNEWPALETTE instead of WM_WINDOWPOSCHANGING */ > + { WM_WINDOWPOSCHANGING, sent|wparam|optional, SWP_NOSIZE|SWP_NOMOVE }, > + { WM_QUERYNEWPALETTE, sent|optional }, > + { WM_NCACTIVATE, sent|wparam|lparam, WA_ACTIVE, (LPARAM)hdlg }, > + { WM_GETTEXT, sent|optional|defwinproc }, > + { WM_ACTIVATE, sent|wparam|lparam, WA_ACTIVE, (LPARAM)hdlg }, > + { HCBT_SETFOCUS, hook|wparam, (WPARAM)hchild }, > + { WM_KILLFOCUS, sent|wparam, (WPARAM)hchild }, > + { WM_SETFOCUS, sent|defwinproc|wparam, (WPARAM)hdlg }, > + { 0 } > + }; > + > + flush_sequence(); > + EndDialog(hdlg, 0); > + ok_sequence(WmEndDialogSeq, "EndDialog", FALSE); > + } Please don't create fake embedded blocks. Also it would be really nice to send the tests first so it would be more clear when a todo_wine is removed with a subsequent patch in order to see whether the patch is actually related to the previously sent tests. -- Dmitry.

Feb. 29, 2016

Re: ole32: Do not open registry keys with no access rights.

by Sebastian Lackner

On 29.02.2016 16:49, Thomas Faber wrote: > While simply opening a subkey (as done here) does not require any > access rights in particular, NtOpenKey with 0 desired access always > fails with STATUS_ACCESS_DENIED on Windows. > Consequently specifying a nonzero access is a prerequisite for some day > fixing Wine's NtOpenKey behavior in this regard. It would be nice to have tests for that in the wine source tree (marked with todo_wine, until all places which rely on it have been fixed).

Feb. 29, 2016

Re: [PATCH 2/3] user32: Added more dialog owner tests.

by Marvin

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=20971 Your paranoid android. === w7u (32 bit msg) === msg.c:8926: Test failed: did not get expected count for minimum timeout (51 != ~100). === w7pro64 (32 bit msg) === msg.c:15473: Test failed: wrong status 00400040 === w1064 (32 bit msg) === msg.c:6417: Test failed: Update region shouldn't be empty msg.c:6651: Test failed: InvalidateErase: 0: the msg sequence is not complete: expected 0085 - actual 0000 msg.c:6653: Test failed: Paint: 0: the msg sequence is not complete: expected 000f - actual 0000 msg.c:6423: Test failed: Regions are different msg.c:6423: Test failed: Regions are different msg.c:6423: Test failed: Regions are different

Feb. 29, 2016

Re: [PATCH 1/2] include: add authz.h

by Alexandre Julliard

Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> writes: > +typedef struct _AUTHZ_ACCESS_REPLY > +{ > + DWORD ResultListLength; > + DWORD *SaclEvaluationResults; > + DWORD *Error; > +} AUTHZ_ACCESS_REPLY, *PAUTHZ_ACCESS_REPLY; There's a field missing here. > +BOOL WINAPI AuthzInitializeResourceManager(DWORD flags, void *access_checker, > + void *compute_dyn_groups, void *free_dyn_groups, > + const WCHAR *managername, void *handle ); That's not the correct prototype. -- Alexandre Julliard julliard(a)winehq.org

Feb. 29, 2016

Re: [v4 PATCH] comctl32/tooltip: Protect TTM_ADDTOOLW from invalid text pointers

by Alexandre Julliard

Alistair Leslie-Hughes <leslie_alistair(a)hotmail.com> writes: > @@ -1041,6 +1041,9 @@ TOOLTIPS_AddToolT (TOOLTIPS_INFO *infoPtr, const TTTOOLINFOW *ti, BOOL isW) > if (ti->cbSize >= TTTOOLINFOW_V2_SIZE && !ti->lpszText && isW) > return FALSE; > > + if(isW && ti->lpszText && IsBadReadPtr(ti->lpszText, sizeof(WCHAR))) > + return FALSE; You want IsBadStringPtrW, and this should most likely be merged with the check above it one way or another. -- Alexandre Julliard julliard(a)winehq.org

Feb. 29, 2016