Re: 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  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.