[PATCH 2/2 v3] user32: Add more dialog creation message tests

[Dmitry Timoshkov]

Piotr Caban <piotr at codeweavers.com> wrote:

> On 17/02/16 10:37, Dmitry Timoshkov wrote:
> >>>> On 02/16/16 14:08, Dmitry Timoshkov wrote:
> >>>>> It's getting better, but again doesn't show a lot of details. Please add
> >>>>> parameters for most of the recorded messages in the sequence, in particular
> >>>>> EM_SETSEL and WM_GETDLGCODE are the mandatory ones. Also, in order to see
> >>>>> the origin of EM_SETSEL I'd suggest to call DefDlgProc() manually and use
> >>>>> defwndproc_counter around it together with the defwinproc flag.
> >>>> DefDlgProc can't be called manually in this case. There are already some
> >>>> comments about it in the tests.
> >>> TestDlgProcA() calls DefDlgProc() this way without any problem.
> >> It's because this test is using CreateWindow function family instead of
> >> CreateDialog*.
> > Then please create a window for testing using a way that is appropriate
> > for your testing purposes and that allows using DefDlgProc().
> You're not paying much attention to what's in the patch. I'm modifying 
> part of CreateDialog function so it can't be tested if CreateWindow is 
> used.

I'm sure that there are ways of testing of the discussed functionality, and
personally I find exisitng tests not convincing, and not explaining the reason
of adding a proposed patch.

> > Many of the tests in msg.c and win.c are created by me, I know the related
> > code in user32 pretty well, and based on my knowledge I can't agree that
> > your change is obvious, at least it's absolutely not obvious to me. Focus
> > handling is very fragile part of user32 code, there are applications that
> > are very sensitive to any change in that area, and it's very easy to break
> > them. Please take time to create a more convincing set of tests.
> I'm not changing anything focus related. I'm just adding text selection 
> to code that was already setting focus.

Your patch is changing a part of the code that handles setting the focus, so
it is focus related. I already explained the reason why I ask for additional
tests, I only could add that I spent too much time already figuring out all
kinds of broken user32 behaviour, and I definitely don't won't to see more of
questionable code added in that area. Adding more tests is always good, it not
only excersises the functionality of the target, it's also a way to better
understand behind the scene. user32 is not the piece of code that can be changed
just basing on a test of couple lines long, this area required much more tests
to confirm that the observed behaviour and the proposed change are absolutely
correct.

Please demonstrate with the tests that you deeply understand what is going on
in the area your patch is testing.

-- 
Dmitry.