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

Re: [PATCH 3/8] d3dx9: Reference top level parameter instead of dirty flag.
by Matteo Bruni 21 Apr '17

21 Apr '17

2017-04-20 13:26 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > -static BOOL param_set_dirty_flag_ptr(void *dirty_flag_ptr, struct d3dx_parameter *param) > +static BOOL param_set_top_level_param(void *top_level_param, struct d3dx_parameter *param) > { > - param->dirty_flag_ptr = (DWORD *)dirty_flag_ptr; > + param->top_level_param = (struct d3dx_parameter *)top_level_param; > return FALSE; > } The cast is unnecessary (yeah, it was already unnecessary, I missed that earlier). It might make sense to store both? Avoiding the extra indirection in is_param_dirty() might help a tiny bit. It's fine to tackle this later on.

2 2

Re: [PATCH 8/8] d3dx9: Use local dirty flags for shared parameters.
by Matteo Bruni 21 Apr '17

21 Apr '17

2017-04-20 13:26 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: As I somewhat hinted at in the previous patch, this one should probably be merged with 7/8. It did help to keep them separated for the first review pass though :) > diff --git a/dlls/d3dx9_36/d3dx9_private.h b/dlls/d3dx9_36/d3dx9_private.h > index d6d4280..f9cbf7c 100644 > --- a/dlls/d3dx9_36/d3dx9_private.h > +++ b/dlls/d3dx9_36/d3dx9_private.h > @@ -222,15 +222,14 @@ struct d3dx_parameter > { > struct d3dx_parameter *referenced_param; > struct d3dx_parameter *shared_param; > + struct d3dx_parameter **shared_param_list; This name is a bit confusing given that it is an array of (pointers to) matching parameters from different effects and shared_param is a pointer to the shared pool parameter. It becomes less of an issue if the new field moves to the separate shared parameter structure I mentioned. Maybe just call it "parameters"? > @@ -3193,7 +3196,16 @@ static void d3dx_pool_sync_shared_parameter(struct ID3DXEffectPoolImpl *pool, st > param_set_data_pointer(param, (unsigned char *)pool->parameters[i]->data, FALSE, TRUE); > } > new_refcount = InterlockedIncrement(&pool->parameters[i]->shared_refcount); > + if (new_refcount == 1) > + pool->parameters[i]->shared_param_list = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, > + sizeof(*pool->parameters[i]->shared_param_list)); > + else > + pool->parameters[i]->shared_param_list = HeapReAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, > + pool->parameters[i]->shared_param_list, > + sizeof(*pool->parameters[i]->shared_param_list) * new_refcount); It's probably better to make an initial allocation for > 1 elements and then expand by doubling the size, as usual with this kind of stuff.

1 0

Re: [PATCH 6/8] d3dx9: Factor out free_parameter_data() function.
by Matteo Bruni 21 Apr '17

21 Apr '17

