June 2022 - Wine-Devel - list.winehq.org

[PATCH 3/5] user32/tests: Remove SetForegroundWindow success checks.

by Rémi Bernon

From: Rémi Bernon <rbernon(a)codeweavers.com> It's not supposed to fail and skipping the tests simply creates false successful runs. Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/user32/tests/win.c | 21 +++------------------ 1 file changed, 3 insertions(+), 18 deletions(-) diff --git a/dlls/user32/tests/win.c b/dlls/user32/tests/win.c index c6667cafeff..e5b50b55482 100644 --- a/dlls/user32/tests/win.c +++ b/dlls/user32/tests/win.c @@ -3491,12 +3491,7 @@ static void test_popup_zorder(HWND hwnd_D, HWND hwnd_E, DWORD style) HWND hwnd_A, hwnd_B, hwnd_C, hwnd_F; /* Give current thread foreground state otherwise the tests may fail. */ - if (!SetForegroundWindow(hwnd_D)) - { - skip("SetForegroundWindow not working\n"); - return; - } - + SetForegroundWindow(hwnd_D); SetWindowPos(hwnd_E, hwnd_D, 0,0,0,0, SWP_NOSIZE|SWP_NOMOVE|SWP_NOACTIVATE); check_z_order(hwnd_D, hwnd_E, 0, 0, FALSE); @@ -3954,12 +3949,7 @@ static void test_SetForegroundWindow(HWND hwnd) ShowWindow(hwnd, SW_SHOW); check_wnd_state(hwnd, hwnd, hwnd, 0); - ret = SetForegroundWindow(hwnd); - if (!ret) - { - skip( "SetForegroundWindow not working\n" ); - return; - } + SetForegroundWindow(hwnd); check_wnd_state(hwnd, hwnd, hwnd, 0); SetLastError(0xdeadbeef); @@ -11606,12 +11596,7 @@ static void test_topmost(void) ok(owner != 0, "Failed to create owner window (%ld)\n", GetLastError()); /* Give current thread foreground state otherwise the tests may fail. */ - if (!SetForegroundWindow(owner)) - { - DestroyWindow(owner); - skip("SetForegroundWindow not working\n"); - return; - } + SetForegroundWindow(owner); hwnd = create_tool_window(WS_CAPTION | WS_SYSMENU | WS_MINIMIZEBOX | WS_MAXIMIZEBOX | WS_POPUP | WS_VISIBLE, owner); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/169

June 1, 2022

[PATCH 2/5] user32/tests: Remove unnecessary test skipping checks.

by Rémi Bernon

