Bug 46186 - [Qt] window.close() doesn't work in qt
Summary: [Qt] window.close() doesn't work in qt
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P3 Normal
Assignee: qi
URL:
Keywords: Qt
Depends on:
Blocks: 31552
  Show dependency treegraph
 
Reported: 2010-09-21 07:24 PDT by qi
Modified: 2010-10-15 12:55 PDT (History)
7 users (show)

See Also:


Attachments
patch (1.86 KB, patch)
2010-09-21 07:40 PDT, qi
tonikitoo: review-
Details | Formatted Diff | Diff
patch2 (2.23 KB, patch)
2010-09-21 08:35 PDT, qi
vestbo: review-
commit-queue: commit-queue-
Details | Formatted Diff | Diff
patch3 (2.62 KB, patch)
2010-10-15 08:33 PDT, qi
kling: review-
Details | Formatted Diff | Diff
patch4 (2.64 KB, patch)
2010-10-15 08:48 PDT, qi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description qi 2010-09-21 07:24:53 PDT
QWebSettings doesn't provide attribute to trigger "setAllowScriptsToCloseWindows" in settings.h.

See bug http://bugs.nokia-boston.com/bugzilla/process_bug.cgi,    #4731
Comment 1 qi 2010-09-21 07:40:15 PDT
Created attachment 68240 [details]
patch

Introduce a new attribute in QWebSetting to let application can trigger "setAllowScriptsToCloseWindow" to make window.close() works.
Comment 2 Antonio Gomes 2010-09-21 08:08:29 PDT
Comment on attachment 68240 [details]
patch

I need documentation at least (and of course an API review).
Comment 3 qi 2010-09-21 08:11:43 PDT
What documentation I need to prepare, can you give me a detail instruction? This is first time I try to modify a API.
Comment 4 Antonio Gomes 2010-09-21 08:14:06 PDT
Sure. For example, for all other members of the same enum, see their respective documentation in qwebsettings.cpp
Comment 5 qi 2010-09-21 08:35:07 PDT
Created attachment 68242 [details]
patch2

Update the documentation in QWebSettings to create a new patch.

Thanks.
Comment 6 Adam Barth 2010-09-26 22:24:48 PDT
Comment on attachment 68242 [details]
patch2

ok
Comment 7 WebKit Commit Bot 2010-09-27 06:49:54 PDT
Comment on attachment 68242 [details]
patch2

Rejecting patch 68242 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'apply-attachment', '--force-clean', '--non-interactive', '--quiet', 68242]" exit_code: 2
Cleaning working directory
Updating working directory
Logging in as commit-queue@webkit.org...
Fetching: https://bugs.webkit.org/attachment.cgi?id=68242&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=46186&ctype=xml
Processing 1 patch from 1 bug.
Processing patch 68242 from bug 46186.
Failed to run "[u'/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/4070132
Comment 8 Tor Arne Vestbø 2010-09-27 06:52:37 PDT
Comment on attachment 68242 [details]
patch2

This should follow the naming of JavascriptCanOpenWindows, especiallt if the functionality is symetric with regards to user-initiated gestures, etc
Comment 9 Jocelyn Turcotte 2010-09-28 02:03:09 PDT
Result of investigations following the API review:
JavascriptCanOpenWindows affects only non-user-initiated window.open tentatives.

Currently in the implementation, Settings::allowScriptsToCloseWindows() allows the window to be closed only if the window was opened by the script and if the user didn't navigate/load any other page on this window.

I think that these two behaviors are related enough to use the symetrical enum name JavascriptCanCloseWindows to wrap Settings::setAllowScriptsToCloseWindows().
Comment 10 qi 2010-10-15 08:33:19 PDT
Created attachment 70868 [details]
patch3

Created a new patch based on Jocelyn's comments.
Comment 11 Andreas Kling 2010-10-15 08:39:34 PDT
Comment on attachment 70868 [details]
patch3

View in context: https://bugs.webkit.org/attachment.cgi?id=70868&action=review

> WebKit/qt/Api/qwebsettings.h:58
> +        JavascriptCanCloseWindows,

This will break binary compatibility. Add it after the existing enum values instead.
Comment 12 qi 2010-10-15 08:48:38 PDT
Created attachment 70870 [details]
patch4

new  patch based on Andreas Kling's comments.
Comment 13 Andreas Kling 2010-10-15 09:13:32 PDT
Comment on attachment 70870 [details]
patch4

Nice, r=me
Comment 14 WebKit Commit Bot 2010-10-15 09:27:36 PDT
Comment on attachment 70870 [details]
patch4

Clearing flags on attachment: 70870

Committed r69856: <http://trac.webkit.org/changeset/69856>
Comment 15 WebKit Commit Bot 2010-10-15 09:27:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Ademar Reis 2010-10-15 12:55:40 PDT
Revision r69856 cherry-picked into qtwebkit-2.1 with commit 42e90ac <http://gitorious.org/webkit/qtwebkit/commit/42e90ac>