Bug 63256 - [GTK] Fix runtime critical warnings in WebKit2
Summary: [GTK] Fix runtime critical warnings in WebKit2
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-23 07:45 PDT by Carlos Garcia Campos
Modified: 2011-06-24 08:42 PDT (History)
0 users

See Also:


Attachments
Patch (3.14 KB, patch)
2011-06-23 07:49 PDT, Carlos Garcia Campos
mrobinson: review-
Details | Formatted Diff | Diff
New patch (3.67 KB, patch)
2011-06-24 00:57 PDT, Carlos Garcia Campos
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2011-06-23 07:45:45 PDT
(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.
Comment 1 Carlos Garcia Campos 2011-06-23 07:49:00 PDT
Created attachment 98350 [details]
Patch
Comment 2 Martin Robinson 2011-06-23 08:08:56 PDT
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?
Comment 3 Carlos Garcia Campos 2011-06-23 08:21:15 PDT
(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.
Comment 4 Carlos Garcia Campos 2011-06-24 00:28:14 PDT
(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.
Comment 5 Carlos Garcia Campos 2011-06-24 00:57:46 PDT
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.
Comment 6 Carlos Garcia Campos 2011-06-24 08:42:46 PDT
Committed r89673: <http://trac.webkit.org/changeset/89673>