Bug 210206

Created attachment 395850 [details] Patch

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

Comment on attachment 395850 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395850&action=review > Source/WebKit/UIProcess/API/glib/WebKitWindowProperties.cpp:47 > + * sizes bigger than 100x100. And if no size its specified then this value If no size is specified, then

Comment on attachment 395850 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395850&action=review > Tools/MiniBrowser/gtk/BrowserWindow.c:323 > + * The issue is that 100x100 its too small and cause failures on some is too small > Tools/MiniBrowser/gtk/BrowserWindow.c:328 > + if (geometry.width > 100 && geometry.height > 100) Maybe we could do this only when web view is controlled by automation?

(In reply to Carlos Garcia Campos from comment #4) > Comment on attachment 395850 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=395850&action=review > > > Tools/MiniBrowser/gtk/BrowserWindow.c:323 > > + * The issue is that 100x100 its too small and cause failures on some > > is too small > > > Tools/MiniBrowser/gtk/BrowserWindow.c:328 > > + if (geometry.width > 100 && geometry.height > 100) > > Maybe we could do this only when web view is controlled by automation? I think it makes sense to do it in any case. Just try a basic example of window.open with the minibrowser and check how bad it looks the new window it opens: (you almost can't see it). Ex: https://www.w3schools.com/jsref/tryit.asp?filename=tryjsref_win_open

(In reply to Carlos Alberto Lopez Perez from comment #5) > (In reply to Carlos Garcia Campos from comment #4) > > Comment on attachment 395850 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=395850&action=review > > > > > Tools/MiniBrowser/gtk/BrowserWindow.c:323 > > > + * The issue is that 100x100 its too small and cause failures on some > > > > is too small > > > > > Tools/MiniBrowser/gtk/BrowserWindow.c:328 > > > + if (geometry.width > 100 && geometry.height > 100) > > > > Maybe we could do this only when web view is controlled by automation? > > I think it makes sense to do it in any case. > > Just try a basic example of window.open with the minibrowser and check how > bad it looks the new window it opens: (you almost can't see it). > > Ex: https://www.w3schools.com/jsref/tryit.asp?filename=tryjsref_win_open Then I would not change this in MiniBrowser. The spec says 100 is the minimum but not the default, so we can change that behavior and use a different default size, for example parent window size.

(In reply to Carlos Garcia Campos from comment #6) > (In reply to Carlos Alberto Lopez Perez from comment #5) > > (In reply to Carlos Garcia Campos from comment #4) > > > Comment on attachment 395850 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=395850&action=review > > > > > > > Tools/MiniBrowser/gtk/BrowserWindow.c:323 > > > > + * The issue is that 100x100 its too small and cause failures on some > > > > > > is too small > > > > > > > Tools/MiniBrowser/gtk/BrowserWindow.c:328 > > > > + if (geometry.width > 100 && geometry.height > 100) > > > > > > Maybe we could do this only when web view is controlled by automation? > > > > I think it makes sense to do it in any case. > > > > Just try a basic example of window.open with the minibrowser and check how > > bad it looks the new window it opens: (you almost can't see it). > > > > Ex: https://www.w3schools.com/jsref/tryit.asp?filename=tryjsref_win_open > > Then I would not change this in MiniBrowser. The spec says 100 is the > minimum but not the default, so we can change that behavior and use a > different default size, for example parent window size. Its there a spec for this minimum/default size? I can't find any reference at https://html.spec.whatwg.org/multipage/window-object.html#dom-open What Firefox and Chrome seem to do its to open new windows without a specific size on new tabs.

I saw it in https://www.w3schools.com/jsref/met_win_open.asp also included in https://developer.mozilla.org/en-US/docs/Web/API/Window/open#Window_features

Created attachment 396003 [details] Patch v2: set the default requested window size to the one of the old window

Comment on attachment 396003 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396003&action=review > Source/WebCore/ChangeLog:11 > + The size of the new window its not specified, and we were defaulting is not specified > Source/WebCore/ChangeLog:12 > + to the minimum of 100x100 which its just too small for some tests which is > Source/WebCore/loader/FrameLoader.cpp:-4133 > - // Zero width and height mean using default size, not minumum one. So, WebCore is indeed doing the right thing here, if not size is specified the default one is used. We are failing to provide a default size when windowRect is called before the window is shown. We can fix it in GLib UI client doing something like this in windowFrame(): GdkRectangle geometry = { 0, 0, 0, 0 }; GtkWidget* window = gtk_widget_get_toplevel(GTK_WIDGET(m_webView)); if (WebCore::widgetIsOnscreenToplevelWindow(window) && gtk_widget_get_visible(window)) { gtk_window_get_position(GTK_WINDOW(window), &geometry.x, &geometry.y); gtk_window_get_size(GTK_WINDOW(window), &geometry.width, &geometry.height); } else { GdkRectangle defaultGeometry; webkit_window_properties_get_geometry(webkit_web_view_get_window_properties(m_webView), &defaultGeometry); if ((!defaultGeometry.width || !defaultGeometry.height) && WebCore::widgetIsOnscreenToplevelWindow(window)) { int defaultWidth, defaultHeight; gtk_window_get_default_size(GTK_WINDOW(window), &defaultWidth, &defaultHeight); if (!defaultGeometry.width && defaultWidth != -1) geometry.width = defaultWidth; if (!defaultGeometry.height && defaultHeight != -1) geometry.height = defaultHeight; } } completionHandler(WebCore::FloatRect(geometry)); Note that we will still get 100x100 when the window doesn't have a default size set (it's not the case of MiniBrowser), but we don't have a way to get the previous window here, so I think we can just assume that apps should handle that case by setting their own minimum size if 100x100 is too small. > Source/WebCore/loader/FrameLoader.cpp:4141 > + else > + windowRect.setWidth(oldWindowRect.width()); This is changing the behavior for other ports, I think we should handle this in WebKit layer, but if we end up doing it here, it should be under plartform ifdefs, or ar least only done when windowRect.width() == 0. > Source/WebCore/loader/FrameLoader.cpp:4145 > + else > + windowRect.setHeight(oldWindowRect.height()); Ditto.

(In reply to Carlos Garcia Campos from comment #10) > Comment on attachment 396003 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=396003&action=review > > > Source/WebCore/ChangeLog:11 > > + The size of the new window its not specified, and we were defaulting > > is not specified > > > Source/WebCore/ChangeLog:12 > > + to the minimum of 100x100 which its just too small for some tests > > which is > > > Source/WebCore/loader/FrameLoader.cpp:-4133 > > - // Zero width and height mean using default size, not minumum one. > > So, WebCore is indeed doing the right thing here, if not size is specified > the default one is used. We are failing to provide a default size when > windowRect is called before the window is shown. We can fix it in GLib UI > client doing something like this in windowFrame(): > > GdkRectangle geometry = { 0, 0, 0, 0 }; > GtkWidget* window = gtk_widget_get_toplevel(GTK_WIDGET(m_webView)); > if (WebCore::widgetIsOnscreenToplevelWindow(window) && > gtk_widget_get_visible(window)) { > gtk_window_get_position(GTK_WINDOW(window), &geometry.x, > &geometry.y); > gtk_window_get_size(GTK_WINDOW(window), &geometry.width, > &geometry.height); > } else { > GdkRectangle defaultGeometry; > > webkit_window_properties_get_geometry(webkit_web_view_get_window_properties(m > _webView), &defaultGeometry); > if ((!defaultGeometry.width || !defaultGeometry.height) && > WebCore::widgetIsOnscreenToplevelWindow(window)) { > int defaultWidth, defaultHeight; > gtk_window_get_default_size(GTK_WINDOW(window), > &defaultWidth, &defaultHeight); > if (!defaultGeometry.width && defaultWidth != -1) > geometry.width = defaultWidth; > if (!defaultGeometry.height && defaultHeight != -1) > geometry.height = defaultHeight; > } > } > completionHandler(WebCore::FloatRect(geometry)); > Good point and thanks for the hints! :) > Note that we will still get 100x100 when the window doesn't have a default > size set (it's not the case of MiniBrowser), but we don't have a way to get > the previous window here, so I think we can just assume that apps should > handle that case by setting their own minimum size if 100x100 is too small. > I see. Maybe we can try to do both things? use the default window size if set, and if there is no default window size, then use the size of the previous window. > > Source/WebCore/loader/FrameLoader.cpp:4141 > > + else > > + windowRect.setWidth(oldWindowRect.width()); > > This is changing the behavior for other ports, I think we should handle this > in WebKit layer, but if we end up doing it here, it should be under > plartform ifdefs, or ar least only done when windowRect.width() == 0. > > > Source/WebCore/loader/FrameLoader.cpp:4145 > > + else > > + windowRect.setHeight(oldWindowRect.height()); > > Ditto. Ok.

Created attachment 396452 [details] Patch

Created attachment 396454 [details] Patch

Comment on attachment 396454 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396454&action=review > Source/WebCore/ChangeLog:11 > + The size of the new window is not specified, and we were failimg failimg -> failing > Source/WebCore/loader/FrameLoader.cpp:4129 > +#if PLATFORM(GTK) || PLATFORM(WPE) I'm not sure this works in WPE. It's unfortunate we have to do this here, but we don't have access to the previous window from the API layer. > Tools/TestWebKitAPI/Tests/WebKitGLib/TestUIClient.cpp:370 > +#if PLATFORM(GTK) > + if (m_shouldCreateWebViewsInNewWindowsAutomatically) { > + g_assert_null(m_parentWindow); > + m_parentWindow = gtk_window_new(GTK_WINDOW_TOPLEVEL); > + gtk_window_set_default_size(GTK_WINDOW(m_parentWindow), m_DefaultGeometryNewWindows.width, m_DefaultGeometryNewWindows.height); > + gtk_container_add(GTK_CONTAINER(m_parentWindow), GTK_WIDGET(newWebView)); > + gtk_widget_show(GTK_WIDGET(newWebView)); > + gtk_widget_show(m_parentWindow); > + } > +#endif Maybe we can just change showInWindowAndWaitUntilMapped() to set the default size, I don't think that will affect other tests. Ok, I see now that we need it for this particular test. Then I would add a parameter ShouldSetDefaultSize or something similar. > Tools/TestWebKitAPI/Tests/WebKitGLib/TestUIClient.cpp:705 > + // if no size specified for window.open(), then new windows open with the default window size. If > Tools/TestWebKitAPI/Tests/WebKitGLib/TestUIClient.cpp:715 > + // if no size specified for window.open(), and new windows are not set to a specific default size with gtk_window_set_default_size() If

(In reply to Carlos Garcia Campos from comment #14) > Comment on attachment 396454 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=396454&action=review > > > Source/WebCore/ChangeLog:11 > > + The size of the new window is not specified, and we were failimg > > failimg -> failing > > > Source/WebCore/loader/FrameLoader.cpp:4129 > > +#if PLATFORM(GTK) || PLATFORM(WPE) > > I'm not sure this works in WPE. It's unfortunate we have to do this here, > but we don't have access to the previous window from the API layer. > I tested with some printfs and it seems WPE always reports the size of the view backend as the default window size. I'm not 100% sure if you can have a view backend without a specified size, but I suspect its not possible. So I guess this its not needed for WPE. > > Tools/TestWebKitAPI/Tests/WebKitGLib/TestUIClient.cpp:370 > > +#if PLATFORM(GTK) > > + if (m_shouldCreateWebViewsInNewWindowsAutomatically) { > > + g_assert_null(m_parentWindow); > > + m_parentWindow = gtk_window_new(GTK_WINDOW_TOPLEVEL); > > + gtk_window_set_default_size(GTK_WINDOW(m_parentWindow), m_DefaultGeometryNewWindows.width, m_DefaultGeometryNewWindows.height); > > + gtk_container_add(GTK_CONTAINER(m_parentWindow), GTK_WIDGET(newWebView)); > > + gtk_widget_show(GTK_WIDGET(newWebView)); > > + gtk_widget_show(m_parentWindow); > > + } > > +#endif > > Maybe we can just change showInWindowAndWaitUntilMapped() to set the default > size, I don't think that will affect other tests. Ok, I see now that we need > it for this particular test. Then I would add a parameter > ShouldSetDefaultSize or something similar. > I tried but I couldn't manage to set a default size from the function showInWindowAndWaitUntilMapped() that webkit_window_properties_get_geometry() would report. As far as I can see the main issue with showInWindowAndWaitUntilMapped() its that its not called on the "create" signal when the webview its created, but its called manually by the test after that. So it works when you care about setting a size for the current test window but it doesn't work for setting an automatic/default size for a new window that opens. For this test to work I need to set the default size of the window when the new webview its created in the signal handler for the "create" signal, so when the signal "ready-to-show" triggers the window already has a default size that webkit_window_properties_get_geometry() can query.

Committed r260176: <https://trac.webkit.org/changeset/260176>