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

[Jacek Caban]

Hi Dmitry,

On 02/29/16 17:16, Dmitry Timoshkov wrote:
> Hi Jacek,
>
> Jacek Caban <jacek at codeweavers.com> wrote:
>
>> +    parent = CreateWindowExA(0, "TestParentClass", "Test parent",
>> +                             WS_OVERLAPPEDWINDOW | WS_VISIBLE,
>> +                             100, 100, 200, 200, 0, 0, 0, NULL);
>> +    ok (parent != 0, "Failed to create parent window\n");
>> +
>> +    child = CreateWindowExA(0, "TestWindowClass", "Test child",
>> +                            WS_OVERLAPPEDWINDOW | WS_VISIBLE,
>> +                            100, 100, 200, 200, 0, 0, 0, NULL);
>> +    ok (child != 0, "Failed to create child window\n");
>> +
>> +    child2 = CreateWindowExA(0, "SimpleWindowClass", "Test child2",
>> +                             WS_OVERLAPPEDWINDOW | WS_VISIBLE | WS_CHILD,
>> +                             100, 100, 200, 200, child, 0, 0, NULL);
>> +    ok (child2 != 0, "Failed to create child window\n");
>> +
>> +    SetParent(child, parent);
>> +    SetFocus(child);
> Is that really required to use SetParent() instead of specifying correct
> parent in the CreateWindowEx call?

Yes, SetParent and CreateWindowEx's parent are not the same thing. See
for example d13a44e4aa12fa5ad3498c39934e88a7dcd30916 to understand the
difference.

>> +    flush_sequence();
>> +    DialogBoxA( 0, "TEST_DIALOG", child2, TestModalDlgProc2 );
>> +    ok_sequence(WmModalDialogSeq_2, "ModalDialog2", TRUE);
> Is this test supposed to show that actual dialog owner is parent window?
> If yes, then this theory is not tested at all.

No, it's actually supposed to show that owner is not the parent. We know
that owner is child in this case, but current code will use parent
anyway, which is not right. My tests show that messages are sent to
child, not parent.

>> +    SetForegroundWindow(hdlg);
>> +
>> +    {
>> +        const struct message WmEndDialogSeq[] = {
>> +            { WM_ENABLE, sent },
>> +            { WM_WINDOWPOSCHANGING, sent|wparam, SWP_HIDEWINDOW|SWP_NOACTIVATE|SWP_NOSIZE|SWP_NOMOVE },
>> +            { HCBT_ACTIVATE, hook|wparam, (WPARAM)hchild },
>> +            { WM_NCACTIVATE, sent|wparam|lparam, WA_INACTIVE, (LPARAM)hchild },
>> +            { WM_ACTIVATE, sent|wparam|lparam, WA_INACTIVE, (LPARAM)hchild },
>> +            /* FIXME: Following two are optional because Wine sends WM_QUERYNEWPALETTE instead of WM_WINDOWPOSCHANGING */
>> +            { WM_WINDOWPOSCHANGING, sent|wparam|optional, SWP_NOSIZE|SWP_NOMOVE },
>> +            { WM_QUERYNEWPALETTE, sent|optional },
>> +            { WM_NCACTIVATE, sent|wparam|lparam, WA_ACTIVE, (LPARAM)hdlg },
>> +            { WM_GETTEXT, sent|optional|defwinproc },
>> +            { WM_ACTIVATE, sent|wparam|lparam, WA_ACTIVE, (LPARAM)hdlg },
>> +            { HCBT_SETFOCUS, hook|wparam, (WPARAM)hchild },
>> +            { WM_KILLFOCUS, sent|wparam, (WPARAM)hchild },
>> +            { WM_SETFOCUS, sent|defwinproc|wparam, (WPARAM)hdlg },
>> +            { 0 }
>> +        };
>> +
>> +        flush_sequence();
>> +        EndDialog(hdlg, 0);
>> +        ok_sequence(WmEndDialogSeq, "EndDialog", FALSE);
>> +    }
> 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.

> Also it would be really nice to send the tests first so it would be more clear
> when a todo_wine is removed with a subsequent patch in order to see whether
> the patch is actually related to the previously sent tests.

I'm not sure I agree.


Thanks for the review,
Jacek
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20160229/005fb452/attachment.html>