Fixing bug #2181, standard EDIT class extra value

[Julius Schwartzenberg]

Hi,

I'm trying to solve bug #2181. With help from Roderick, I created this 
patch together with a test, which allows Civilization II to work.

I noticed that there are currently no similar tests which check the 
standard classes. Shouldn't at least the extra values of the other 
classes be tested too? In that case I would try to add tests for those too.

The current test works for me on Windows 98 & XP. Can others try
it on other Windows versions?

This is the first time I'm developing for Wine, so please let me know if 
you have any comments.

Regards,
Julius

[Julius Schwartzenberg]

Julius Schwartzenberg wrote:
> This is the first time I'm developing for Wine

and I made a stupid beginners mistake. Here is the corrected patch.

[Dmitry Timoshkov]

"Julius Schwartzenberg"  wrote:
> diff --git a/dlls/user32/edit.c b/dlls/user32/edit.c
> index 8ad945b..53eb052 100644
> --- a/dlls/user32/edit.c
> +++ b/dlls/user32/edit.c
> @@ -5416,7 +5416,7 @@ const struct builtin_class_descr EDIT_builtin_class =
>     CS_DBLCLKS | CS_PARENTDC,   /* style */
>     EditWndProcA,         /* procA */
>     EditWndProcW,         /* procW */
> - sizeof(EDITSTATE *),  /* extra */
> - 6,                    /* extra */
> IDC_IBEAM,            /* cursor */
> 0                     /* brush */
>
> };

This obviously won't work for 64-bit. I'd suggest to make extra
2 * sizeof(void *). and add a comment about compatibility.
> diff --git a/dlls/user32/tests/edit.c b/dlls/user32/tests/edit.c
> index 5ca012e..ec00b90 100644
> --- a/dlls/user32/tests/edit.c
> +++ b/dlls/user32/tests/edit.c
> @@ -2260,6 +2260,15 @@ static void test_dialogmode(void)
>     destroy_child_editcontrol(hwEdit);
> }
>
> +static void test_extra_value()
> +{
> - WNDCLASSEX cls;
> - int r;
> - GetClassInfoEx(NULL,"Edit",&cls);
> - r = cls.cbWndExtra;
> - ok(6 == r, "expected 6, got %d\n", r);
>
> +}

ok(cls.cbWndExtra > sizeof(void *), "blah ...\n") doesn't require
any intermediate variables.

[Julius Schwartzenberg]

Dmitry Timoshkov wrote:
> "Julius Schwartzenberg"  wrote:
>
> This obviously won't work for 64-bit. I'd suggest to make extra
> 2 * sizeof(void *). and add a comment about compatibility.
>
> ok(cls.cbWndExtra > sizeof(void *), "blah ...\n") doesn't require
> any intermediate variables.

Thanks for your feedback! I turns out however that Civilization II 
crashes with anything other than 6, so I used an IFDEF to check for 
64-bit as advised on IRC. The 64-bit version of Windows XP returns 8 
instead of 6 and Civilization II crashes at exactly the same point as 
Wine in it.

I've attached a new version and split the test and the patch. The test 
will fail on 64-bit Windows XP however, just like Civilization II. What 
is the best way to cope with this?

I also wondered whether it wouldn't be good to also test the extra 
values of the other standard classes instead of just the edit class. I 
created a table which shows all the return values for different Windows 
versions & Wine:
http://haar.student.utwente.nl/~julius/extra_value_table

To my eyes it seems the edit class is the only candidate that needs a 
test, but maybe someone more experienced could judge the table. In case 
other classes may also cause problems I will also add tests for those.

Julius

From cba35b6ba1a5deb18f721780181907050091c596 Mon Sep 17 00:00:00 2001
From: Julius Schwartzenberg 
Date: Tue, 6 Oct 2009 22:59:54 +0200
Subject: Fix for bug #2181. Changes the extra value of the standard edit class from 4 to 6.

---
 dlls/user32/edit.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/dlls/user32/edit.c b/dlls/user32/edit.c