From: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/user32/tests/win.c | 24 +++--------------------- 1 file changed, 3 insertions(+), 21 deletions(-) diff --git a/dlls/user32/tests/win.c b/dlls/user32/tests/win.c index 3727efb1b41..c6667cafeff 100644 --- a/dlls/user32/tests/win.c +++ b/dlls/user32/tests/win.c @@ -1453,10 +1453,6 @@ static void test_nonclient_area(HWND hwnd) GetWindowRect(hwnd, &rc_window); GetClientRect(hwnd, &rc_client); - /* avoid some cases when things go wrong */ - if (IsRectEmpty(&rc_window) || IsRectEmpty(&rc_client) || - rc_window.right > 32768 || rc_window.bottom > 32768) return; - rc = rc_client; MapWindowPoints(hwnd, 0, (LPPOINT)&rc, 2); FixedAdjustWindowRectEx(&rc, style, menu, exstyle); @@ -4314,7 +4310,7 @@ static void test_capture_4(void) WS_OVERLAPPEDWINDOW, CW_USEDEFAULT, 0, 400, 200, NULL, NULL, hInstance, NULL); ok(hwnd != NULL, "CreateWindowEx failed with error %ld\n", GetLastError()); - if (!hwnd) return; + hmenu = CreatePopupMenu(); ret = AppendMenuA( hmenu, MF_STRING, 1, "winetest2"); @@ -4379,11 +4375,8 @@ static void test_keyboard_input(HWND hwnd) ok(GetFocus() == hwnd, "wrong focus window %p\n", GetFocus()); keybd_event(VK_SPACE, 0, 0, 0); - if (!peek_message(&msg)) - { - skip( "keybd_event didn't work, skipping keyboard test\n" ); - return; - } + ret = peek_message(&msg); + ok( ret, "message %04x available\n", msg.message); ok(msg.hwnd == hwnd && msg.message == WM_KEYDOWN, "hwnd %p message %04x\n", msg.hwnd, msg.message); ret = peek_message(&msg); ok( !ret, "message %04x available\n", msg.message); @@ -11289,12 +11282,6 @@ static void test_LockWindowUpdate(HWND parent) {parent, parent, 1, 1} }; - if (!child) - { - skip("CreateWindow failed, skipping LockWindowUpdate tests\n"); - return; - } - ShowWindow(parent, SW_SHOW); UpdateWindow(parent); flush_events(TRUE); @@ -12181,11 +12168,6 @@ static void test_destroy_quit(void) "destroy test main", WS_OVERLAPPED | WS_CAPTION, 100, 100, 100, 100, 0, 0, GetModuleHandleA(NULL), NULL); ok(destroy_data.main_wnd != NULL, "CreateWindowEx failed with error %ld\n", GetLastError()); - if (!destroy_data.main_wnd) - { - CloseHandle(destroy_data.evt); - return; - } thread1 = CreateThread(NULL, 0, destroy_thread1, 0, 0, NULL); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/169

June 1, 2022

[PATCH 1/5] user32/tests: Fix indentation to silence a warning.

by Rémi Bernon

From: Rémi Bernon <rbernon(a)codeweavers.com> Signed-off-by: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/user32/tests/msg.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/dlls/user32/tests/msg.c b/dlls/user32/tests/msg.c index 2705914d5e5..235724a5ff9 100644 --- a/dlls/user32/tests/msg.c +++ b/dlls/user32/tests/msg.c @@ -14089,9 +14089,11 @@ static void test_ShowWindow(void) ok(wp.ptMaxPosition.x == sw[i].wp_max.x && wp.ptMaxPosition.y == sw[i].wp_max.y, "expected %ld,%ld got %ld,%ld\n", sw[i].wp_max.x, sw[i].wp_max.y, wp.ptMaxPosition.x, wp.ptMaxPosition.y); -if (0) /* FIXME: Wine behaves completely different here */ - ok(EqualRect(&win_rc, &wp.rcNormalPosition), "expected %s got %s\n", - wine_dbgstr_rect(&win_rc), wine_dbgstr_rect(&wp.rcNormalPosition)); + if (0) /* FIXME: Wine behaves completely different here */ + { + ok(EqualRect(&win_rc, &wp.rcNormalPosition), "expected %s got %s\n", + wine_dbgstr_rect(&win_rc), wine_dbgstr_rect(&wp.rcNormalPosition)); + } } DestroyWindow(hwnd); flush_events(); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/169

June 1, 2022

[PATCH 0/5] MR169: user32/tests: Cleanup some msg.c tests.

by Rémi Bernon

-- https://gitlab.winehq.org/wine/wine/-/merge_requests/169

June 1, 2022

Re: [PATCH v2 0/5] MR162: explorer: Move clipboard manager thread from display driver.

by Martin Storsjö

This (in particular, the “explorer: …” commit) caused a regression for me. In an entirely headless configuration (configured with `--without-x`) on arm32 (probably happens on other architectures too, but that’s where it happened for me), when bootstrapping a new wineprefix (setting `WINEPREFIX` to a new, nonexistent directory, running `wine wineboot`), it can hang, printing errors like these: ``` 0048:err:sync:RtlpWaitForCriticalSection section 7BC8A3F0 "../src/dlls/ntdll /loader.c: loader_section" wait timed out in thread 0048, blocked by 0054, r etrying (60 sec) 0048:err:sync:RtlpWaitForCriticalSection section 7BC8A3F0 "../src/dlls/ntdll /loader.c: loader_section" wait timed out in thread 0048, blocked by 0054, r etrying (60 sec) ``` Does this trigger some race condition when bootstrapping the environment? (I can report it in bugzilla later, I presume that’s still preferred, I just happened to have better access to gitlab.) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/162#note_1460

