[webkit-reviews] review denied: [Bug 210206] [GTK] MiniBrowser opens new windows too small causing failures on some WPT tests : [Attachment 396003] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Apr 10 02:34:00 PDT 2020


Carlos Garcia Campos <cgarcia at igalia.com> has denied Carlos Alberto Lopez Perez
<clopez at igalia.com>'s request for review:
Bug 210206: [GTK] MiniBrowser opens new windows too small causing failures on
some WPT tests
https://bugs.webkit.org/show_bug.cgi?id=210206

Attachment 396003: Patch

https://bugs.webkit.org/attachment.cgi?id=396003&action=review




--- Comment #10 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 396003
  --> https://bugs.webkit.org/attachment.cgi?id=396003
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_w
ebView), &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.


More information about the webkit-reviews mailing list