Bug 63256 – [GTK] Fix runtime critical warnings in WebKit2

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Carlos Garcia Campos]

(WebKitWebProcess:27317): Gtk-CRITICAL **: gtk_widget_get_toplevel: assertion `GTK_IS_WIDGET (widget)' failed

(WebKitWebProcess:27317): Gtk-CRITICAL **: gtk_widget_is_toplevel: assertion `GTK_IS_WIDGET (widget)' failed

The problem is that PlatformScreenGtk.cpp tries to use the view widget, be in WebKit2 the web process doesn't have a view widget.

[Carlos Garcia Campos]

Created attachment 98350 [details]
Patch

[Martin Robinson]

Comment on attachment 98350 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=98350&action=review

> Source/WebCore/platform/gtk/PlatformScreenGtk.cpp:92
> -FloatRect screenRect(Widget* widget)
> +static gboolean getScreenAndMonitorForWidget(Widget* widget, GdkScreen** screenOut, gint* monitorOut)

I'd rather this be a smaller helper (getScreen) like getVisual. In fact, getVisual should just use this helper to get the screen.

> Source/WebCore/platform/gtk/PlatformScreenGtk.cpp:127
> +    if (!widget)
> +        return FloatRect();
> +
> +    GdkScreen* screen;
> +    gint monitor;
> +    if (!getScreenAndMonitorForWidget(widget, &screen, &monitor))
>          return FloatRect();
>  
> -    gint monitor = gdk_screen_get_monitor_at_window(screen, gtk_widget_get_window(GTK_WIDGET(container)));
>      GdkRectangle geometry;
>      gdk_screen_get_monitor_geometry(screen, monitor, &geometry);
> -    
> +

I think this could be a bit more straightforward. The use of out-parameters is a bit cludgy here. Why not just maintain the structure of screenRect and then do an early return if getScreenForWidget/getScreen fails?

[Carlos Garcia Campos]

(In reply to comment #2)
> (From update of attachment 98350 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98350&action=review
> 
> > Source/WebCore/platform/gtk/PlatformScreenGtk.cpp:92
> > -FloatRect screenRect(Widget* widget)
> > +static gboolean getScreenAndMonitorForWidget(Widget* widget, GdkScreen** screenOut, gint* monitorOut)
> 
> I'd rather this be a smaller helper (getScreen) like getVisual. In fact, getVisual should just use this helper to get the screen.

You mean the name? or that we only return the screen and not the monitor?

> > Source/WebCore/platform/gtk/PlatformScreenGtk.cpp:127
> > +    if (!widget)
> > +        return FloatRect();
> > +
> > +    GdkScreen* screen;
> > +    gint monitor;
> > +    if (!getScreenAndMonitorForWidget(widget, &screen, &monitor))
> >          return FloatRect();
> >  
> > -    gint monitor = gdk_screen_get_monitor_at_window(screen, gtk_widget_get_window(GTK_WIDGET(container)));
> >      GdkRectangle geometry;
> >      gdk_screen_get_monitor_geometry(screen, monitor, &geometry);
> > -    
> > +
> 
> I think this could be a bit more straightforward. The use of out-parameters is a bit cludgy here. Why not just maintain the structure of screenRect and then do an early return if getScreenForWidget/getScreen fails?

I'm not sure I get it, that's what it does, it returns early when getScreenAndMonitorForWidget() fails.

[Carlos Garcia Campos]

(In reply to comment #2)
> (From update of attachment 98350 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98350&action=review
> 
> > Source/WebCore/platform/gtk/PlatformScreenGtk.cpp:92
> > -FloatRect screenRect(Widget* widget)
> > +static gboolean getScreenAndMonitorForWidget(Widget* widget, GdkScreen** screenOut, gint* monitorOut)
> 
> I'd rather this be a smaller helper (getScreen) like getVisual.

The problem is that screenRect needs the toplevel widget to get monitor, so if we only return the screen, we need to get the toplevel again to get the monitor. That's why I added a function to get both the screen and monitor.

> In fact, getVisual should just use this helper to get the screen.

getVisual() doesn't need the screen, only when there's no widget, but in that case it uses the default screen to return the system visual.

[Carlos Garcia Campos]

Created attachment 98470 [details]
New patch

Ok, I've tried to simplify the patch by adding smaller helper functions and avoiding the use of out parameters.

[Carlos Garcia Campos]

Committed r89673: <http://trac.webkit.org/changeset/89673>