Bug 107705

The change is in QWebPage::geometryChangeRequested(QRect) signal. Until QtWebKit 2.3, it fired with * invalid QRect if it was opening links with target="_blank" and * valid QRect if it was opening real popup window (window.open) However, it has now changed to return QRect.size() == QSize(100, 100) if it is opening target="_blank" links. So it makes impossible to distinguish window.open (with size 100x100) and target="_blank" links.

(In reply to comment #1) > The change is in QWebPage::geometryChangeRequested(QRect) signal. > Until QtWebKit 2.3, it fired with > * invalid QRect if it was opening links with target="_blank" and > * valid QRect if it was opening real popup window (window.open) > > However, it has now changed to return QRect.size() == QSize(100, 100) if it is opening target="_blank" links. > So it makes impossible to distinguish window.open (with size 100x100) and target="_blank" links. Is that the same trick used by all the Qt browsers, to detect this?

I know only about kWebKitPart and QupZilla that are trying to detect whether to open popup window. And yes, kWebKitPart is also using the exact same trick.

The issue appears to be with enforcing the minimumWindowSize from ChromeClient.h

Seems to be regresssion caused the patch for bug #102072

Created attachment 184465 [details] Patch Quick patch, needs test.

(In reply to comment #6) > Created an attachment (id=184465) [details] > Patch > > Quick patch, needs test. It works! Thank you very much.

(In reply to comment #7) > (In reply to comment #6) > > Created an attachment (id=184465) [details] [details] > > Patch > > > > Quick patch, needs test. > > It works! Thank you very much. Some more information: window.open opens window in new tab if no width or height are defined and if only one of those is defined. If both window and height are defined window.open opens a popup window with given dimensions. If height/width is defined lower than 100 px opened popup window defaults to 100 px.

Confirm, the patch fixe the issue. @daimonion This has nothing to do with QtWebKit, this decision is made inside client application (kWebKitPart or QupZilla)

Created attachment 184469 [details] Patch

Attachment 184469 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/page/DOMWindow.cpp', u'Source/WebKit/qt/ChangeLog', u'Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp']" exit_code: 1 Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:448: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:3281: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:3282: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 3 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 184469 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184469&action=review > Source/WebCore/page/DOMWindow.cpp:346 > + // Let size 0 pass through, since that indicates default size, not minimum size. > + if (window.width()) > + window.setWidth(min(max(minimumSize.width(), window.width()), screen.width())); > + if (window.height()) > + window.setHeight(min(max(minimumSize.height(), window.height()), screen.height())); Shouldnt this be documented in some API? Can we make a WK2 C api unit test?

LGTM, but please check if there are some places where documentation has to be updated (say EFL, GTK etc)

Created attachment 186857 [details] Patch Update EFL and GTK to handle the return to possibly empty rects in setWindowRect.

Attachment 186857 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/page/DOMWindow.cpp', u'Source/WebKit/efl/ChangeLog', u'Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp', u'Source/WebKit/qt/ChangeLog', u'Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp']" exit_code: 1 Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:448: Place brace on its own line for function definitions. [whitespace/braces] [4] Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:3280: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:3281: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 3 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 186857 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186857&action=review r=me with comments > Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:131 > - if (!ewk_view_setting_enable_auto_resize_window_get(m_view)) > + if (!ewk_view_setting_enable_auto_resize_window_get(m_view) || rect.isEmpty()) You should add a FIXME: An empty rect means that default size should be used. Handle this case. > Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:146 > - gtk_window_resize(GTK_WINDOW(window), intrect.width(), intrect.height()); > + if (!intrect.isEmpty()) > + gtk_window_resize(GTK_WINDOW(window), intrect.width(), intrect.height()); Same here

Comment on attachment 186857 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=186857&action=review r=me with comments > Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:131 > - if (!ewk_view_setting_enable_auto_resize_window_get(m_view)) > + if (!ewk_view_setting_enable_auto_resize_window_get(m_view) || rect.isEmpty()) You should add a FIXME: An empty rect means that default size should be used. Handle this case. > Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:146 > - gtk_window_resize(GTK_WINDOW(window), intrect.width(), intrect.height()); > + if (!intrect.isEmpty()) > + gtk_window_resize(GTK_WINDOW(window), intrect.width(), intrect.height()); Same here

(In reply to comment #17) > (From update of attachment 186857 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=186857&action=review > > r=me with comments > > > Source/WebKit/efl/WebCoreSupport/ChromeClientEfl.cpp:131 > > - if (!ewk_view_setting_enable_auto_resize_window_get(m_view)) > > + if (!ewk_view_setting_enable_auto_resize_window_get(m_view) || rect.isEmpty()) > > You should add a FIXME: An empty rect means that default size should be used. Handle this case. > > > Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:146 > > - gtk_window_resize(GTK_WINDOW(window), intrect.width(), intrect.height()); > > + if (!intrect.isEmpty()) > > + gtk_window_resize(GTK_WINDOW(window), intrect.width(), intrect.height()); > > Same here I was assuming the default size would be used when the window was opened, which means only the resize is unnecessary.

You are probably right... fine with mee

(In reply to comment #19) > You are probably right... fine with mee Thanks!

Comment on attachment 186857 [details] Patch Clearing flags on attachment: 186857 Committed r142755: <http://trac.webkit.org/changeset/142755>

All reviewed patches have been landed. Closing bug.

Please don't use bracketed prefixes on cross-platform patches. This one had cross-platform changes, and actually caused a regression in Safari (<rdar://problem/14693930>).

(In reply to comment #23) > Please don't use bracketed prefixes on cross-platform patches. This one had cross-platform changes, and actually caused a regression in Safari (<rdar://problem/14693930>). Sorry, the brackets should not have been there. I was later told the patch solve the same problem on GTK and Chromium.

Filed bug 119633.