Re: [PATCH] user32: Add support for class versions
On 05.11.2016 16:36, Ivan Akulinchev wrote:
Signed-off-by: Ivan Akulinchev <ivan.akulinchev(a)gmail.com> --- dlls/comctl32/comctl32.manifest | 1 - dlls/comctl32/theming.c | 15 +++++++-- dlls/user32/class.c | 73 +++++++++++++++++++++++++++++++++++++---- dlls/user32/user_private.h | 2 ++ dlls/user32/win.c | 7 ++-- 5 files changed, 86 insertions(+), 12 deletions(-)
Hi, thanks for looking into this.
diff --git a/dlls/comctl32/comctl32.manifest b/dlls/comctl32/comctl32.manifest index 4c86137..5aa55a3 100644 --- a/dlls/comctl32/comctl32.manifest +++ b/dlls/comctl32/comctl32.manifest @@ -12,7 +12,6 @@ <windowClass>NativeFontCtl</windowClass> <windowClass>ReBarWindow32</windowClass> <windowClass>ScrollBar</windowClass> - <windowClass>Static</windowClass> <windowClass>SysAnimate32</windowClass> <windowClass>SysDateTimePick32</windowClass> <windowClass>SysHeader32</windowClass>
Something tells me we'll need opposite logic here, if versioned classes support are to be added one by one.
diff --git a/dlls/comctl32/theming.c b/dlls/comctl32/theming.c index 93d6fe6..b78ecc9 100644 --- a/dlls/comctl32/theming.c +++ b/dlls/comctl32/theming.c @@ -28,6 +28,7 @@ #include "comctl32.h" #include "uxtheme.h" #include "wine/debug.h" +#include "wine/unicode.h"
WINE_DEFAULT_DEBUG_CHANNEL(theming);
@@ -124,8 +125,6 @@ void THEMING_Initialize (void) static const WCHAR refDataPropName[] = { 'C','C','3','2','T','h','e','m','i','n','g','D','a','t','a',0 };
- if (!IsThemeActive()) return; - atSubclassProp = GlobalAddAtomW (subclassPropName); atRefDataProp = GlobalAddAtomW (refDataPropName);
@@ -142,6 +141,18 @@ void THEMING_Initialize (void) } originalProcs[i] = class.lpfnWndProc; class.lpfnWndProc = subclassProcs[i]; + + /* + * FIXME: The Dialog class ('#32770') should NOT be overridden here. + * + * Temporarily ignore this issue using the hack below... + */ + if (strcmpiW(subclasses[i].className, dialogClass) != 0) + { + /* If not a dialog (see above), make the class global */ + class.style |= CS_GLOBALCLASS; + class.hInstance = NULL; + }
if (!class.lpfnWndProc) {
All theming.c should go when transition is complete, because once you're able to create 6.0.0.0!Static it will support theme drawing API regardless if theming is enabled or not.
diff --git a/dlls/user32/class.c b/dlls/user32/class.c index 0b3582e..5b16008 100644 --- a/dlls/user32/class.c +++ b/dlls/user32/class.c @@ -411,6 +411,42 @@ static CLASS *CLASS_RegisterClass( LPCWSTR name, HINSTANCE hInstance, BOOL local return classPtr; }
+/*********************************************************************** + * CLASS_GetVersionedName + * + * Return a versioned class name, like "6.0.2600.2982!Button". + */ +LPCWSTR CLASS_GetVersionedName( LPCWSTR name ) +{ + ACTCTX_SECTION_KEYED_DATA data = { sizeof(data) }; + + static const WCHAR staticW[] = { 'S','t','a','t','i','c',0 }; + + if (!name) + return NULL; + + if (IS_INTRESOURCE( name )) + return name; + + /* + * FIXME: I disabled the Static class in comctl32.manifest (because it is + * not implemented there yet), however I still get "6.0.2600.2982!Static". + * + * Is it a FindActCtxSectionString bug? + */ + if (strcmpiW( name, staticW ) == 0) + return name;
This needs separate investigation if it's truly happens without Static present in manifest.
+ + if (FindActCtxSectionStringW( 0, NULL, 3, name, &data )) + { + BYTE *res = (BYTE *)data.lpData; + ULONG offset = *(ULONG *)(res + sizeof(ULONG) * 2 + sizeof(DWORD)); + return (LPCWSTR)(res + offset); + } + + return name;
This makes sense, but please use types from ntdll/actctx.c (just duplicate what you need in user32, same as it's done in ole32 and kernel32 already). Also 3 here stands for ACTIVATION_CONTEXT_SECTION_WINDOW_CLASS_REDIRECTION.
+} +
/*********************************************************************** * register_builtin @@ -576,10 +612,12 @@ ATOM WINAPI RegisterClassExA( const WNDCLASSEXA* wc )
if (!IS_INTRESOURCE(wc->lpszClassName)) { + LPCWSTR versioned_name; WCHAR name[MAX_ATOM_LEN + 1];
if (!MultiByteToWideChar( CP_ACP, 0, wc->lpszClassName, -1, name, MAX_ATOM_LEN + 1 )) return 0; - classPtr = CLASS_RegisterClass( name, instance, !(wc->style & CS_GLOBALCLASS), + versioned_name = CLASS_GetVersionedName( name ); + classPtr = CLASS_RegisterClass( versioned_name, instance, !(wc->style & CS_GLOBALCLASS), wc->style, wc->cbClsExtra, wc->cbWndExtra ); } Why do you need this in -A call too?
else @@ -619,6 +657,7 @@ ATOM WINAPI RegisterClassExW( const WNDCLASSEXW* wc ) ATOM atom; CLASS *classPtr; HINSTANCE instance; + LPCWSTR versioned_name;
GetDesktopWindow(); /* create the desktop window to trigger builtin class registration */
@@ -630,7 +669,9 @@ ATOM WINAPI RegisterClassExW( const WNDCLASSEXW* wc ) } if (!(instance = wc->hInstance)) instance = GetModuleHandleW( NULL );
- if (!(classPtr = CLASS_RegisterClass( wc->lpszClassName, instance, !(wc->style & CS_GLOBALCLASS), + versioned_name = CLASS_GetVersionedName( wc->lpszClassName ); + + if (!(classPtr = CLASS_RegisterClass( versioned_name, instance, !(wc->style & CS_GLOBALCLASS), wc->style, wc->cbClsExtra, wc->cbWndExtra ))) return 0;
This part I looks wrong. Why would it register something that is not what user asked for? My understanding is that comctl32 itself should register its versioned classes (and in ideal v5 vs v6 separation only v6 module should be doing that). So comctl32 is loaded, it registers regular classes first, like Listview, then it registered versioned names like 6.0.0.0!Listview. What RegisterClassExW should probably check is if target class in current context is already registered, so if application tries to register ClassName, and context has redirection set as 6.0.0.0!ClassName, registration should fail. I think it should be possible to add a test for that: - create temporary context with window class redirection section; - manually register corresponding versioned class name; - activate context; - see what happens if you try to register non-version class. I vaguely remember I tried something like that on Windows years ago, and as expected it breaks this whole redirection scheme, because if 6.x.x.x!Button is already registered by the time context is activated comctl32 won't be able to register it again.
@@ -678,6 +719,7 @@ BOOL WINAPI UnregisterClassA( LPCSTR className, HINSTANCE hInstance ) BOOL WINAPI UnregisterClassW( LPCWSTR className, HINSTANCE hInstance ) { CLASS *classPtr = NULL; + LPCWSTR versioned_name = CLASS_GetVersionedName( className );
GetDesktopWindow(); /* create the desktop window to trigger builtin class registration */
@@ -685,7 +727,7 @@ BOOL WINAPI UnregisterClassW( LPCWSTR className, HINSTANCE hInstance ) { req->instance = wine_server_client_ptr( hInstance ); if (!(req->atom = get_int_atom_value(className)) && className) - wine_server_add_data( req, className, strlenW(className) * sizeof(WCHAR) ); + wine_server_add_data( req, versioned_name, strlenW(versioned_name) * sizeof(WCHAR) ); if (!wine_server_call_err( req )) classPtr = wine_server_get_ptr( reply->client_ptr ); } SERVER_END_REQ; @@ -1192,10 +1234,15 @@ BOOL WINAPI GetClassInfoExA( HINSTANCE hInstance, LPCSTR name, WNDCLASSEXA *wc )
if (!IS_INTRESOURCE(name)) { + LPCWSTR versioned_name; WCHAR nameW[MAX_ATOM_LEN + 1]; if (!MultiByteToWideChar( CP_ACP, 0, name, -1, nameW, sizeof(nameW)/sizeof(WCHAR) )) return FALSE; - classPtr = CLASS_FindClass( nameW, hInstance ); + versioned_name = CLASS_GetVersionedName( nameW ); + classPtr = CLASS_FindClass( versioned_name, hInstance ); + /* FIXME: I believe we shouldn't fall back to the normal class. Fill + * free to remove it if necessary. */ + if (!classPtr) classPtr = CLASS_FindClass( nameW, hInstance ); } else classPtr = CLASS_FindClass( (LPCWSTR)name, hInstance );
@@ -1230,6 +1277,7 @@ BOOL WINAPI GetClassInfoExW( HINSTANCE hInstance, LPCWSTR name, WNDCLASSEXW *wc { ATOM atom; CLASS *classPtr; + LPCWSTR versioned_name;
TRACE("%p %s %p\n", hInstance, debugstr_w(name), wc);
@@ -1241,10 +1289,21 @@ BOOL WINAPI GetClassInfoExW( HINSTANCE hInstance, LPCWSTR name, WNDCLASSEXW *wc
if (!hInstance) hInstance = user32_module;
- if (!(classPtr = CLASS_FindClass( name, hInstance ))) + versioned_name = CLASS_GetVersionedName( name ); + + classPtr = CLASS_FindClass( versioned_name, hInstance ); + if (!classPtr) { - SetLastError( ERROR_CLASS_DOES_NOT_EXIST ); - return FALSE; + /* + * FIXME: It's not correct to fall back to the normal class, but our + * comctl32 implementation depends on it... + */ + classPtr = CLASS_FindClass( name, hInstance ); + if (!classPtr) + { + SetLastError( ERROR_CLASS_DOES_NOT_EXIST ); + return FALSE; + } } wc->style = classPtr->style; wc->lpfnWndProc = WINPROC_GetProc( classPtr->winproc, TRUE );
GetClassInfo part looks okay. It shouldn't break anything even if we had it today, except that it adds bogus context lookup that is a waste of time.
diff --git a/dlls/user32/user_private.h b/dlls/user32/user_private.h index 0b5b2ac..0c212ce 100644 --- a/dlls/user32/user_private.h +++ b/dlls/user32/user_private.h @@ -281,6 +281,8 @@ extern void SPY_EnterMessage( INT iFlag, HWND hwnd, UINT msg, WPARAM wParam, LPA extern void SPY_ExitMessage( INT iFlag, HWND hwnd, UINT msg, LRESULT lReturn, WPARAM wParam, LPARAM lParam ) DECLSPEC_HIDDEN;
+extern LPCWSTR CLASS_GetVersionedName( LPCWSTR name ) DECLSPEC_HIDDEN; + #include "pshpack1.h"
typedef struct diff --git a/dlls/user32/win.c b/dlls/user32/win.c index a7be4a3..42676b5 100644 --- a/dlls/user32/win.c +++ b/dlls/user32/win.c @@ -1740,9 +1740,11 @@ HWND WINAPI DECLSPEC_HOTPATCH CreateWindowExA( DWORD exStyle, LPCSTR className, if (!IS_INTRESOURCE(className)) { WCHAR bufferW[256]; + LPCWSTR versioned_name; if (!MultiByteToWideChar( CP_ACP, 0, className, -1, bufferW, sizeof(bufferW)/sizeof(WCHAR) )) return 0; - return wow_handlers.create_window( (CREATESTRUCTW *)&cs, bufferW, instance, FALSE ); + versioned_name = CLASS_GetVersionedName( bufferW ); + return wow_handlers.create_window( (CREATESTRUCTW *)&cs, versioned_name, instance, FALSE ); } /* Note: we rely on the fact that CREATESTRUCTA and */ /* CREATESTRUCTW have the same layout. */ @@ -1760,6 +1762,7 @@ HWND WINAPI DECLSPEC_HOTPATCH CreateWindowExW( DWORD exStyle, LPCWSTR className, HINSTANCE instance, LPVOID data ) { CREATESTRUCTW cs; + LPCWSTR versioned_name = CLASS_GetVersionedName( className );
cs.lpCreateParams = data; cs.hInstance = instance; @@ -1774,7 +1777,7 @@ HWND WINAPI DECLSPEC_HOTPATCH CreateWindowExW( DWORD exStyle, LPCWSTR className, cs.lpszClass = className; cs.dwExStyle = exStyle;
- return wow_handlers.create_window( &cs, className, instance, TRUE ); + return wow_handlers.create_window( &cs, versioned_name, instance, TRUE ); }
Looks fine, but it could go to WIN_CreateWindowEx(). There's one potentially very important thing about all this, according to docs every winproc call is guaranteed to happen under correct context. So basically if you send a message while particular context was activated, if this window belongs to different thread docs imply that context is temporary activated around winproc call on that receiving thread. I was unable to make this happen, maybe a fresh pair of eyes will be helpful.
Sorry, forgot to CC wine-devel(a)winehq.org... ------------- Hi, thanks for a reply! On Saturday, 5 November 2016 17:56:47 CET you wrote:
On 05.11.2016 16:36, Ivan Akulinchev wrote:
Signed-off-by: Ivan Akulinchev <ivan.akulinchev(a)gmail.com> ---
dlls/comctl32/comctl32.manifest | 1 - dlls/comctl32/theming.c | 15 +++++++-- dlls/user32/class.c | 73 +++++++++++++++++++++++++++++++++++++---- dlls/user32/user_private.h | 2 ++ dlls/user32/win.c | 7 ++-- 5 files changed, 86 insertions(+), 12 deletions(-)
Hi, thanks for looking into this.
diff --git a/dlls/comctl32/comctl32.manifest b/dlls/comctl32/comctl32.manifest index 4c86137..5aa55a3 100644 --- a/dlls/comctl32/comctl32.manifest +++ b/dlls/comctl32/comctl32.manifest @@ -12,7 +12,6 @@
<windowClass>NativeFontCtl</windowClass> <windowClass>ReBarWindow32</windowClass> <windowClass>ScrollBar</windowClass>
- <windowClass>Static</windowClass>
<windowClass>SysAnimate32</windowClass> <windowClass>SysDateTimePick32</windowClass> <windowClass>SysHeader32</windowClass>
Something tells me we'll need opposite logic here, if versioned classes support are to be added one by one.
This line ("<windowClass>Static</windowClass>") tells Windows to use the "6.0.0.0!Static" class instead of "Static". However this class does not exist and all applications crash. If you remove this line, the standard Static class from user32 is used.
diff --git a/dlls/comctl32/theming.c b/dlls/comctl32/theming.c index 93d6fe6..b78ecc9 100644 --- a/dlls/comctl32/theming.c +++ b/dlls/comctl32/theming.c @@ -28,6 +28,7 @@
#include "comctl32.h" #include "uxtheme.h" #include "wine/debug.h"
+#include "wine/unicode.h"
WINE_DEFAULT_DEBUG_CHANNEL(theming);
@@ -124,8 +125,6 @@ void THEMING_Initialize (void)
static const WCHAR refDataPropName[] =
{ 'C','C','3','2','T','h','e','m','i','n','g','D','a','t','a',0 };
- if (!IsThemeActive()) return; -
atSubclassProp = GlobalAddAtomW (subclassPropName); atRefDataProp = GlobalAddAtomW (refDataPropName);
@@ -142,6 +141,18 @@ void THEMING_Initialize (void)
} originalProcs[i] = class.lpfnWndProc; class.lpfnWndProc = subclassProcs[i];
+ + /* + * FIXME: The Dialog class ('#32770') should NOT be overridden here. + * + * Temporarily ignore this issue using the hack below... + */ + if (strcmpiW(subclasses[i].className, dialogClass) != 0) + { + /* If not a dialog (see above), make the class global */ + class.style |= CS_GLOBALCLASS; + class.hInstance = NULL; + }
if (!class.lpfnWndProc) {
All theming.c should go when transition is complete, because once you're able to create 6.0.0.0!Static it will support theme drawing API regardless if theming is enabled or not.
The problem here is that it is impossible to create a versioned class for #32770, and therefore we can not create a global class - only a local one. I believe this stuff is a part of the undocumented Theme Hooks API, and not a part of comctl32... theming.c is needed because nobody wants to copy & paste the corresponding code from user32, as Microsoft did. Maybe we can create a private library containing the base classes, used by both libraries?
diff --git a/dlls/user32/class.c b/dlls/user32/class.c index 0b3582e..5b16008 100644 --- a/dlls/user32/class.c +++ b/dlls/user32/class.c @@ -411,6 +411,42 @@ static CLASS *CLASS_RegisterClass( LPCWSTR name, HINSTANCE hInstance, BOOL local> return classPtr;
}
+/*********************************************************************** + * CLASS_GetVersionedName + * + * Return a versioned class name, like "6.0.2600.2982!Button". + */ +LPCWSTR CLASS_GetVersionedName( LPCWSTR name ) +{ + ACTCTX_SECTION_KEYED_DATA data = { sizeof(data) }; + + static const WCHAR staticW[] = { 'S','t','a','t','i','c',0 }; + + if (!name) + return NULL; + + if (IS_INTRESOURCE( name )) + return name; + + /* + * FIXME: I disabled the Static class in comctl32.manifest (because it is + * not implemented there yet), however I still get "6.0.2600.2982!Static". + * + * Is it a FindActCtxSectionString bug? + */ + if (strcmpiW( name, staticW ) == 0) + return name;
This needs separate investigation if it's truly happens without Static present in manifest.
Yes, I was very surprised by this behaviour.
+ + if (FindActCtxSectionStringW( 0, NULL, 3, name, &data )) + { + BYTE *res = (BYTE *)data.lpData; + ULONG offset = *(ULONG *)(res + sizeof(ULONG) * 2 + sizeof(DWORD)); + return (LPCWSTR)(res + offset); + } + + return name;
This makes sense, but please use types from ntdll/actctx.c (just duplicate what you need in user32, same as it's done in ole32 and kernel32 already). Also 3 here stands for ACTIVATION_CONTEXT_SECTION_WINDOW_CLASS_REDIRECTION.
+} +
/***********************************************************************
* register_builtin
@@ -576,10 +612,12 @@ ATOM WINAPI RegisterClassExA( const WNDCLASSEXA* wc )
if (!IS_INTRESOURCE(wc->lpszClassName)) {
+ LPCWSTR versioned_name;
WCHAR name[MAX_ATOM_LEN + 1];
if (!MultiByteToWideChar( CP_ACP, 0, wc->lpszClassName, -1, name, MAX_ATOM_LEN + 1 )) return 0;> - classPtr = CLASS_RegisterClass( name, instance, !(wc->style & CS_GLOBALCLASS), + versioned_name = CLASS_GetVersionedName( name ); + classPtr = CLASS_RegisterClass( versioned_name, instance, !(wc->style & CS_GLOBALCLASS),> wc->style, wc->cbClsExtra, wc->cbWndExtra );
}
Why do you need this in -A call too?
I didn't test the ASCII functions on a real Windows instance, but why do you think this shouldn't have the class redirections too?
else
@@ -619,6 +657,7 @@ ATOM WINAPI RegisterClassExW( const WNDCLASSEXW* wc )
ATOM atom; CLASS *classPtr; HINSTANCE instance;
+ LPCWSTR versioned_name;
GetDesktopWindow(); /* create the desktop window to trigger builtin class registration */> @@ -630,7 +669,9 @@ ATOM WINAPI RegisterClassExW( const WNDCLASSEXW* wc )
} if (!(instance = wc->hInstance)) instance = GetModuleHandleW( NULL );
- if (!(classPtr = CLASS_RegisterClass( wc->lpszClassName, instance, !(wc->style & CS_GLOBALCLASS), + versioned_name = CLASS_GetVersionedName( wc->lpszClassName ); + + if (!(classPtr = CLASS_RegisterClass( versioned_name, instance, !(wc->style & CS_GLOBALCLASS),> wc->style, wc->cbClsExtra, wc->cbWndExtra )))
return 0;
This part I looks wrong. Why would it register something that is not what user asked for? My understanding is that comctl32 itself should register its versioned classes (and in ideal v5 vs v6 separation only v6 module should be doing that). So comctl32 is loaded, it registers regular classes first, like Listview, then it registered versioned names like 6.0.0.0!Listview.
Well, I had to disassemble this chunk of code, but I can suggest you to do the following test, which doesn't violate the Clean Room design. 1. On Windows XP or higher, try to register a "Button" class with CS_GLOBALCLASS. This should fail. 2. Add a manifest file like: <?xml version="1.0" encoding="UTF-8" standalone="yes"?> <assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0"> <assemblyIdentity type="win32" name="Wine.TestMe" version="1.0.0.0" /> <file name="yourapp.exe"> <windowClass>Button</windowClass> </file> </assembly> 3. Launch your application again. Now the "Button" class is registered, and it is actually "1.0.0.0!Button". You can create a new window with both names. This test shows, that RegisterClassExW takes care about the prefix itself, and comctl32 doesn't.
What RegisterClassExW should probably check is if target class in current context is already registered, so if application tries to register ClassName, and context has redirection set as 6.0.0.0!ClassName, registration should fail. I think it should be possible to add a test for that:
- create temporary context with window class redirection section; - manually register corresponding versioned class name; - activate context; - see what happens if you try to register non-version class.
I vaguely remember I tried something like that on Windows years ago, and as expected it breaks this whole redirection scheme, because if 6.x.x.x!Button is already registered by the time context is activated comctl32 won't be able to register it again.
See above.
@@ -678,6 +719,7 @@ BOOL WINAPI UnregisterClassA( LPCSTR className, HINSTANCE hInstance )> BOOL WINAPI UnregisterClassW( LPCWSTR className, HINSTANCE hInstance ) {
CLASS *classPtr = NULL;
+ LPCWSTR versioned_name = CLASS_GetVersionedName( className );
GetDesktopWindow(); /* create the desktop window to trigger builtin class registration */> @@ -685,7 +727,7 @@ BOOL WINAPI UnregisterClassW( LPCWSTR className, HINSTANCE hInstance )> {
req->instance = wine_server_client_ptr( hInstance ); if (!(req->atom = get_int_atom_value(className)) && className)
- wine_server_add_data( req, className, strlenW(className) * sizeof(WCHAR) ); + wine_server_add_data( req, versioned_name, strlenW(versioned_name) * sizeof(WCHAR) );> if (!wine_server_call_err( req )) classPtr = wine_server_get_ptr( reply->client_ptr );> } SERVER_END_REQ;
@@ -1192,10 +1234,15 @@ BOOL WINAPI GetClassInfoExA( HINSTANCE hInstance, LPCSTR name, WNDCLASSEXA *wc )> if (!IS_INTRESOURCE(name)) {
+ LPCWSTR versioned_name;
WCHAR nameW[MAX_ATOM_LEN + 1]; if (!MultiByteToWideChar( CP_ACP, 0, name, -1, nameW, sizeof(nameW)/sizeof(WCHAR) ))> return FALSE;
- classPtr = CLASS_FindClass( nameW, hInstance ); + versioned_name = CLASS_GetVersionedName( nameW ); + classPtr = CLASS_FindClass( versioned_name, hInstance ); + /* FIXME: I believe we shouldn't fall back to the normal class. Fill + * free to remove it if necessary. */ + if (!classPtr) classPtr = CLASS_FindClass( nameW, hInstance );
} else classPtr = CLASS_FindClass( (LPCWSTR)name, hInstance );
@@ -1230,6 +1277,7 @@ BOOL WINAPI GetClassInfoExW( HINSTANCE hInstance, LPCWSTR name, WNDCLASSEXW *wc> {
ATOM atom; CLASS *classPtr;
+ LPCWSTR versioned_name;
TRACE("%p %s %p\n", hInstance, debugstr_w(name), wc);
@@ -1241,10 +1289,21 @@ BOOL WINAPI GetClassInfoExW( HINSTANCE hInstance, LPCWSTR name, WNDCLASSEXW *wc> if (!hInstance) hInstance = user32_module;
- if (!(classPtr = CLASS_FindClass( name, hInstance ))) + versioned_name = CLASS_GetVersionedName( name ); + + classPtr = CLASS_FindClass( versioned_name, hInstance ); + if (!classPtr)
{
- SetLastError( ERROR_CLASS_DOES_NOT_EXIST ); - return FALSE; + /* + * FIXME: It's not correct to fall back to the normal class, but our + * comctl32 implementation depends on it... + */ + classPtr = CLASS_FindClass( name, hInstance ); + if (!classPtr) + { + SetLastError( ERROR_CLASS_DOES_NOT_EXIST ); + return FALSE; + }
} wc->style = classPtr->style; wc->lpfnWndProc = WINPROC_GetProc( classPtr->winproc, TRUE );
GetClassInfo part looks okay. It shouldn't break anything even if we had it today, except that it adds bogus context lookup that is a waste of time.
CLASS_GetVersionedName should return the original class name, if no activation context exist. The only reason I wrote a fall-back routine, because comctl32 needs to get a pointer to a windowproc from user32. I believe Windows doesn't has this "fall-back" mode.
diff --git a/dlls/user32/user_private.h b/dlls/user32/user_private.h index 0b5b2ac..0c212ce 100644 --- a/dlls/user32/user_private.h +++ b/dlls/user32/user_private.h @@ -281,6 +281,8 @@ extern void SPY_EnterMessage( INT iFlag, HWND hwnd, UINT msg, WPARAM wParam, LPA> extern void SPY_ExitMessage( INT iFlag, HWND hwnd, UINT msg,
LRESULT lReturn, WPARAM wParam, LPARAM lParam ) DECLSPEC_HIDDEN;
+extern LPCWSTR CLASS_GetVersionedName( LPCWSTR name ) DECLSPEC_HIDDEN; +
#include "pshpack1.h"
typedef struct
diff --git a/dlls/user32/win.c b/dlls/user32/win.c index a7be4a3..42676b5 100644 --- a/dlls/user32/win.c +++ b/dlls/user32/win.c @@ -1740,9 +1740,11 @@ HWND WINAPI DECLSPEC_HOTPATCH CreateWindowExA( DWORD exStyle, LPCSTR className,> if (!IS_INTRESOURCE(className)) {
WCHAR bufferW[256];
+ LPCWSTR versioned_name;
if (!MultiByteToWideChar( CP_ACP, 0, className, -1, bufferW, sizeof(bufferW)/sizeof(WCHAR) ))> return 0;
- return wow_handlers.create_window( (CREATESTRUCTW *)&cs, bufferW, instance, FALSE ); + versioned_name = CLASS_GetVersionedName( bufferW ); + return wow_handlers.create_window( (CREATESTRUCTW *)&cs, versioned_name, instance, FALSE );> } /* Note: we rely on the fact that CREATESTRUCTA and */ /* CREATESTRUCTW have the same layout. */
@@ -1760,6 +1762,7 @@ HWND WINAPI DECLSPEC_HOTPATCH CreateWindowExW( DWORD exStyle, LPCWSTR className,> HINSTANCE instance, LPVOID data )
{
CREATESTRUCTW cs;
+ LPCWSTR versioned_name = CLASS_GetVersionedName( className );
cs.lpCreateParams = data; cs.hInstance = instance;
@@ -1774,7 +1777,7 @@ HWND WINAPI DECLSPEC_HOTPATCH CreateWindowExW( DWORD exStyle, LPCWSTR className,> cs.lpszClass = className; cs.dwExStyle = exStyle;
- return wow_handlers.create_window( &cs, className, instance, TRUE ); + return wow_handlers.create_window( &cs, versioned_name, instance, TRUE );> }
Looks fine, but it could go to WIN_CreateWindowEx().
There's one potentially very important thing about all this, according to docs every winproc call is guaranteed to happen under correct context. So basically if you send a message while particular context was activated, if this window belongs to different thread docs imply that context is temporary activated around winproc call on that receiving thread. I was unable to make this happen, maybe a fresh pair of eyes will be helpful.
I tried something like this, but FindActCtxSectionStringW fails if you call it too early, what is the case for the internal functions. Therefore I decided to place it higher.
On 05.11.2016 19:31, Ivan Akulinchev wrote:
Sorry, forgot to CC wine-devel(a)winehq.org...
-------------
Hi, thanks for a reply!
On Saturday, 5 November 2016 17:56:47 CET you wrote:
On 05.11.2016 16:36, Ivan Akulinchev wrote:
Signed-off-by: Ivan Akulinchev <ivan.akulinchev(a)gmail.com> ---
dlls/comctl32/comctl32.manifest | 1 - dlls/comctl32/theming.c | 15 +++++++-- dlls/user32/class.c | 73 +++++++++++++++++++++++++++++++++++++---- dlls/user32/user_private.h | 2 ++ dlls/user32/win.c | 7 ++-- 5 files changed, 86 insertions(+), 12 deletions(-)
Hi, thanks for looking into this.
diff --git a/dlls/comctl32/comctl32.manifest b/dlls/comctl32/comctl32.manifest index 4c86137..5aa55a3 100644 --- a/dlls/comctl32/comctl32.manifest +++ b/dlls/comctl32/comctl32.manifest @@ -12,7 +12,6 @@
<windowClass>NativeFontCtl</windowClass> <windowClass>ReBarWindow32</windowClass> <windowClass>ScrollBar</windowClass>
- <windowClass>Static</windowClass>
<windowClass>SysAnimate32</windowClass> <windowClass>SysDateTimePick32</windowClass> <windowClass>SysHeader32</windowClass>
Something tells me we'll need opposite logic here, if versioned classes support are to be added one by one.
This line ("<windowClass>Static</windowClass>") tells Windows to use the "6.0.0.0!Static" class instead of "Static". However this class does not exist and all applications crash.
If you remove this line, the standard Static class from user32 is used.
Right, but we don't want that. The goal is to use class from comctl32, that's the point of all changes you're making.
diff --git a/dlls/comctl32/theming.c b/dlls/comctl32/theming.c index 93d6fe6..b78ecc9 100644 --- a/dlls/comctl32/theming.c +++ b/dlls/comctl32/theming.c @@ -28,6 +28,7 @@
#include "comctl32.h" #include "uxtheme.h" #include "wine/debug.h"
+#include "wine/unicode.h"
WINE_DEFAULT_DEBUG_CHANNEL(theming);
@@ -124,8 +125,6 @@ void THEMING_Initialize (void)
static const WCHAR refDataPropName[] =
{ 'C','C','3','2','T','h','e','m','i','n','g','D','a','t','a',0 };
- if (!IsThemeActive()) return; -
atSubclassProp = GlobalAddAtomW (subclassPropName); atRefDataProp = GlobalAddAtomW (refDataPropName);
@@ -142,6 +141,18 @@ void THEMING_Initialize (void)
} originalProcs[i] = class.lpfnWndProc; class.lpfnWndProc = subclassProcs[i];
+ + /* + * FIXME: The Dialog class ('#32770') should NOT be overridden here. + * + * Temporarily ignore this issue using the hack below... + */ + if (strcmpiW(subclasses[i].className, dialogClass) != 0) + { + /* If not a dialog (see above), make the class global */ + class.style |= CS_GLOBALCLASS; + class.hInstance = NULL; + }
if (!class.lpfnWndProc) {
All theming.c should go when transition is complete, because once you're able to create 6.0.0.0!Static it will support theme drawing API regardless if theming is enabled or not.
The problem here is that it is impossible to create a versioned class for #32770, and therefore we can not create a global class - only a local one. I believe this stuff is a part of the undocumented Theme Hooks API, and not a part of comctl32...
theming.c is needed because nobody wants to copy & paste the corresponding code from user32, as Microsoft did. Maybe we can create a private library containing the base classes, used by both libraries?
I actually do want to duplicate code from user32 one way or another, and once done it want be called theming.c, because it's not limited to visual appearance.
/***********************************************************************
* register_builtin
@@ -576,10 +612,12 @@ ATOM WINAPI RegisterClassExA( const WNDCLASSEXA* wc )
if (!IS_INTRESOURCE(wc->lpszClassName)) {
+ LPCWSTR versioned_name;
WCHAR name[MAX_ATOM_LEN + 1];
if (!MultiByteToWideChar( CP_ACP, 0, wc->lpszClassName, -1, name, MAX_ATOM_LEN + 1 )) return 0;> - classPtr = CLASS_RegisterClass( name, instance, !(wc->style & CS_GLOBALCLASS), + versioned_name = CLASS_GetVersionedName( name ); + classPtr = CLASS_RegisterClass( versioned_name, instance, !(wc->style & CS_GLOBALCLASS),> wc->style, wc->cbClsExtra, wc->cbWndExtra );
}
Why do you need this in -A call too?
I didn't test the ASCII functions on a real Windows instance, but why do you think this shouldn't have the class redirections too?
Yes, sorry about that, I thought that we forward from A to W as we usually do. It probably makes sense to duplicate GetVersionedName as you did, if only to avoid calling context functions when class IS_INTRESOURCE().
else
@@ -619,6 +657,7 @@ ATOM WINAPI RegisterClassExW( const WNDCLASSEXW* wc )
ATOM atom; CLASS *classPtr; HINSTANCE instance;
+ LPCWSTR versioned_name;
GetDesktopWindow(); /* create the desktop window to trigger builtin class registration */> @@ -630,7 +669,9 @@ ATOM WINAPI RegisterClassExW( const WNDCLASSEXW* wc )
} if (!(instance = wc->hInstance)) instance = GetModuleHandleW( NULL );
- if (!(classPtr = CLASS_RegisterClass( wc->lpszClassName, instance, !(wc->style & CS_GLOBALCLASS), + versioned_name = CLASS_GetVersionedName( wc->lpszClassName ); + + if (!(classPtr = CLASS_RegisterClass( versioned_name, instance, !(wc->style & CS_GLOBALCLASS),> wc->style, wc->cbClsExtra, wc->cbWndExtra )))
return 0;
This part I looks wrong. Why would it register something that is not what user asked for? My understanding is that comctl32 itself should register its versioned classes (and in ideal v5 vs v6 separation only v6 module should be doing that). So comctl32 is loaded, it registers regular classes first, like Listview, then it registered versioned names like 6.0.0.0!Listview.
Well, I had to disassemble this chunk of code, but I can suggest you to do the following test, which doesn't violate the Clean Room design.
1. On Windows XP or higher, try to register a "Button" class with CS_GLOBALCLASS. This should fail. 2. Add a manifest file like: <?xml version="1.0" encoding="UTF-8" standalone="yes"?> <assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0"> <assemblyIdentity type="win32" name="Wine.TestMe" version="1.0.0.0" /> <file name="yourapp.exe"> <windowClass>Button</windowClass> </file> </assembly> 3. Launch your application again. Now the "Button" class is registered, and it is actually "1.0.0.0!Button". You can create a new window with both names.
If you disassembled user32/comctl32 dlls, I suggest to stop this conversation, none of what you submitted is usable now.
If you disassembled user32/comctl32 dlls
Yes, I disassembled comctl32. Have I changed something in the Wine's implementation of this library? Nothing. I just "commented out" the obviously wrong code (otherwise it wouldn't work) and changed some class properties according to this article [1] from MSDN (section "Registering a Window Class", see the note about CS_GLOBALCLASS).
My understanding is that comctl32 itself should register its versioned classes
Well, after the months of debugging you'll finally find out that your assertion was wrong and (as far as only 2 options are possible) you'll write exactly the same thing, as I did. In this case, I just saved my time, starting with the right option, and skipping the wrong one. I believe this action does not violate the Clean Room design. [1] https://msdn.microsoft.com/en-us/library/windows/desktop/ms633574.aspx
On Sun, Nov 6, 2016, 13:32 Ivan Akulinchev <ivan.akulinchev(a)gmail.com> wrote:
If you disassembled user32/comctl32 dlls
Yes, I disassembled comctl32. [...] I believe this action does not violate the Clean Room design.
You are not a lawyer. And even if you were a lawyer, you are not a lawyer representing the project. What you believe does little to impress a team of lawyers hired by one of the largest companies on Earth.
There is a clear set of guidelines on this topic. https://wiki.winehq.org/Disassembly https://wiki.winehq.org/Clean_Room_Guidelines
On Sunday, 6 November 2016 20:55:04 CET you wrote:
On Sun, Nov 6, 2016, 13:32 Ivan Akulinchev <ivan.akulinchev(a)gmail.com>
wrote:
If you disassembled user32/comctl32 dlls
Yes, I disassembled comctl32. [...] I believe this action does not violate the Clean Room design.
You are not a lawyer. And even if you were a lawyer, you are not a lawyer
representing the project. What you believe does little to impress a team of lawyers hired by one of the largest companies on Earth.
There is a clear set of guidelines on this topic. https://wiki.winehq.org/Disassembly https://wiki.winehq.org/Clean_Room_Guidelines
You should not miss the common part of my message. I put "believe" at the end just to make the last sentence more polite. The CLASS_GetVersionedName function was written by me before I started the disassembler at all and I used the similar functions from comctl32/tests/ v6util.h and comctl32/tests/button.c as a basis. I believe this files are "clean" as far as they are already in the Wine source tree (if they aren't, they should be urgently removed out of there, and I should be recognized as a victim). The next question was where should this function be: user32, comctl32, both of them or somewhere else. Looking at the Wine source code I suggested, that only first two libraries make sense for me, because user32 is the endpoint for all class-related stuff and don't pass the class name to the underling libraries (except the Wine server). Therefore it was clearly, that I should place a copy of this function in user32 (in our case), as far as MSDN says [1] the only thing I need to do to enable the Visual Styles support in my application is to use a manifest file. In other words, this means, that the class names conversation stuff is out of my application (i.e. a part of user32 or its underling libraries, if any). Then (and only then) I tried to locate this function inside the original Microsoft's binary using a disassembler and I was unable to locate it there. Relying on my experience I suggested, that no functions similar to the function I wrote is in the library. Writing the patch I was 100% sure in it, but this doesn't mean, that this function is really out of there (Microsoft could use a very cool anti-disassembling technology to hide this function, but it's very unlikely). Therefore I decided to limit my changes to user32, and the tests I done after (tests were based on comctl32/ tests/v6util.h, comctl32/tests/button.c, comctl32/comctl32.manifest and my own program I wrote based on the Win32 Hello World application from MSDN [2]) confirmed that. Later I had to make some changes in comctl32, because it didn't want to work with my patch. Changing comctl32 I couldn't use the information I learned analyzing the original implementation, because I was unable to find a Microsoft's manual "How to fix Wine after Ivan's patches" there (really, it's stupid to make such statements). If you read this line and skipped the paragraph above, it's OK. Just say me the laws of what country does the Wine's lawyer use, and I'll find you an authoritative research confirming, that the things I wrote above don't violate the laws of that country (well, the legal system is not mathematics, but I'll likely find a precedent, if this country has such term). Another question is, what if I'll resend the patch, doing the stuff intentionally "not like in Windows" (it is theoretically possible, but may break some third-party libraries). Or does "doing intentionally not like in X" violate the law as well, if I know, how does X works? (links to the long paragraph above, skip it, if you didn't read it) [1] https://msdn.microsoft.com/en-us/library/windows/desktop/bb773175.aspx [2] https://msdn.microsoft.com/en-us/library/h9x39eaw.aspx
Ivan Akulinchev <ivan.akulinchev(a)gmail.com> writes:
On Sunday, 6 November 2016 20:55:04 CET you wrote:
On Sun, Nov 6, 2016, 13:32 Ivan Akulinchev <ivan.akulinchev(a)gmail.com>
wrote:
If you disassembled user32/comctl32 dlls
Yes, I disassembled comctl32. [...] I believe this action does not violate the Clean Room design.
You are not a lawyer. And even if you were a lawyer, you are not a lawyer
representing the project. What you believe does little to impress a team of lawyers hired by one of the largest companies on Earth.
There is a clear set of guidelines on this topic. https://wiki.winehq.org/Disassembly https://wiki.winehq.org/Clean_Room_Guidelines
You should not miss the common part of my message. I put "believe" at the end just to make the last sentence more polite.
The CLASS_GetVersionedName function was written by me before I started the disassembler at all and I used the similar functions from comctl32/tests/ v6util.h and comctl32/tests/button.c as a basis. I believe this files are "clean" as far as they are already in the Wine source tree (if they aren't, they should be urgently removed out of there, and I should be recognized as a victim).
Disassembling Windows is not an acceptable way of figuring out how things work, and we have clear rules about this. I'm afraid that I can't accept any patches of yours in this area. -- Alexandre Julliard julliard(a)winehq.org
participants (4)
-
Alexandre Julliard -
Christopher Harrington -
Ivan Akulinchev -
Nikolay Sivov