index 8ad945b..e1159f9 100644
--- a/dlls/user32/edit.c
+++ b/dlls/user32/edit.c
@@ -5410,13 +5410,19 @@ static LRESULT WINAPI EditWndProcW(HWND hWnd, UINT uMsg, WPARAM wParam, LPARAM l
  * edit class descriptor
  */
 static const WCHAR editW[] = {'E','d','i','t',0};
+#ifdef _WIN64
+#define EDIT_EXTRA_VALUE sizeof(EDITSTATE *)
+#else
+#define EDIT_EXTRA_VALUE 6 /* This has to be 6 for 32-bit, otherwise Civilization II crashes, bug #2181  */
+#endif
 const struct builtin_class_descr EDIT_builtin_class =
 {
     editW,                /* name */
     CS_DBLCLKS | CS_PARENTDC,   /* style */
     EditWndProcA,         /* procA */
     EditWndProcW,         /* procW */
-    sizeof(EDITSTATE *),  /* extra */
+    EDIT_EXTRA_VALUE,     /* extra */
     IDC_IBEAM,            /* cursor */
     0                     /* brush */
 };
+#undef EDIT_EXTRA_VALUE

[Dmitry Timoshkov]

"Julius Schwartzenberg"  wrote:
> static const WCHAR editW[] = {'E','d','i','t',0};
> +#ifdef _WIN64
> +#define EDIT_EXTRA_VALUE sizeof(EDITSTATE *)
> +#else
> +#define EDIT_EXTRA_VALUE 6 /* This has to be 6 for 32-bit, otherwise Civilization II crashes, bug #2181  */
> +#endif
> const struct builtin_class_descr EDIT_builtin_class =
> {
>     editW,                /* name */
>     CS_DBLCLKS | CS_PARENTDC,   /* style */
>     EditWndProcA,         /* procA */
>     EditWndProcW,         /* procW */
> - sizeof(EDITSTATE *),  /* extra */
> - EDIT_EXTRA_VALUE,     /* extra */
> IDC_IBEAM,            /* cursor */
> 0                     /* brush */
>
> };
> +#undef EDIT_EXTRA_VALUE

It would be cleaner IMHO to have it the following way:

#ifdef _WIN64
#define EDIT_EXTRA_VALUE 0
#else
#define EDIT_EXTRA_VALUE 2
#endif

const struct builtin_class_descr EDIT_builtin_class =
{
    editW,                /* name */
    CS_DBLCLKS | CS_PARENTDC,   /* style */
    EditWndProcA,         /* procA */
    EditWndProcW,         /* procW */
    sizeof(EDITSTATE *) + EDIT_EXTRA_VALUE,  /* extra */
...

and completely omit the comment about the bug #, but explain that there are
applications (Civilization II is one of them) that depend on having 2 extra
bytes in the extra class storage.
> +static void test_extra_value()
> +{
> +todo_wine {
> - WNDCLASSEX cls;
> - GetClassInfoEx(NULL,"Edit",&cls);
> - #ifdef _WIN64
> - ok(cls.cbWndExtra == 8, "expected 6, got %d\n", cls.cbWndExtra);
> - #else
> - ok(cls.cbWndExtra == 6, "expected 6, got %d\n", cls.cbWndExtra);
> - #endif
>
> +}
> +}

You should check the return value of GetClassInfoEx() and put todo_wine
only around the failing ok() call (otherwise it will fail under 64-bit).

[Dmitry Timoshkov]

"Dmitry Timoshkov"  wrote:
> > - #ifdef _WIN64
> > - ok(cls.cbWndExtra == 8, "expected 6, got %d\n", cls.cbWndExtra);
> > - #else
> > - ok(cls.cbWndExtra == 6, "expected 6, got %d\n", cls.cbWndExtra);
> > - #endif
> >
> > +}
> > +}
>
> You should check the return value of GetClassInfoEx() and put todo_wine
> only around the failing ok() call (otherwise it will fail under 64-bit).

And of course fix the message in the ok() for 64-bit case.

[Julius Schwartzenberg]

Dmitry Timoshkov wrote:
> "Dmitry Timoshkov"  wrote:
> > > - #ifdef _WIN64
> > > - ok(cls.cbWndExtra == 8, "expected 6, got %d\n", cls.cbWndExtra);
> > > - #else
> > > - ok(cls.cbWndExtra == 6, "expected 6, got %d\n", cls.cbWndExtra);
> > > - #endif
> > >
> > > +}
> > > +}
> >
> > You should check the return value of GetClassInfoEx() and put todo_wine
> > only around the failing ok() call (otherwise it will fail under 64-bit).
>
> And of course fix the message in the ok() for 64-bit case.

Thanks again for your comments!

Hereby again a new version. Based on help from IRC, I added the broken 
call for Windows XP 64-bit. Based on the values from [1] I also added a 
test for the Dialog class (and moved the tests to class.c). Although 
this particular value is just defined in winuser.h maybe tests for other 
extra values could be useful in the future.

Please let me know what you think.

[1] http://haar.student.utwente.nl/~julius/extra_value_table (updated)
> From ad33ba4e77207be1e36f1717003fc7c0623d9be5 Mon Sep 17 00:00:00 2001

From: Julius Schwartzenberg 
Date: Sun, 11 Oct 2009 15:27:44 +0200
Subject: Fix for bug #2181. Changes the extra value of the standard edit class from 4 to 6.

---
 dlls/user32/edit.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/dlls/user32/edit.c b/dlls/user32/edit.c
index 8ad945b..e42ed41 100644
--- a/dlls/user32/edit.c
+++ b/dlls/user32/edit.c
@@ -5410,13 +5410,19 @@ static LRESULT WINAPI EditWndProcW(HWND hWnd, UINT uMsg, WPARAM wParam, LPARAM l
  * edit class descriptor
  */
 static const WCHAR editW[] = {'E','d','i','t',0};
+#ifdef _WIN64
+#define EDIT_EXTRA_VALUE 0
+#else
+#define EDIT_EXTRA_VALUE 2 /* This has to be 6 in total for 32-bit, otherwise Civilization II crashes, bug #2181  */
+#endif
 const struct builtin_class_descr EDIT_builtin_class =
 {
     editW,                /* name */
     CS_DBLCLKS | CS_PARENTDC,   /* style */
     EditWndProcA,         /* procA */
     EditWndProcW,         /* procW */
-    sizeof(EDITSTATE *),  /* extra */
+    sizeof(EDITSTATE *) + EDIT_EXTRA_VALUE,   /* extra */
     IDC_IBEAM,            /* cursor */
     0                     /* brush */
 };
+#undef EDIT_EXTRA_VALUE