June 1, 2022

Re: RFC: Allow marking unreliable tests as flaky.

by Rémi Bernon

On 6/1/22 15:26, Francois Gouget wrote: > > Wine has many unreliable tests which makes it hard to know whether a > patch causes new failures. So one proposal was to mark unreliable tests > as flaky, and have a mode where flaky failures can ignored by setting > $WINETEST_ALLOW_FLAKY. > > So one would be able to apply a patch, run 'WINETEST_ALLOW_FLAKY=1 make > test' and if that fails that means the patch introduces a new failure. > > The way it works is that when a flaky test fails the message is 'Flaky > failed...' instead of 'Test failed...'. I also added the count of flaky > failures to the summary line. So the flaky failures are not totally > swept under the rug. The main difference is that if > $WINETEST_ALLOW_FLAKY is set, the exit code is 0 if the only failures > are flaky ones. > > > That still leaves some open questions though: > > * Should the message be 'Test failed' instead of 'Flaky failed' when > $WINETEST_ALLOW_FLAKY is not set? I opted for the latter because it > adds information that the failed test has been marked as flaky. > Imho the message should keep the 'Test failed' / 'Test succeeded' prefixes. For instance I think it would look more consistent with something like: Test succeeded inside todo block: Test succeeded inside flaky block: Test marked todo: Test marked flaky: Test failed: Test succeeded: > > * On a related note the patch below has flaky_windows and flaky_wine > macros. Is that too fine grained? Maybe we don't want to run the risk > of marking a test as flaky_windows only to discover later that it can > also fail in Wine? But conversely, wouldn't it be valuable to know > that a test is only flaky in Wine and not in Windows? > > Note: flaky_windows is a bit long but I was worried that flaky_win > would be too similar to flaky_wine. > > * I decided having flaky_{windows,wine}_if() macros would be overkill. I think flaky_if should be enough, and then there could be some shortcut global variables for `!strcmp(winetest_platform, "windows" / "wine")`, so it's not too long to write. Cheers, -- Rémi Bernon <rbernon(a)codeweavers.com>

June 1, 2022

Re: [PATCH v2 2/3] jscript: Don't execute any functions if script state does not allow it.

by Gabriel Ivăncescu

On 01/06/2022 03:07, Jacek Caban wrote: > On 5/31/22 16:56, Gabriel Ivăncescu wrote: >> Instead of only interpreted functions. Property retrievals or setters are >> allowed though, as long as they are not accessors. >> >> Signed-off-by: Gabriel Ivăncescu<gabrielopcode(a)gmail.com> >> --- >> >> Note that native GetDispID seem to fail when retrieving builtin/prototype >> functions (?) if script is uninitialized, but retrieving the DISPID >> before and >> using it after it's uninitialized still works, though (that's what the >> tests >> do). I don't think it's worth replicating this GetDispID quirk for the >> moment. > > > I'm not really concerned about GetDispID quirks, but implication of your > choice for the next patch. I think that releasing objects sooner is > better, so it would be good to have a better argument for moving that. > Your reasoning there is based on details of how you decided to not > follow native here. > > > More generally, creating function objects in response for property get > is an implementation detail that supposed to be transparent. If it's > causing problems elsewhere, I'd first look at making it more > transparent. In this case, maybe we should create function objects > sooner, in response to GetDispID. This would both match all tests and > allow us to release function constructor sooner. > > > Jacek > Right, but how is releasing such objects not an implementation detail as well? The thing is, there's a very rare crash in FFXIV Launcher when logging in that happens when some proxy prototype tries to obtain the object prototype, which for some reason is NULL (suggesting uninitialized script). I haven't been able to investigate it in depth since I wasn't able to reproduce it since then. But it would be neatly solved by this as well without adding any complexity. What's such a big deal about releasing them later? It's not like scripts stay that long in the uninitialized state, nor does it complicate the code, and it's an implementation detail anyway. If the function object was created earlier, it would still use the same function object, so the end result is exactly the same. On the other hand, obtaining the function object on dispid seems like too much work for no real gain. On top of work, that would also lose all the current optimizations with builtins (which are only retrieved now on PROPERTYGET, which is pretty rare—most of the time they're just called directly, while looking up their DISPIDs happens all the time). I'm definitely not certain that's a good idea.

