Bug 29847

Summary: [Qt] Add support for automatically creating new windows in QWebView
Product: WebKit Reporter: Tor Arne Vestbø <vestbo>
Component: WebKit QtAssignee: Antonio Gomes <tonikitoo>
Status: RESOLVED FIXED    
Severity: Enhancement CC: ademar, hausmann, kenneth, kent.hansen, tonikitoo
Priority: P2 Keywords: Qt
Version: 528+ (Nightly build)   
Hardware: Other   
OS: All   
Bug Depends on:    
Bug Blocks: 35784    
Attachments:
Description Flags
patch v1 - fix the documentation
hausmann: review-
(commit r66217, r=hausmann)patch v2 - addressed simon's review none

Description Tor Arne Vestbø 2009-09-29 05:30:01 PDT
This bug report originated from issue QTBUG-2162
http://bugreports.qt.nokia.com/browse/QTBUG-2162

--- Description ---

QWebView QWebPage - mainframe()-evaluateJavaScript("window.open('url')") doesn't open new browser window.

When calling evaluateJavaScript("window.open('url')) on a mainFrame() or just load a  webpage containing <script>window.open('http://www.trolltech.com/');</script> should open a new window containing the webpage.

This is due to QWebView::createWindow() not creating a new window by default. We should consider adding a property to QWebView that enables the default implementation to create new QWebView instances, something along the lines of a 'bool autoCreateWindows' property.


- How to reproduce:
1. Build and compile the code.
2. Start the application

- Expected result:
Two windows should appear. One containing the Trolltech webpage and one saying foo... ect.
- Actual result:
Only the last page containing:

foo...before calling open

foo...after calling open. A new window should have popped up showing the Trolltech webpage.


Test case main.cpp for above
=============================
class MainWidget : public QWidget {
 Q_OBJECT
public:
 MainWidget();
private:
 QWebPage *page;
 QWebView *view;
};
MainWidget::MainWidget(){
 view = new QWebView(this);
 page = new QWebPage(this);
 QUrl url("test.html");

 page->settings()->setAttribute(QWebSettings::JavascriptEnabled,true); // Make sure JavaScript is enabled
 page->mainFrame()->load(url);            // Load the test case 
 view->setPage(page);

 QVariant variant = page->mainFrame()->evaluateJavaScript("window.open('http://www.trolltech.com/');");
 qDebug() << variant.toString();            // This doesn't return anything either.

 QVBoxLayout *layout = new QVBoxLayout;
 layout->addWidget(view);
 setLayout(layout);
}

int main(int argc, char *argv[]){
 QApplication app(argc, argv);
 MainWidget mainWid;
 mainWid.show();
 return app.exec();
}

--- Comments ---

Does this still occur when setting the property QWebSettings::JavascriptCanOpenWindows ?
Comment 1 Simon Hausmann 2009-11-22 10:08:49 PST
I disagree that by default we should create new windows. I think this is part of the application integrate that needs to be taken care of by the application developer. Allowing new top-level  windows coming up by default (that can be controlled via JavaScript) sounds like the wrong default to me from a security POV.
Comment 2 Antonio Gomes 2009-11-23 13:01:32 PST
> --- Comments ---
> 
> Does this still occur when setting the property
> QWebSettings::JavascriptCanOpenWindows ?

I just tested the sample application pasted here, and setting the property has no effect (and it ready shouldn't).

so one who wants to make JavaScript to open new windows by default should set the 'JavascriptCanOpenWindows' to 'true' AND reimplement QWebPage's createWindow method ...

it should get mentioned in DOCS in a clear way (if it is not yet there).

(In reply to comment #1)
> I disagree that by default we should create new windows. I think this is part
> of the application integrate that needs to be taken care of by the application
> developer. Allowing new top-level  windows coming up by default (that can be
> controlled via JavaScript) sounds like the wrong default to me from a security
> POV.

INVALID or WONTFIX then, simon ?
Comment 3 Simon Hausmann 2009-11-24 07:46:10 PST
(In reply to comment #2)
> > --- Comments ---
> > 
> > Does this still occur when setting the property
> > QWebSettings::JavascriptCanOpenWindows ?
> 
> I just tested the sample application pasted here, and setting the property has
> no effect (and it ready shouldn't).
> 
> so one who wants to make JavaScript to open new windows by default should set
> the 'JavascriptCanOpenWindows' to 'true' AND reimplement QWebPage's
> createWindow method ...
> 
> it should get mentioned in DOCS in a clear way (if it is not yet there).
> 
> (In reply to comment #1)
> > I disagree that by default we should create new windows. I think this is part
> > of the application integrate that needs to be taken care of by the application
> > developer. Allowing new top-level  windows coming up by default (that can be
> > controlled via JavaScript) sounds like the wrong default to me from a security
> > POV.
> 
> INVALID or WONTFIX then, simon ?

I'd say WONTFIX, but ideally RESOLVED/FIXED with a patch to the documentation ;-)
Comment 4 Kent Hansen 2010-03-12 08:22:51 PST
(In reply to comment #1)
> I disagree that by default we should create new windows. I think this is part
> of the application integrate that needs to be taken care of by the application
> developer. Allowing new top-level  windows coming up by default (that can be
> controlled via JavaScript) sounds like the wrong default to me from a security
> POV.

Creating new windows by default is not what the description suggests.
It suggests adding a property that the default implementation (QWebView) can check. This property (e.g. autoCreateWindows) would be "false" by default, preserving the existing behavior.

But regardless of whether this is implemented, the doc for QWebView::createWindow() should refer to QWebSettings::JavascriptCanOpenWindows (indeed, the testcase didn't enable this attribute, so createWindow() wasn't called regardless), and say what the default createWindow() implementation does (e.g. "always returns 0" or "depends on the autoCreateWindows property").

I'm in favor of just doing the doc update like Simon suggested.
Comment 5 Antonio Gomes 2010-08-26 22:44:31 PDT
> I'm in favor of just doing the doc update like Simon suggested.

Right. I will try something shortly ...
Comment 6 Antonio Gomes 2010-08-26 22:54:14 PDT
Created attachment 65678 [details]
patch v1 - fix the documentation

If it is ok, it should be cherry-picked into both qtwebkit2.0 and qtwebkit2.1 branches.

Kenneth, Simon, Kent, what are your takes on it?
Comment 7 Simon Hausmann 2010-08-26 23:49:11 PDT
Comment on attachment 65678 [details]
patch v1 - fix the documentation

I'm okay with that, please make this bug block the release bugs.

However there's a typo in this patch: QWebSettings::setJavaScriptCanOpenWindowsAutomatically doesn't exist and should be setAttribute(...) :)
Comment 8 Antonio Gomes 2010-08-27 05:23:47 PDT
Created attachment 65701 [details]
(commit r66217, r=hausmann)patch v2 - addressed simon's review
Comment 9 Simon Hausmann 2010-08-27 06:35:26 PDT
Comment on attachment 65701 [details]
(commit r66217, r=hausmann)patch v2 - addressed simon's review

WebKit/qt/Api/qwebview.cpp:960
 +      \sa QWebPage::createWindow(), QWebSettings::setJavaScriptCanOpenWindowsAutomatically()
There's one left :)

Please fix before landing, otherwise r=me :)
Comment 10 Antonio Gomes 2010-08-27 06:47:01 PDT
Comment on attachment 65701 [details]
(commit r66217, r=hausmann)patch v2 - addressed simon's review

Clearing flags on attachment 65701 [details]

Committed r66217: <http://trac.webkit.org/changeset/66217>
Comment 11 Antonio Gomes 2010-08-27 06:47:37 PDT
(In reply to comment #9)
> (From update of attachment 65701 [details])
> WebKit/qt/Api/qwebview.cpp:960
>  +      \sa QWebPage::createWindow(), QWebSettings::setJavaScriptCanOpenWindowsAutomatically()
> There's one left :)
> 
> Please fix before landing, otherwise r=me :)

Done before landing... Thanks.
Comment 12 Ademar Reis 2010-08-27 07:01:52 PDT
Revision r66217 cherry-picked into qtwebkit-2.1 with commit de555d5dc3ef9ad493ba5f8b874efacfc440b1dd
Comment 13 Kenneth Rohde Christiansen 2010-08-27 07:02:46 PDT
"of QWebSettings to true in order to get it called."

This sounds a bit bad.

"in order for it to get called", would be better
Comment 14 Antonio Gomes 2010-08-27 07:16:41 PDT
(In reply to comment #13)
> "of QWebSettings to true in order to get it called."
> 
> This sounds a bit bad.
> 
> "in order for it to get called", would be better

As per kenneth suggestion, changed in r66221.

Committed r66221: <http://trac.webkit.org/changeset/66217>
Comment 15 Ademar Reis 2010-08-27 07:44:52 PDT
Revision r66221 cherry-picked into qtwebkit-2.1 with commit c61bb8fd4eec94c5ffa7107bb48f0f360ae3c2d3