user32: Fix error handling in {Begin,End,}DeferWindowPos() to match Windows behavior (resend)

[Nikolay Sivov]

On 25.08.2015 17:01, Rodrigo Rivas wrote:
> [Sorry for the double post, I forgot to reply to the list]
>
> On Tue, Aug 25, 2015 at 3:35 PM, Nikolay Sivov <bunglehead at gmail.com> wrote:
>> On 25.08.2015 16:08, Rodrigo Rivas wrote:
>>>
>>> I sent this on July 15th, but it went through the cracks. Maybe the
>>> mail text was not detailed enough, so I will try and explain it better
>>> this time.
>>>
>>> The error behaviour of functions BeginDeferWindowPos(),
>>> DeferWindowPos() and EndDeferWindowPos() in Wine is different than in
>>> Windows.
>>>
>>> In Windows, when DeferWindowPos() is used with an invalid HWND, this
>>> function fails, but the eventual EndDeferWindowPos() will succeed and
>>> move all the other windows in the set.
>>> In Wine, however, DeferWindowPos() does not check the HWND, so it
>>> never fails. But then, it fails in EndDeferWindowPos(), moving only
>>> the windows passed before the invalid HWND and leaving the windows
>>> after the invalid HWND unmoved.
>>>
>>> The Windows behaviour is consistent in every version I tested (XP and 7).
>>>
>>> The attached patch makes the Wine error behaviour the same as the Windows
>>> one.
>>> It also considers the case where the HWND is valid when calling
>>> DeferWindowPos() but is destroyed before calling EndDeferWindowPos():
>>> The error is simply ignored.
>>>
>>> It fixes bug #23187.
>>>
>>> ---
>>>    dlls/user32/tests/Makefile.in |   1 +
>>>    dlls/user32/tests/defer.c     | 168
>>> ++++++++++++++++++++++++++++++++++++++++++
>>>    dlls/user32/winpos.c          |  14 ++--
>>>    3 files changed, 178 insertions(+), 5 deletions(-)
>>>    create mode 100644 dlls/user32/tests/defer.c
>>>
>>
>> I don't think you need a new file for that, how about moving this to win.c?
>
> It may make sense in win.c. I assumed that user32/tests/win.c is
> related to user32/win.c. And since the affected functions are in
> user32/winpos.c... maybe I should have named it user32/tests/winpos.c?
> But I don't have a strong preference
> If you really want it, would you like me to do the merge and resend
> the patch? Or is it not necessary?
>

We try to avoid adding new files for every new test, to keep number of 
files under reasonable limit. So yes, I think it's better to have that 
moved to win.c, and no, renaming it is not necessary.

>> Regarding fix itself, it's probably better to split it to two - fix
>> DeferWindowPos() in first + related tests, and End* one in the second.
>
> In this I disagree. Changing DeferWindowPos() without changing
> EndDeferWindowPos() would make little sense, because the error
> checking is moving from EDWP to DWP.
> The special case of the window being destroyed between DWP and EDWP is
> just a consequence of EDWP not having any error checking left.
> You could have one patch that adds the check to DWP and another that
> removes it from EDWP, but what's the point? The intermediate step will
> not even pass the test case, it will fail at
> TestDestroyDeferWindowPos().
>
> So to fair you would have to split in: a) fix DWP + tess1+test2; b)
> fix EDWP + test3.
>

Sure, that's what I meant, to have one fix + its tests in first patch, 
followed by second fix + its own tests. But let's see if someone else 
has anything to add to this, it might be okay to have it they way you did.