This bug opened because portions of an issue reported downstream, https://bugs.kde.org/show_bug.cgi?id=210291, can be clearly duplicated using QtTestBrowser from QtWebKit 2.1. The issue that is QtWebKit specific from that report as follows: 1.) Go to http://www.dpd.at/ 2.) Enter 01635001689282H (non working tracking # from downstream report, comment #6). 3.) Click on the '>' button and see what happens in QtTestBrowser... Expected behavior: A single new window with error message should be shown as verified using both Chromium 6.0.472.53 (0) and Firefox 3.6.9. Actual behavior: Two new windows/tabs are created when using QtWebKit based browsers. One shows "about:blank" and the other shows an error message. Actually with QtTestBrowser the second page is completely blank. What is even worse is the fact that if you close these two new windows and repeat the process again, only the "about:blank" window is created. I guess the question is why does repeating the process result in these two incorrect behaviors ?
> Actual behavior: > Two new windows/tabs are created when using QtWebKit based browsers. One shows "about:blank" and the other shows an error message. Actually with QtTestBrowser the second page is completely blank. After the MIME sniffing (bug 46968) the second screen now shows the expected content in QtTestBrowser. Other issues continue as they were. > What is even worse is the fact that if you close these two new windows and repeat the process again, only the "about:blank" window is created. I guess the question is why does repeating the process result in these two incorrect behaviors ?
The timer is firing the wrong frame for the new window. Investigating
Pages needs to be set at the same group name, to avoid problems with popup windows and link coloration. Will be used only one page group name, making it similar as others port's are doing
Created attachment 95038 [details] Patch
Comment on attachment 95038 [details] Patch no test? :)
Comment on attachment 95038 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95038&action=review r- because of the string literal. With this change all pages are automatically in the same page group, which to some extend this bug is about. This is a behavioural change that we should really verify with a test. I see that the Gtk+ port seems to behave the same, but I don't see the same behaviour in the other ports directly (as they have an API). What's the pattern their callers use? What's the safest default for us? > Source/WebKit/qt/Api/qwebpage.cpp:159 > +const char* QWebPagePrivate::pageGroupName = "QtWebKit"; As a side-note: This declares a variable that is initialized to a read-only string. The variable itself is writable however, which means it will unnecessarily end up in the data section of unshared memory. > Source/WebKit/qt/Api/qwebpage.cpp:343 > + page->setGroupName(QWebPagePrivate::pageGroupName); I think we should use a string literal here, instead of a variable.
(In reply to comment #6) > (From update of attachment 95038 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95038&action=review > Thanks Simon :) > With this change all pages are automatically in the same page group, which to some extend this bug is about. This is a behavioural change that we should really verify with a test. Test added > I see that the Gtk+ port seems to behave the same, but I don't see the same behaviour in the other ports directly (as they have an API). What's the pattern their callers use? What's the safest default for us? Chromium also uses the same way, but mac and window uses an API for it. > > Source/WebKit/qt/Api/qwebpage.cpp:159 > > +const char* QWebPagePrivate::pageGroupName = "QtWebKit"; > > As a side-note: This declares a variable that is initialized to a read-only string. The variable itself is writable however, which means it will unnecessarily end up in the data section of unshared memory. > > > Source/WebKit/qt/Api/qwebpage.cpp:343 > > + page->setGroupName(QWebPagePrivate::pageGroupName); > > I think we should use a string literal here, instead of a variable. using a string now :)
Created attachment 95798 [details] Patch with test
Comment on attachment 95798 [details] Patch with test View in context: https://bugs.webkit.org/attachment.cgi?id=95798&action=review > Source/WebKit/qt/Api/qwebpage.cpp:347 > + // similar as others port's are doing Add dot :-D > Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:402 > + QTest::qWait(500); Can we avoid the qWait?
(In reply to comment #9) > (From update of attachment 95798 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95798&action=review > > > Source/WebKit/qt/Api/qwebpage.cpp:347 > > + // similar as others port's are doing > > Add dot :-D > > > Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:402 > > + QTest::qWait(500); > > Can we avoid the qWait? Hummmm, is necessary a little time to the form be updated :(
Created attachment 96797 [details] Patch wit NIT changes
Comment on attachment 96797 [details] Patch wit NIT changes Just curious, what is the inconsistency the patch is fixing? :)
(In reply to comment #12) > (From update of attachment 96797 [details]) > Just curious, what is the inconsistency the patch is fixing? :) Following the steps reported, the inconsistency is when a occurs a form submission requesting a new window, two windows are opened (instead of only) and the first window is opened with a blank page.
(In reply to comment #13) > (In reply to comment #12) > > (From update of attachment 96797 [details] [details]) > > Just curious, what is the inconsistency the patch is fixing? :) > > Following the steps reported, the inconsistency is when a occurs a form submission requesting a new window, two windows are opened (instead of only) and the first window is opened with a blank page. Nice! I'd have added it to the ChangeLog.
DRT also sets a group name for pages. Maybe that can be removed now?
(In reply to comment #15) > DRT also sets a group name for pages. Maybe that can be removed now? Hummm maybe but I don't knoew if it is really necessary, because when DRT sets the group name it's overlap the group name already set.
Created attachment 97159 [details] Patch updated changelog
Comment on attachment 97159 [details] Patch updated changelog View in context: https://bugs.webkit.org/attachment.cgi?id=97159&action=review > Source/WebKit/qt/Api/qwebpage.cpp:350 > + // Pages needs to be set at the same group name, to avoid problems with popup > + // windows and link coloration. Will be used only one page group name, making > + // it is similar as others port's are doing. This needs better explanation. Let me help you out here. // My default each page is put into their own unique page group, which affects popup windows and visited links. // Page groups (per process only) is a feature making it possible to use separate settings for each group, so that // for instance an integrated browser/email reader can use different settings for displaying HTML pages and HTML email. // To make QtWebKit work as expected out of the box, we use a default group similar to what other ports are doing. Someone might be able to make this even more clear and short :-) > Source/WebKit/qt/Api/qwebpage.cpp:351 > + page->setGroupName("org.webkit.qt.QtWebKit"); Given the above description I am not sure why such a complicated name is needed. Why not just "Default Group" or so.
Created attachment 97194 [details] Patch updated v2
The commit-queue encountered the following flaky tests while processing attachment 97194 [details]: inspector/inspected-objects-not-overriden.html bug 62729 (authors: pfeldman@chromium.org and yurys@chromium.org) The commit-queue is continuing to process your patch.
Comment on attachment 97194 [details] Patch updated v2 Clearing flags on attachment: 97194 Committed r88933: <http://trac.webkit.org/changeset/88933>
All reviewed patches have been landed. Closing bug.
Revision r88933 cherry-picked into qtwebkit-2.2 with commit a2ce773 <http://gitorious.org/webkit/qtwebkit/commit/a2ce773>
Unfortunately the fix for this caused a problem in a related area with dismissing javascript alert windows that are shown from a hidden QWebView as it will crash on the 2nd dismissal. This was working in the Webkit version with Qt 4.7.x but broken in Qt 4.8.0. Reverting the change in the patch above was confirmed to solve the problem. A complete testcase for Qt is attached.
Created attachment 114938 [details] Example to reproduce the regression problem