Summary: | window.open scripts do not trigger newWindowDelegate callback. | ||
---|---|---|---|
Product: | WebKit | Reporter: | Dirk Theisen <d.theisen> |
Component: | WebKit API | Assignee: | Nobody <webkit-unassigned> |
Status: | UNCONFIRMED --- | ||
Severity: | Normal | CC: | abarth, buildbot, dbates, dglazkov, gustavo, japhet, joone.hur, joone.hur, lucas.de.marchi, mathstuf, nicolas, senko, webkit.review.bot, xan.lopez |
Priority: | P2 | ||
Version: | 525.x (Safari 3.2) | ||
Hardware: | Mac (Intel) | ||
OS: | OS X 10.5 | ||
URL: | http://code.google.com/p/pandoraboy/source/browse/tags/pandoraboy_0_3/Controller.m#156 | ||
Attachments: |
Description
Dirk Theisen
2009-01-20 07:54:50 PST
In WebKitGtk port, "new-window-policy-decision-requested" signal isn't also triggered when using the window.open call. It seems like a bug. (In reply to comment #1) > In WebKitGtk port, "new-window-policy-decision-requested" signal isn't also triggered when using the window.open call. > It seems like a bug. For javascript it will call ChromeClient::createWindow() instead. In EFL port I've implemented both ChromeClientEfl::createWindow() and ChromeClient::decidePolicyForNewWindowAction() by calling our ewk_view_window_create() with different parameters in order to be able to differentiate between javascript ones and link ones. Unfortunately, sending upstream this code is being blocked by other bugs that are not reviewed yet. As soon as they are r+ cq+ I'll send them. Created attachment 70783 [details]
Simple test case, that show new-window-policy-decision-requested is not emited on JS window.open()
For the record, this has something to do with a fixme in JSDOMWindowCustom.cpp line 678: // FIXME: It's much better for client API if a new window starts with a URL, here where we // know what URL we are going to open. Unfortunately, this code passes the empty string // for the URL, but there's a reason for that. Before loading we have to set up the opener, // openedByDOM, and dialogArguments values. Also, to decide whether to use the URL we currently // do an allowsAccessFrom call using the window we create, which can't be done before creating it. // We'd have to resolve all those issues to pass the URL instead of "". After more analyses, I concluded that this missing newWindowDelegate callback hides a bigger problem. The problem is that checking the window policy is an asynchronous call but window.open() calls are handled synchronously. From my point of view, if new window is denied by the client, window.open() should return undefined. This cannot be achieved with current API. As exposed by Lucas, one solution is to check the policy when ChromeClient::createWindow() is called. I'm currently writing a patch for the Gtk platform. There is other issues that will need fixing before it actually work. 1. The FrameLoadRequest passed to createWindow() always contains an empty URL (""). 2. There is only NavigationTypeOther that seems to fit, maybe a NavigationTypeJavascript would be better. (optional I guess) (In reply to comment #5) > I'm currently writing a patch for the Gtk platform. There is other issues that > will need fixing before it actually work. Sorry for replying to my own comment, but it's not possible to implement the check in Gtk's ChromeClient::createWindow(). The reason is that Gtk port allow to queue the decisions for later. I've compared window.open() with other similar API like history.go() and found that they all return void which make it simple to delay their execution. For window.open(), the return value is a JavaScript Window object, which is currently bound at creation time to the native frame/page object. This function is implemented as one synchronous native call, and thus cannot be interrupted and resumed later. Base on these facts, I think a potential solution would be to delay the creation of native frame until the navigation policy decision has been taken. The side effect is that call to window.open() will return a JavaScript window even if the window creation is denied. Is this acceptable ? After more work on this I've finally managed to implement a solution that I will soon attach. The solution I've found is to allow creating page without making it visible (see Page::chrome().show() and WebCore::createWindow()). This enables two things: 1) you can actually request for location change before displaying the window, which allow making popup blocking decision/triage based on URI before the window is displayed and 2) for synchronous scripted popup (e.g. window.open()), you can simply create an hidden window, fire NewWindowPolicy on initial location change and show only if decision was positive (close if not). At first I was concern about show() call being flipped with ChangeLocationPolicy, but I found that there exist cases where we already get this order. Stay tunned ... Created attachment 73415 [details]
[PATCH 1/4] Allow creating hidden window
Allow creating hidden window that can be destroyed if it's creation was
refused by NewWindowPolicy. This is required to fullfill the synchronous
requirement of JS call to window.open().
Created attachment 73416 [details]
[PATCH 2/4] Show window only after initial location request
To fix this bug we enabled creating hidden windows. This patch takes
benifit of this new feature and queue location change before calling
Page::chrome()->show(). This mean URI can be considered sooner by
non-destructive popup blocker like the one in Chrome.
Created attachment 73417 [details]
[PATCH 3/4] Don't duplicate code from FrameTree::find()
In JSDOMWindow::open() we duplicate some of the code from
FrameTree::find(). This patch also removes one call to FrameTree::find().
This code cleanup is attached to this bug since it ensure that createWindow
is actually called to create new window, reducing side effect risk.
Created attachment 73418 [details]
[PATCH 4/4] Trigger newWindowDelegate when window.open() is called
This is a bit tricky since newWindowDelegate() is normally triggered before
creating the actual window. But since window.open() returns a window, we have
no choice but to create it. In this patch, we play with the window visibility,
creating the window and launching the newWindowDelegate() through
changeLocation() and a special optional parameter. The result is that the
window will only be showed if the policy is accepted, and will be closed
Comment on attachment 73418 [details]
[PATCH 4/4] Trigger newWindowDelegate when window.open() is called
Note: style check fails because this patch depends on cleanup patch to apply cleanly.
Attachment 73416 [details] did not build on qt: Build output: http://queues.webkit.org/results/5528067 Attachment 73416 [details] did not build on gtk: Build output: http://queues.webkit.org/results/5496057 Attachment 73416 [details] did not build on win: Build output: http://queues.webkit.org/results/5484069 Ok, tell me what is the procedure here, because what the automated build does wont work, you can't try all patches separately, e.g. 2 depends on 1, and 4 depends on 3 and 1. Maybe I should have requested review for 1 and 3 first, and waited for them to be upstream ? thanks, and sorry for noise. > Ok, tell me what is the procedure here, because what the automated build does wont work, you can't try all patches separately, e.g. 2 depends on 1, and 4 depends on 3 and 1. Maybe I should have requested review for 1 and 3 first, and waited for them to be upstream ?
The EWS doesn't handle dependent patch sequences. You're welcome to add support for them if you like, but it doesn't come up that often. Sorry for the bugmail spam.
Attachment 73416 [details] did not build on chromium: Build output: http://queues.webkit.org/results/5530077 Comment on attachment 73418 [details]
[PATCH 4/4] Trigger newWindowDelegate when window.open() is called
It's too soon to request review, need to wait for patch 1 and 3.
Comment on attachment 73416 [details]
[PATCH 2/4] Show window only after initial location request
Too soon to request view, need to wait for patch 1.
Created attachment 75107 [details]
Patch
Attachment 75107 [details] did not build on chromium: Build output: http://queues.webkit.org/results/6459013 (In reply to comment #22) > Attachment 75107 [details] did not build on chromium: > Build output: http://queues.webkit.org/results/6459013 It seems that I broke V8 build, I'll fix soon, see log: In file included from WebCore/bindings/v8/specialization/V8BindingDOMWindow.h:34, from WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:53: WebCore/bindings/generic/BindingDOMWindow.h: In static member function ‘static WebCore::Frame* WebCore::BindingDOMWindow<Binding>::createWindow(WebCore::State<Binding>*, WebCore::Frame*, WebCore::Frame*, WebCore::Frame*, const WTF::String&, const WTF::String&, const WebCore::WindowFeatures&, typename Binding::Value)’: WebCore/bindings/generic/BindingDOMWindow.h:128: error: ‘create’ was not declared in this scope Comment on attachment 75107 [details]
Patch
This patch has an incorrect ChangeLog, does not contain tests, and does not compile.
(In reply to comment #24) > (From update of attachment 75107 [details]) > This patch has an incorrect ChangeLog Please specify so I can fix. > does not contain tests, As explain in the log, this is not visible from HTML side. As every port wraps this differently, I cannot come up with a test that works for all ports, at best I can provide a GTK port, base on the test case originally attached. > and does not compile. This can be fixed, please leave the bug open, I need to rebase against the new folder organization, and I'm doing that on free time, I have no more particular interest on my side anymore, aside correctness. If anyone needs it sooner, well take over the patch, the implementation is correct. *** Bug 34715 has been marked as a duplicate of this bug. *** |