RESOLVED INVALID 50550
[Qt] Add a new WebWindowType (WebDialog)... 
https://bugs.webkit.org/show_bug.cgi?id=50550
Summary [Qt] Add a new WebWindowType (WebDialog)...
Dawit A.
Reported 2010-12-05 23:24:13 PST
For some reason the WebWindowType parameter of QWebPage::createWindow is always set to WebBrowserWindow even when a dialog is requested as shown in the following HTML example: <html> <head> <script type="text/javascript"> function createWindow() { window.open("http://webkit.org", "newwindow", "dialog=yes"); } </script> </head> <body> <a href="javascript:createWindow()">click me</a> </body> </html>
Attachments
Unit test patch I (1.54 KB, patch)
2010-12-05 23:28 PST, Dawit A. 		no flags
DetailsFormatted DiffDiff
Proposed patch... (3.22 KB, patch)
2010-12-06 17:13 PST, Dawit A. 		no flags
DetailsFormatted DiffDiff
Proposed patch v2 (3.40 KB, patch)
2010-12-06 20:34 PST, Dawit A. 		no flags
DetailsFormatted DiffDiff
Proposed patch v3 (7.15 KB, patch)
2010-12-08 11:25 PST, Dawit A. 		no flags
DetailsFormatted DiffDiff
Proposed patch v4 (7.12 KB, patch)
2010-12-09 09:32 PST, Dawit A. 		no flags
DetailsFormatted DiffDiff
Proposed patch v5 (7.70 KB, patch)
2010-12-10 12:29 PST, Dawit A. 		no flags
DetailsFormatted DiffDiff
Proposed patch v6 (8.32 KB, patch)
2010-12-10 12:44 PST, Dawit A. 		no flags
DetailsFormatted DiffDiff
Proposed patch v7 (8.53 KB, patch)
2010-12-10 13:57 PST, Dawit A. 		no flags
DetailsFormatted DiffDiff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.
Dawit A.
Comment 1 2010-12-05 23:28:45 PST
Created attachment 75650 [details] Unit test patch I Patch against tst_qwebpage.cpp to demonstrate the failure of QWebPage::createWindow to receive proper WebWindowType information...
Benjamin Poulain
Comment 2 2010-12-06 07:32:31 PST
I set this to P1, API break.
Dawit A.
Comment 3 2010-12-06 17:13:38 PST
Created attachment 75758 [details] Proposed patch... Here is a patch that fixes the problem. The issue is not QtWebKit specific, but rather webkit specific. No idea why the bug does not show up in chromium though. Perhaps it is because they use their own javascript engine v8...
WebKit Review Bot
Comment 4 2010-12-06 18:09:12 PST
Attachment 75758 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'WebCore/ChangeLog', u'WebCore/page/WindowFeatures.cpp', u'WebKit/qt/ChangeLog', u'WebKit/qt/tests/qwebpage/tst_qwebpage.cpp']" exit_code: 1 WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:353: Place brace on its own line for function definitions. [whitespace/braces] [4] Total errors found: 1 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Dawit A.
Comment 5 2010-12-06 20:34:24 PST
Created attachment 75780 [details] Proposed patch v2 Fixed style failure...
Antonio Gomes
Comment 6 2010-12-06 22:17:35 PST
Comment on attachment 75780 [details] Proposed patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=75780&action=review lgtm > WebCore/ChangeLog:8 > + No new tests. (OOPS!) You have an auto test. Either delete this line OR mention why it can not be tested.
Benjamin Poulain
Comment 7 2010-12-07 04:11:50 PST
Comment on attachment 75780 [details] Proposed patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=75780&action=review >> WebCore/ChangeLog:8 >> + No new tests. (OOPS!) > > You have an auto test. Either delete this line OR mention why it can not be tested. I am in favor of extending the controller to have a layout test. It is too important to be limited to Qt autotests only. > WebCore/page/WindowFeatures.cpp:159 > + else if (keyString == "dialog") > + dialog = value; In FrameLoader, there is an assert against this: ASSERT(!features.dialog || request.frameName().isEmpty()); This could never been hit before, could you check if it is still the case? > WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:359 > + virtual QWebPage* createWindow(WebWindowType type) > + { > QWebPage* page = new TestPage(this); > createdWindows.append(page); > + windowType = type; > return page; This is missleading in my opinion. You create a new page, but set the windowType of the current page, not the page returned by the call. > WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:@ > void tst_QWebPage::acceptNavigationRequestWithNewWindow() I would prefer a new test.
WebKit Review Bot
Comment 8 2010-12-07 08:31:51 PST
Attachment 75780 [details] did not pass style-queue: Failed to run "[u'git', u'reset', u'--hard', u'refs/remotes/trunk']" exit_code: 128 error: Could not write new index file. fatal: Could not reset index file to revision 'refs/remotes/trunk'. If any of these errors are false positives, please file a bug against check-webkit-style.
Dawit A.
Comment 9 2010-12-07 15:18:26 PST
(In reply to comment #7) > (From update of attachment 75780 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=75780&action=review > > >> WebCore/ChangeLog:8 > >> + No new tests. (OOPS!) > > > > You have an auto test. Either delete this line OR mention why it can not be tested. > > I am in favor of extending the controller to have a layout test. It is too important to be limited to Qt autotests only. > > > WebCore/page/WindowFeatures.cpp:159 > > + else if (keyString == "dialog") > > + dialog = value; > > In FrameLoader, there is an assert against this: > ASSERT(!features.dialog || request.frameName().isEmpty()); > This could never been hit before, could you check if it is still the case? hmm... unfortunately that will be hit in debug mode. :( I am unsure why that check is there. Perhaps it assumes "dialog=yes" means a "modal dialog" ? Not sure. Anyhow, the qtwebkit implementation does not make sense either. Not only does the type property never get set to WebModalDialog, but also the suggestion in QWebPage::createWindow's documentation to set the WindowModality to ApplicationModal is wrong. Allowing any arbitrary untrusted script to set a modal dialog is a recipe for disaster. See the section on the "modal" property at https://developer.mozilla.org/en/DOM/window.open. That flag instead should have simply been labeled WebDialog instead... > > WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:359 > > + virtual QWebPage* createWindow(WebWindowType type) > > + { > > QWebPage* page = new TestPage(this); > > createdWindows.append(page); > > + windowType = type; > > return page; > > This is missleading in my opinion. You create a new page, but set the windowType of the current page, not the page returned by the call. > > > WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:@ > > void tst_QWebPage::acceptNavigationRequestWithNewWindow() > > I would prefer a new test. I will do a new test once I finish implementing this correctly. After much testing and playing around with other browsers, I have come to the conclusion that the patch I provided needs more work. Specifically, as already mentioned above the would be qtwebkit implementation for a dialog like window, though currently non-functional, has security risks associated with it. Further more, both of the other browsers I tested seem to create a new window if any property is present, even a non-valid one!! For example "window.open("about:blank", "", "foobar");" will result in a new window instead of a new tab. Only when no properties are specified do they resort to creating a tab or rather a browser window...
Dawit A.
Comment 10 2010-12-08 11:25:12 PST
Created attachment 75933 [details] Proposed patch v3 First disregard my ignorant comment regarding the purpose of the WebModalDialog type. I now see that its purpose is to handle the "window.showModalDialog" Javascript calls and not the "modal" feature of the "window.open" ones. Anyhow, here is the proper implementation I promised with the following new modifications: - Completely abondon the changes made in the WindowFeatures class as they are not necessary. - Add a new WebWindowType enum, WebDialog, to make reimplementators of QWebPage::createWindow aware of the kind of window they should create when that function is invoked. In this case WebDialog is used to convey the fact that the window created MUST be a completely new window and not just another tab in the current window as might be the case if type was set to WebBrowserWindow. This makes it possible to provide implementation consistent with other browsers. - For window.open javascript calles, the WebWindowType parameter of QWebPage::createWindow will be set to WebWindowBrowser only when the windows feature portion the afformentioned javascript call is empty. Again, this is consistent with the implementation in other browsers.
Dawit A.
Comment 11 2010-12-09 09:25:14 PST
This is not really a P1, but rather an ehancement request. See comment #10. Changed title accordingly...
Dawit A.
Comment 12 2010-12-09 09:32:12 PST
Created attachment 76074 [details] Proposed patch v4 Updated changelog entry...
Benjamin Poulain
Comment 13 2010-12-09 09:35:57 PST
(In reply to comment #11) > This is not really a P1, but rather an ehancement request. See comment #10. Changed title accordingly... Good point, thanks for updating that. I haven't really looked at the patch but I think you should update the documentation of both WebModalDialog and WebDialog. Mentionning exactly when each can be called, and what feature is expected from the window created.
Dawit A.
Comment 14 2010-12-10 12:29:34 PST
Created attachment 76236 [details] Proposed patch v5 Updated QWebPage::createWindow's documentation with information about the new WebDialog enum.
Dawit A.
Comment 15 2010-12-10 12:44:01 PST
Created attachment 76241 [details] Proposed patch v6 Updated the WebWindowType enum documentation...
Early Warning System Bot
Comment 16 2010-12-10 13:17:08 PST
Attachment 76241 [details] did not build on qt: Build output: http://queues.webkit.org/results/6939029
Dawit A.
Comment 17 2010-12-10 13:57:40 PST
Created attachment 76253 [details] Proposed patch v7 Fix compilation of tst_qwebpage.cpp...
Dawit A.
Comment 18 2010-12-11 18:28:39 PST
Comment on attachment 76253 [details] Proposed patch v7 Rescinded my own request and patch... Found out that adding a new flag does not help those for whom this patch was intended. Namely people that are attempting to adapt one API into another.
Note You need to log in before you can comment on or make changes to this bug.