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.
<rdar://problem/8953558>
Created attachment 281724 [details] Patch
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?
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.
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 for reviewing, guys!
Committed r202326: <https://trac.webkit.org/changeset/202326>
(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.