Created attachment 72725 [details] HTML document that can be use to demonstrate the bug When a link with attribute target="myTarget" is clicked twice, two new windows are being opened. This bug is cause by all new pages being created in there own PageGroup. Until a WebKitGTK+ provide an API for PageGroup (see https://bugs.webkit.org/show_bug.cgi?id=16977) we should fix this bug by using a global page group name for all pages.
Created attachment 72727 [details] Set global PageGroup name to all created pages
Sorry, wrong reference for WebKitWebGroup API, see https://bugs.webkit.org/show_bug.cgi?id=32789
Some people ask on IRC if that was fixing some tests (or required additional tests). This bug is covered buy LayoutTests/http/tests/navigation/target-frame-from-window.html and works in the GTK DRT but not in GtkLauncher. The DRT uses private API function webkit_web_view_set_group_name() to set a constant group name to every newly created View/Page. The propose solution is to adopt a default constant group name to all newly created page. While the best final solution would be to give control to user over group names (probably useful for Desktop widgets and/or privacy mode), we should still be able to get correct result with minimum effort, which is why I'm proposing this one liner fix.
Comment on attachment 72727 [details] Set global PageGroup name to all created pages r-, because, if I recall correctly you are working on a new version of this patch which also unskips a test.
(In reply to comment #4) > (From update of attachment 72727 [details]) > r-, because, if I recall correctly you are working on a new version of this patch which also unskips a test. Not exactly. What I was trying to say in previous message is that LayoutTests/http/tests/navigation/target-frame-from-window.html is the test that covers that bug (no need to write a new one). What I found is that the test pass in DumpRenderTree but fails in any other WebKitGTK+ application. The reason is because DRT uses a non-public API to do just like my patch does locally, which is to use same PageGroup name for all pages. Please reconsider reviewing this patch.
One way we can ensure adequate testing here remove webkit_web_view_set_group_name(view, "org.webkit.gtk.DumpRenderTree"); from the DRT and the webkit_web_view_set_group_name private API.
Created attachment 72951 [details] Set global PageGroup name to all created pages
Created attachment 72955 [details] Set global PageGroup name to all created pages This one removes the set_group_name() call in DRT and removed the uneeded extern declaration for the private set_group_name() method (not sure if it's used anywhere else now).
Comment on attachment 72955 [details] Set global PageGroup name to all created pages View in context: https://bugs.webkit.org/attachment.cgi?id=72955&action=review Great! Is it possible to go one step further and remove webkit_web_view_set_group_name from webkitwebview.cpp entirely, since, if I understand correctly, it is now dead code. > WebKit/gtk/webkit/webkitwebview.cpp:3250 > + priv->corePage->setGroupName("WebKitGTK"); I think we should have a comment here explaining why this is necessary.
Created attachment 73097 [details] Set global PageGroup name to all created pages As requested, I added a comment and removed the set_group_name() method from private API (and all reference to it).
Comment on attachment 73097 [details] Set global PageGroup name to all created pages Great!
Comment on attachment 73097 [details] Set global PageGroup name to all created pages Clearing flags on attachment: 73097 Committed r71466: <http://trac.webkit.org/changeset/71466>
All reviewed patches have been landed. Closing bug.
Reverted r71466 for reason: It Committed r71510: <http://trac.webkit.org/changeset/71510>
(In reply to comment #14) > Reverted r71466 for reason: > Committed r71510: <http://trac.webkit.org/changeset/71510> It broke layout tests on all GTK bots. After an inspector test, run-webkit-tests timed out: inspector ................... command timed out: 1200 seconds without output, killing pid 12840 process killed by signal 9 program finished with exit code -1
(In reply to comment #15) > (In reply to comment #14) > > Reverted r71466 for reason: > > Committed r71510: <http://trac.webkit.org/changeset/71510> > > It broke layout tests on all GTK bots. > After an inspector test, run-webkit-tests timed out: > > inspector ................... > command timed out: 1200 seconds without output, killing pid 12840 > process killed by signal 9 > program finished with exit code -1 Please provide additionnal information, this patch does not change the code bahaviour, it just moves the group name setting somewhere else. It most likely the wrong patch being reverted, or simply a luck that you don't get deadlock without it. Thanks.
(In reply to comment #16) > Please provide additionnal information, this patch does not change the code bahaviour, it just moves the group name setting somewhere else. It most likely the wrong patch being reverted, or simply a luck that you don't get deadlock without it. I don't have any additional information, I only saw that GTK bots were sick 2 days ago. I didn't revert wrong patch, it was the culprit. Maybe a hidden bug was revealed by this patch, I don't know. But I think well functioning buildbots are more important than one patch.
Created attachment 73248 [details] Set global PageGroup name to all created pages Found that when the inspector as same PageGroup as the page being debugged we may deadlock. In this updated patch I reset the Inspector page group to empty string, which result in creating a private group for the inspector. I also updated the changelogs.
Comment on attachment 73248 [details] Set global PageGroup name to all created pages Okay. Let's try this again.
Comment on attachment 73248 [details] Set global PageGroup name to all created pages Clearing flags on attachment: 73248 Committed r71604: <http://trac.webkit.org/changeset/71604>