Summary: | [Qt] Add support for automatically creating new windows in QWebView | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tor Arne Vestbø <vestbo> | ||||||
Component: | WebKit Qt | Assignee: | 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
Tor Arne Vestbø
2009-09-29 05:30:01 PDT
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. > --- 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 ? (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 ;-) (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. > I'm in favor of just doing the doc update like Simon suggested.
Right. I will try something shortly ...
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 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(...) :)
Created attachment 65701 [details] (commit r66217, r=hausmann)patch v2 - addressed simon's review 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 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> (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. Revision r66217 cherry-picked into qtwebkit-2.1 with commit de555d5dc3ef9ad493ba5f8b874efacfc440b1dd "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 (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> Revision r66221 cherry-picked into qtwebkit-2.1 with commit c61bb8fd4eec94c5ffa7107bb48f0f360ae3c2d3 |