WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Adam Roben (:aroben)
Comment 1
2011-02-03 06:36:43 PST
<
rdar://problem/8953558
>
Per Arne Vollan
Comment 2
2016-06-21 01:47:30 PDT
Created
attachment 281724
[details]
Patch
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
Committed
r202326
: <
https://trac.webkit.org/changeset/202326
>
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.
Top of Page
Format For Printing
XML
Clone This Bug