2017-04-20 13:26 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > +static void free_parameter(struct d3dx_parameter *param, BOOL element, BOOL child) > +{ > + unsigned int i; > + > + TRACE("Free parameter %p, name %s, type %s, child %s\n", param, param->name, > + debug_d3dxparameter_type(param->type), child ? "yes" : "no"); Again, given that it's a chance for some cleanup, please add the period at the end of the message. Also I'd change the "child" part, either just using %#x or child ? "child" : "" (or something else, the point being that the current "child yes" vs "child no" looks pretty awkward). Maybe you can also trace the "element" parameter. > + if (param->param_eval) > + { > + d3dx_free_param_eval(param->param_eval); > + param->param_eval = NULL; Is setting the field to NULL necessary? Same for the others below.

1 0

Re: [PATCH 5/8] d3dx9: Reference effect pool implementation in d3dx9_base_effect structure.
by Matteo Bruni 21 Apr '17

21 Apr '17

2017-04-20 13:26 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > +struct ID3DXEffectPoolImpl > +{ > + ID3DXEffectPool ID3DXEffectPool_iface; > + LONG ref; > +}; Please rename it to "d3dx_effect_pool" or similar, while you're at it. Same for the interface methods (d3dx_effect_pool_QueryInterface(), ...) We want to get rid of the MS-like names for our implementation and this seems like a good chance to clean this one up. > - TRACE("base %p, data %p, data_size %lu, effect %p\n", base, data, data_size, effect); > + TRACE("base %p, data %p, data_size %lu, effect %p, pool %p\n", base, data, data_size, effect, pool); Since you're touching it, please add a period at the end of the message.

1 0

Re: [PATCH 2/8] d3dx9/tests: Add more tests for IsParameterUsed() function.
by Matteo Bruni 21 Apr '17

21 Apr '17

2017-04-20 13:26 GMT+02:00 Paul Gofman <gofmanp(a)gmail.com>: > + todo_wine > + ok(effect3->lpVtbl->IsParameterUsed(effect3, "vs_arr2", "tech0"), > + "Unexpected IsParameterUsed result FALSE.\n"); > + todo_wine > + ok(!effect3->lpVtbl->IsParameterUsed(effect3, "vs_arr2[0]", "tech0"), > + "Unexpected IsParameterUsed result FALSE.\n"); Typo. I'd just leave out the TRUE / FALSE from these messages, one has to look up the code anyway if there is a test failure.

1 0

Re: [2/2] [v2] secur32: Add support for SECPKG_ATTR_KEY_INFO.
by Jacek Caban 21 Apr '17

21 Apr '17

Hi Akihiro, The patch looks mostly good now, thanks. I have a few minor comments. On 20.04.2017 13:58, Akihiro Sagawa wrote: > +struct schan_alg_id_name { > + ALG_ID alg_id; > + const char* name; > + const WCHAR *nameW; > +}; Using an array here like const WCHAR nameW[8]; would make later initialization nicer by not requiring multiple string variables. > + > +static int comp_alg_id(const void *a, const void *b) > +{ > + const struct schan_alg_id_name *lhs = a; > + const struct schan_alg_id_name *rhs = b; > + return (int)lhs->alg_id - (int)rhs->alg_id; > +} > + > +static void* get_alg_name(ALG_ID id, BOOL wide) > +{ > + static const WCHAR ecdsaW[] = {'E','C','D','S','A',0}; > + static const WCHAR rsaW[] = {'R','S','A',0}; > + static const WCHAR desW[] = {'D','E','S',0}; > + static const WCHAR rc2W[] = {'R','C','2',0}; > + static const WCHAR triple_desW[] = {'3','D','E','S',0}; > + static const WCHAR aesW[] = {'A','E','S',0}; > + static const WCHAR rc4W[] = {'R','C','4',0}; > + static const struct schan_alg_id_name alg_name_map[] = { > + { CALG_ECDSA, "ECDSA", ecdsaW }, > + { CALG_RSA_SIGN, "RSA", rsaW }, > + { CALG_DES, "DES", desW }, > + { CALG_RC2, "RC2", rc2W }, > + { CALG_3DES, "3DES", triple_desW }, > + { CALG_AES_128, "AES", aesW }, > + { CALG_AES_192, "AES", aesW }, > + { CALG_AES_256, "AES", aesW }, > + { CALG_RC4, "RC4", rc4W }, > + }; > + const struct schan_alg_id_name *res; > + struct schan_alg_id_name key; > + > + key.alg_id = id; > + res = bsearch(&key, alg_name_map, > + sizeof(alg_name_map)/sizeof(alg_name_map[0]), sizeof(alg_name_map[0]), > + comp_alg_id); You could use id directly as a key. You don't need it to be of schan_alg_id_name* type, the key may be just ALG_ID. Thanks, Jacek

1 0

Re: [PATCH] tests: Don't evaluate the todo condition when not running on Wine.
by Sebastian Lackner 21 Apr '17

21 Apr '17

On 21.04.2017 15:01, Huw Davies wrote: > Signed-off-by: Huw Davies <huw(a)codeweavers.com> > --- > This cropped up because I wanted to write a todo condition involving > IsProcessDPIAware(), since this doesn't exist in XP, its address is > loaded dynamically. I could have written the condition as > pIsProcessDPIAware && pIsProcessDPIAware(), but since I know the > function is always present in Wine, that's unnecessary. Perhaps > more to the point, I didn't expect the current behaviour. > > include/wine/test.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/wine/test.h b/include/wine/test.h > index af602c0fac..fa3f90c932 100644 > --- a/include/wine/test.h > +++ b/include/wine/test.h > @@ -123,7 +123,7 @@ extern void __winetest_cdecl winetest_trace( const char *msg, ... ) WINETEST_PRI > winetest_loop_todo(); \ > winetest_end_todo()) > #define todo_wine todo_if(!strcmp(winetest_platform, "wine")) > -#define todo_wine_if(is_todo) todo_if((is_todo) && !strcmp(winetest_platform, "wine")) > +#define todo_wine_if(is_todo) todo_if(!strcmp(winetest_platform, "wine") && (is_todo)) > > > #ifdef NONAMELESSUNION > I don't mind if its changed, but please note that it was intentional to check "is_todo" first. The idea was that side effects (like assigning variables, changing last error, ...) are applied on both Wine and Windows. Just think about something like: FirstCall(); todo_wine_if(SecondCall()) ok(GetLastError() == ..., "\n"); With your patch applied, it will test two entirely different things on Windows (error from FirstCall) and Wine (error from SecondCall), which would also be confusing. Best regards, Sebastian

3 3

Re: [PATCH] msvcrt: Handle synchronous flag for x64 C++ exceptions
by Piotr Caban 21 Apr '17

21 Apr '17

Hi, On 04/20/17 16:42, Daniel Lehman wrote: > + if (descr->magic >= CXX_FRAME_MAGIC_VC8 && > + (descr->flags & FUNC_DESCR_SYNCHRONOUS) && > + (rec->ExceptionCode != CXX_EXCEPTION && rec->ExceptionCode != STATUS_UNWIND_CONSOLIDATE)) > + return ExceptionContinueSearch; /* handle only c++ exceptions */ I think the condition for STATUS_UNWIND_CONSOLIDATE should also check number of parameters and ExceptionInformation[0]==(ULONG_PTR)call_catch_block like it's done during unwinding. Also the code breaks unwinding for longjump when FUNC_DESCR_SYNCHRONOUS flag is set. Thanks, Piotr

1 0

Re: [PATCH 08/12] d3d10core/tests: Allow passing any interface to get_refcount().
by Henri Verbeet 21 Apr '17

21 Apr '17

On 21 April 2017 at 10:59, Józef Kucia <jkucia(a)codeweavers.com> wrote: > -static ULONG get_refcount(IUnknown *iface) > +#define get_refcount(iface) get_refcount_((IUnknown *)iface) > +static ULONG get_refcount_(IUnknown *iface) You don't really need that macro.

1 0

Linux desktop integration: Fill StartupWMClass in .desktop menu entries
by Eike Hein 21 Apr '17

21 Apr '17

Hi, I maintain the taskbar in Plasma 5 and would like to poke folks about a desktop integration pain point we (and our friends at other DEs) face with apps run via wine. Here's the relevant ticket: https://bugs.winehq.org/show_bug.cgi?id=32699 Let me give some broader context on this. Modern DEs have a swath of UX going on that relies on being able to identify the application owning a particular window in the windowing system. "Identify the application" in practice means "map the window to a .desktop file installed in the menu file system". We want to do this because: a) Many a taskbar/dock-like UI tries to form a smooth life cycle across "launcher button", "startup notification" and "window button(s)". For each of those we need to work out the app they are associated with to be able to match them up. If it fails, ugly things like co-existing task buttons or missing startup throbbers and similar happen. b) UI these days tends to prefer to load assets such as app icons via the values defined in .desktop files instead of using the window icon, to ensure consistent results (apps may load icon assets differently from the shell, leading to asset quality issues, inconsistent theming, the icon changing between the lifecycle states mentioned in 'a', etc.) Plus the Wayland windowing system we're all migrating towards currently doesn'ㅅ have window icons. c) DEs do things like track app starts/quits for things like "Most used apps" launcher lists. To match a window to a .desktop file we rely on window and process metadata. On X11, there's the WM_CLASS window property: https://tronche.com/gui/x/icccm/sec-4.html#WM_CLASS Ideally, the second part of a X11 window's WM_CLASS matches the name of a .desktop file installed on the system. Failing that, it matches the value of StartupWMClass in that .desktop file (this is the key part I'll get back to soon). Here's some more documentation by our friends at Gnome on this standard scheme: https://wiki.gnome.org/Projects/GnomeShell/ApplicationBased (Look for WM_CLASS / StartupWMClass.) In practice, we pull many other tricks to cope with broken meta data. We try to match things by names, we grab process command lines from /proc and check them against Exec keys in .desktop files in complicated ways, ... it's a multi-page heuristic, and other DEs (Unity via bamf, Gnome Shell via iirc ShellWindowTracker) have equivalent code. Something Gnome does that we don't do (because I think it's brittle) is to store the name of the .desktop file an app was launched with as a private window hint on the next window that comes up (which then doesn't work when the app was launched from CLI instead of shell UI). None of these things currently allow us to handle wine very well. As of now (perhaps thanks to the patch mentioned in the linked ticket), wine sets WM_CLASS to the executable name of the app, e.g. "Notepad.exe". But the .desktop files it generates don't match this by name, and they don't contain a StartupWMClass key. The process command line is the Windows-like path to the .exe, which we also can't easily match up to the complicated Exec values in wine's .desktop files. (Aside: On Wayland, there's a more modern version of this: Windows have an appid property, which should be set to a .desktop file name. I don't know what the status of Wayland support in wine is -- currently the wine clients we encounter run over Xwayland, so we're back to WM_CLASS. We hope wine will set this property correctly when the time comes, or we're happy to work with you guys to find a way to support wine.) Long story short ... could this be picked up? The goal must be to have either .desktop file names or their StartupWMClass values match whatever is set as WM_CLASS. Currently, the easiest fix would be to set StartupWMClass to the executable name set as WM_CLASS. I assume the .desktop file generator has access to this, as the .lnk file referred to by Exec should presumably contain the name of the executable to be launched. Cheers, Eike

1 0