June 1, 2022

Re: [PATCH v2 4/6] d3dx10_43: Add D3DX10CreateAsyncTextureInfoProcessor stub.

by Matteo Bruni

On Thu, May 26, 2022 at 6:33 PM Piotr Caban <piotr(a)codeweavers.com> wrote: > > Signed-off-by: Piotr Caban <piotr(a)codeweavers.com> > --- > dlls/d3dx10_43/async.c | 6 ++++++ > dlls/d3dx10_43/d3dx10_43.spec | 2 +- > include/d3dx10.h | 2 +- > include/d3dx10async.h | 2 ++ > 4 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/dlls/d3dx10_43/async.c b/dlls/d3dx10_43/async.c > index ebef97a7e12..d636127475d 100644 > --- a/dlls/d3dx10_43/async.c > +++ b/dlls/d3dx10_43/async.c > @@ -456,6 +456,12 @@ HRESULT WINAPI D3DX10CreateAsyncResourceLoaderW(HMODULE module, const WCHAR *res > return S_OK; > } > > +HRESULT WINAPI D3DX10CreateAsyncTextureInfoProcessor(D3DX10_IMAGE_INFO *info, ID3DX10DataProcessor **processor) > +{ > + FIXME("info %p, processor %p stub!\n", info, processor); > + return E_NOTIMPL; > +} Just for my own curiosity: have you already thought about how to split the functions and the source files between those that are to be shared vs the version specific ones?

June 1, 2022

Re: [PATCH v2 6/6] d3dx10_43/tests: Add D3DX10CreateAsyncTextureInfoProcessor tests.

by Matteo Bruni

On Thu, May 26, 2022 at 6:33 PM Piotr Caban <piotr(a)codeweavers.com> wrote: > > Signed-off-by: Piotr Caban <piotr(a)codeweavers.com> > --- > dlls/d3dx10_43/tests/d3dx10.c | 43 +++++++++++++++++++++++++++++++++++ > 1 file changed, 43 insertions(+) > > + hr = ID3DX10DataProcessor_Process(dp, (void*)test_image[i].data, test_image[i].size); Another nitpick, space between void and *.

June 1, 2022

Re: [PATCH v2 3/6] d3dx10_43: Share code for resource data loading.

by Matteo Bruni

On Thu, May 26, 2022 at 6:32 PM Piotr Caban <piotr(a)codeweavers.com> wrote: > > Signed-off-by: Piotr Caban <piotr(a)codeweavers.com> > --- > dlls/d3dx10_43/async.c | 93 ++++++++++++++++++++++++++++---------- > dlls/d3dx10_43/compiler.c | 34 ++------------ > dlls/d3dx10_43/dxhelpers.h | 4 ++ > dlls/d3dx10_43/texture.c | 70 ++++------------------------ > 4 files changed, 89 insertions(+), 112 deletions(-) > Just a nitpick: > - if (!(rsrc = FindResourceA(module, resource, (const char *)RT_RCDATA))) > - rsrc = FindResourceA(module, resource, (const char *)RT_BITMAP); > - if (!rsrc) > + hr = load_resource_initA(module, resource, &rsrc); > + if (FAILED(hr)) We usually tend to put the call inside the condition in "d3d" code. I don't personally mind too much though.

June 1, 2022