[PATCH 2/3] user32: Added more dialog owner tests.

[Dmitry Timoshkov]

Jacek Caban <jacek at codeweavers.com> wrote:

> >>>>>>>>>>>>> Please don't create fake embedded blocks.
> >>>>>>>>>>>> I need this declaration here, otherwise I wouldn't be able to use
> >>>>>>>>>>>> handles for testing lparam value.
> >>>>>>>>>>> wparam/lparam could be tested directly in the window procedure.
> >>>>>>>>>> It would require some global variables and new window procedure. It's
> >>>>>>>>>> cleaner to do it using existing tools IMO.
> >>>>>>>>> Writing the tests for user32 requires a bit of efforts, yes.
> >>>>>>>> I'd prefer to respond to comments that intend to improve the patch
> >>>>>>>> instead of comments that intend to increase efforts.
> >>>>>>> I already described the ways how to improve the patch. Improving a patch
> >>>>>>> and writing good tests usually increases the efforts, not avoids them.
> >>>>>> Yes, and I addressed all of your comments on wine-devel.
> >>>>>>
> >>>>>> - SetParent needs to be used. If we used CreateWindowEx without WS_CHILD 
> >>>>>> (which the test needs), you'd have an owned window, not child window. Do 
> >>>>>> you need more explanation?
> >>>>>> - As I explained you, I already added GetParent()/GetWindow() tests and 
> >>>>>> fixes for this case to Wine and those are unrelated to the problem that 
> >>>>>> this series fixes. I even showed that to you before you asked me to add 
> >>>>>> them.
> >>>>>> - My tests do test that messages are sent to child (there is parent flag 
> >>>>>> in message sequence which is used for that). That's what you wanted to 
> >>>>>> be tested, but it's there in the patch already.
> >>>>>>
> >>>>>> Did I miss anything?
> >>>>> Since SetParent() apparently makes a difference it would be interesting to
> >>>>> see the tests without it (with parent specified in the CreateWindowEx call).
> >>>> Then there is nothing new. Parent will be the owner same as in other
> >>>> already existing tests.
> >>> But there is no an existing test that checks the lparam the way you seem
> >>> to be mostly interested.
> >> I wasn't mostly interested in them, but they are a nice feature. I was
> >> making my new tests stricter when I was experimenting with them and I
> >> think it's a good thing, so I left it in the patch. It doesn't mean we
> >> should rewrite existing tests to do the same.
> > I'm not asking to rewrite existing tests, I'm asking to add another test
> > which does exactly the same as your new test but specifies parent in the
> > CreateWindowEx instead of using SetParent.
> 
> I already explained to you that it would not test anything that's not
> already tested. We have owner tests for various CreateWindowEx and
> SetParent combinations in win.c and we have modal dialog tests that have
> regular (as in not reparented) top level owner in msg.c.

I don't understand why you're so much persistent about adding another test
that basically replicates what your new tests do, but with different parent.
Such a test clearly doesn't exist, and it would extend the test coverage for
your fixes, it's not apparent at all that this new test won't break due to
them.

-- 
Dmitry.