window.showModalDialog doesn't work in DumpRenderTree on Windows

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

Bug 35350 added support for window.showModalDialog in DumpRenderTree on Mac. A number of tests now rely on this. We should implement this in DumpRenderTree on Windows so that we can run those tests. Until then, the tests are being skipped.

[Adam Roben (:aroben)]

<rdar://problem/8953558>

[Per Arne Vollan]

Created attachment 281724 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 281724 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281724&action=review

Overall this looks good (other than the question I had above), but I'm not a Windows expert, so I'll let Brent or someone else review it officially.

> Tools/DumpRenderTree/win/UIDelegate.cpp:334
> +    COMPtr<IWebViewPrivate2> webViewPrivate;
> +    HRESULT hr = webView->QueryInterface(&webViewPrivate);
> +    if (FAILED(hr))
> +        return nullptr;
> +
> +    HWND webViewWindow = nullptr;
> +    hr = webViewPrivate->viewWindow(&webViewWindow);
> +    if (FAILED(hr))
> +        return nullptr;

Why return nullptr here but 'hr' in UIDelegate::createModalDialog() below?

[Brent Fulgham]

Comment on attachment 281724 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281724&action=review

Looks good! Can we unskip any tests after this change?

>> Tools/DumpRenderTree/win/UIDelegate.cpp:334
>> +        return nullptr;
> 
> Why return nullptr here but 'hr' in UIDelegate::createModalDialog() below?

The function signatures are different. This returns a HWND (pointer), but createModalDialog returns an HRESULT.

[David Kilzer (:ddkilzer)]

Comment on attachment 281724 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=281724&action=review

> Tools/DumpRenderTree/win/UIDelegate.cpp:395
> -HRESULT UIDelegate::runModal(_In_opt_ IWebView*)
> +static HWND findTopLevelParent(HWND window)
>  {
> -    return E_NOTIMPL;
> +    if (!window)
> +        return nullptr;
> +
> +    HWND current = window;
> +    for (HWND parent = GetParent(current); current; current = parent, parent = GetParent(parent)) {
> +        if (!parent)
> +            return current;
> +    }
> +    ASSERT_NOT_REACHED();
> +    return nullptr;
> +}

Can the code from Source/WebKit/win/WebView.cpp be reused here?

See:  Bug 156954: [Win] The function findTopLevelParent should return the first window which is not a child.

(This should not block landing this patch; just a follow-up fix.)

[Per Arne Vollan]

Thanks for reviewing, guys!

[Per Arne Vollan]

Committed r202326: <https://trac.webkit.org/changeset/202326>

[Per Arne Vollan]

(In reply to comment #5)
> Comment on attachment 281724 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=281724&action=review
> 
> > Tools/DumpRenderTree/win/UIDelegate.cpp:395
> > -HRESULT UIDelegate::runModal(_In_opt_ IWebView*)
> > +static HWND findTopLevelParent(HWND window)
> >  {
> > -    return E_NOTIMPL;
> > +    if (!window)
> > +        return nullptr;
> > +
> > +    HWND current = window;
> > +    for (HWND parent = GetParent(current); current; current = parent, parent = GetParent(parent)) {
> > +        if (!parent)
> > +            return current;
> > +    }
> > +    ASSERT_NOT_REACHED();
> > +    return nullptr;
> > +}
> 
> Can the code from Source/WebKit/win/WebView.cpp be reused here?
> 
> See:  Bug 156954: [Win] The function findTopLevelParent should return the
> first window which is not a child.
> 
> (This should not block landing this patch; just a follow-up fix.)

Thanks! This is addressed in https://bugs.webkit.org/show_bug.cgi?id=159021.