RESOLVED FIXED 53675
window.showModalDialog doesn't work in DumpRenderTree on Windows
https://bugs.webkit.org/show_bug.cgi?id=53675
Summary window.showModalDialog doesn't work in DumpRenderTree on Windows
Adam Roben (:aroben)
Reported 2011-02-03 05:54:23 PST
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.
Attachments
Patch (6.71 KB, patch)
2016-06-21 01:47 PDT, Per Arne Vollan
bfulgham: review+
Adam Roben (:aroben)
Comment 1 2011-02-03 06:36:43 PST
Per Arne Vollan
Comment 2 2016-06-21 01:47:30 PDT
David Kilzer (:ddkilzer)
Comment 3 2016-06-21 11:11:52 PDT
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 4 2016-06-21 11:24:34 PDT
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 5 2016-06-21 11:33:54 PDT
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
Comment 6 2016-06-22 00:51:11 PDT
Thanks for reviewing, guys!
Per Arne Vollan
Comment 7 2016-06-22 00:52:32 PDT
Per Arne Vollan
Comment 8 2016-06-22 04:05:43 PDT
(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.
Note You need to log in before you can comment on or make changes to this bug.