Summary: | [GTK] WebKitWebView won't work in a GtkOffscreenWindow | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Claudio Saavedra <csaavedra> | ||||||||||||||||
Component: | WebKitGTK | Assignee: | Martin Robinson <mrobinson> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | cgarcia, gustavo, jeromeg, mrobinson, pnormand, webkit.review.bot, xan.lopez | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Bug Depends on: | |||||||||||||||||||
Bug Blocks: | 76181 | ||||||||||||||||||
Attachments: |
|
Description
Claudio Saavedra
2012-01-24 06:51:26 PST
Created attachment 123728 [details]
simple test program
Created attachment 123730 [details] [GTK] WebKitWebView won't work in a GtkOffscreenWindow [GTK] WebKitWebView won't work in a GtkOffscreenWindow https://bugs.webkit.org/show_bug.cgi?id=76911 Reviewed by NOBODY (OOPS!). * WebCoreSupport/ChromeClientGtk.cpp: (WebKit::ChromeClient::windowRect): (WebKit::ChromeClient::setWindowRect): Do not call GtkWindow API in GtkOffscreenWindow * tests/testwebview.c: Add a simple test for offscreen windows. Comment on attachment 123730 [details]
[GTK] WebKitWebView won't work in a GtkOffscreenWindow
This patch is awesome. The only thing I wonder is if WebKit2 has similar problems. Do you mind double checking that?
(In reply to comment #3) > (From update of attachment 123730 [details]) > This patch is awesome. The only thing I wonder is if WebKit2 has similar problems. Do you mind double checking that? For the same usecase, WK2 is not crashing. However, I see one place where the same assumption is made, but I don't really know how to check it: WebKitUIClient::getWindowFrame(WKPageRef page, const void*) Comment on attachment 123730 [details] [GTK] WebKitWebView won't work in a GtkOffscreenWindow View in context: https://bugs.webkit.org/attachment.cgi?id=123730&action=review Thanks. I think you just need to make the same fix in WebKit2 and fix the few style nits below. > Source/WebKit/gtk/tests/testwebview.c:369 > + GtkWidget* web_view; I think your asterisk is in the wrong place here. > Source/WebKit/gtk/tests/testwebview.c:374 > + window = gtk_offscreen_window_new(); > + web_view = webkit_web_view_new(); Just define the variables where you use them. Created attachment 124589 [details] [GTK] WebKitWebView won't work in a GtkOffscreenWindow [GTK] WebKitWebView won't work in a GtkOffscreenWindow https://bugs.webkit.org/show_bug.cgi?id=76911 Reviewed by NOBODY (OOPS!). * WebCoreSupport/ChromeClientGtk.cpp: (WebKit::ChromeClient::windowRect): (WebKit::ChromeClient::setWindowRect): Do not call GtkWindow API in GtkOffscreenWindow * tests/testwebview.c: Add a simple test for offscreen windows. Attachment 124589 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/gtk/ChangeLog', u'Source/Web..." exit_code: 1
Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:114: Extra space before ( in function call [whitespace/parens] [4]
WARNING: File exempt from style guide. Skipping: "Source/WebKit/gtk/tests/testwebview.c"
Total errors found: 1 in 5 files
If any of these errors are false positives, please file a bug against check-webkit-style.
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 Created attachment 124593 [details] [GTK] WebKitWebView won't work in a GtkOffscreenWindow [GTK] WebKitWebView won't work in a GtkOffscreenWindow https://bugs.webkit.org/show_bug.cgi?id=76911 Reviewed by NOBODY (OOPS!). * WebCoreSupport/ChromeClientGtk.cpp: (WebKit::ChromeClient::windowRect): (WebKit::ChromeClient::setWindowRect): Do not call GtkWindow API in GtkOffscreenWindow * tests/testwebview.c: Add a simple test for offscreen windows. Comment on attachment 124593 [details] [GTK] WebKitWebView won't work in a GtkOffscreenWindow View in context: https://bugs.webkit.org/attachment.cgi?id=124593&action=review > Source/WebKit/gtk/tests/testwebview.c:380 > + loop = g_main_loop_new(NULL, TRUE); > + > + GtkWidget *window = gtk_offscreen_window_new(); > + GtkWidget *web_view = webkit_web_view_new(); > + > + gtk_container_add(GTK_CONTAINER(window), web_view); > + gtk_widget_show_all(window); > + g_signal_connect(web_view, "notify::load-status", G_CALLBACK(idle_quit_loop_cb), NULL); > + webkit_web_view_load_uri(WEBKIT_WEB_VIEW(web_view), base_uri); > + > + g_main_loop_run(loop); > + Seems that the main loop is leaking here? I'll fix when landing. (In reply to comment #0) > GtkOffscreenWindow, although a GtkWindow, shouldn't be used with standard GtkWindow API. Do you know why? isn't it a GtkOffscreenWindow bug? WebKit::ChromeClient makes several calls to gtk_window_* methods on the toplevel that seem to return garbage when used with a GtkOffscreenWindow. As a result of this, it's not possible to use a WebKitWebView inside a GtkOffscreenWindow. > We are assuming a toplevel is a Gtkwindow in several places, I'm not sure it's correct, we should probably always check gtk_widget_is_toplevel() && GTK_IS_WINDOW() but that wouldn't work in this case either. Created attachment 124788 [details]
More extensive version of Claudio's patch
Attachment 124788 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9
Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Auto-merging Source/WebCore/WebCore.exp.in
Auto-merging Source/WebKit/mac/ChangeLog
CONFLICT (content): Merge conflict in Source/WebKit/mac/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.
When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".
rebase refs/remotes/origin/master: command returned error: 1
Died at Tools/Scripts/update-webkit line 164.
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 124788 [details] More extensive version of Claudio's patch Attachment 124788 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11388075 Comment on attachment 124788 [details] More extensive version of Claudio's patch View in context: https://bugs.webkit.org/attachment.cgi?id=124788&action=review > Source/WebCore/platform/gtk/GtkUtilities.cpp:48 > +bool gtkWidgetIsValidToplevel(GtkWidget* widget) > +{ > + return gtk_widget_is_top_level(widget) && GTK_IS_WINDOW(widget) && !GTK_IS_OFFSCREEN_WINDOW(widget); > +} I would call it gtkWidgetIsToplevelWindow, because it can be a valid toplevel, but not a GtkWindow. Also, this is always called after gtk_widget_get_top_level, why not something like gtkWidgetGetToplevelWindow() that returns the toplevel widget or NULL. Or maybe split into two methods for the cases where you already have the toplevel widget. bool gtkWidgetIsToplevelWindow(GtkWidget* widget) { return gtk_widget_is_top_level(widget) && GTK_IS_WINDOW(widget) && !GTK_IS_OFFSCREEN_WINDOW(widget); } GtkWidget* gtkWidgetGetToplevelWindow(GtkWidget* widget) { GtkWidget* toplevel = gtk_widget_get_toplevel(widget); return gtkWidgetIsToplevelWindow(toplevel) ? toplevel : 0; } > Source/WebKit/gtk/tests/testwebview.c:382 > +static void test_webkit_web_view_offscreen_window() > +{ > + loop = g_main_loop_new(NULL, TRUE); > + > + GtkWidget *window = gtk_offscreen_window_new(); > + GtkWidget *web_view = webkit_web_view_new(); > + > + gtk_container_add(GTK_CONTAINER(window), web_view); > + gtk_widget_show_all(window); > + g_signal_connect(web_view, "notify::load-status", G_CALLBACK(idle_quit_loop_cb), NULL); > + webkit_web_view_load_uri(WEBKIT_WEB_VIEW(web_view), base_uri); > + > + g_main_loop_run(loop); > + > + gtk_widget_destroy(window); > + g_main_loop_unref(loop); You should comment here that you are testing that using a WebView inside a GtkOffscreenWindow doesn't crash, because it's not obvious what the test is for. > Source/WebKit/gtk/webkit/webkitwebview.cpp:903 > - if (gtk_widget_is_toplevel(toplevel) && gtk_window_has_toplevel_focus(GTK_WINDOW(toplevel))) { > + if (gtkWidgetIsValidToplevel(toplevel)) { You have removed the has_toplevel_focus check, isn't it necessary anymore? > Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:115 > - if (gtk_widget_is_toplevel(window) && gtk_widget_get_visible(window)) { > + if (gtkWidgetIsValidToplevel(window)) { I'd say the get_visible() check is still needed here. > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:227 > - if (gtk_widget_is_toplevel(toplevel) && gtk_window_has_toplevel_focus(GTK_WINDOW(toplevel))) { > + if (gtkWidgetIsValidToplevel(toplevel)) { Here you removed the has_toplevel_focus check too. (In reply to comment #15) > (From update of attachment 124788 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124788&action=review > > > Source/WebCore/platform/gtk/GtkUtilities.cpp:48 > > +bool gtkWidgetIsValidToplevel(GtkWidget* widget) > > +{ > > + return gtk_widget_is_top_level(widget) && GTK_IS_WINDOW(widget) && !GTK_IS_OFFSCREEN_WINDOW(widget); > > +} > > I would call it gtkWidgetIsToplevelWindow, because it can be a valid toplevel, but not a GtkWindow. Also, this is always called after gtk_widget_get_top_level, why not something like gtkWidgetGetToplevelWindow() that returns the toplevel widget or NULL. Or maybe split into two methods for the cases where you already have the toplevel widget. I changed this to widgetIsOnscreenToplevelWindow. > bool gtkWidgetIsToplevelWindow(GtkWidget* widget) > { > return gtk_widget_is_top_level(widget) && GTK_IS_WINDOW(widget) && !GTK_IS_OFFSCREEN_WINDOW(widget); > } > > GtkWidget* gtkWidgetGetToplevelWindow(GtkWidget* widget) > { > GtkWidget* toplevel = gtk_widget_get_toplevel(widget); > return gtkWidgetIsToplevelWindow(toplevel) ? toplevel : 0; > } Skipped this for now. > > Source/WebKit/gtk/tests/testwebview.c:382 > > +static void test_webkit_web_view_offscreen_window() > > +{ > > + loop = g_main_loop_new(NULL, TRUE); > > + > > + GtkWidget *window = gtk_offscreen_window_new(); > > + GtkWidget *web_view = webkit_web_view_new(); > > + > > + gtk_container_add(GTK_CONTAINER(window), web_view); > > + gtk_widget_show_all(window); > > + g_signal_connect(web_view, "notify::load-status", G_CALLBACK(idle_quit_loop_cb), NULL); > > + webkit_web_view_load_uri(WEBKIT_WEB_VIEW(web_view), base_uri); > > + > > + g_main_loop_run(loop); > > + > > + gtk_widget_destroy(window); > > + g_main_loop_unref(loop); > > You should comment here that you are testing that using a WebView inside a GtkOffscreenWindow doesn't crash, because it's not obvious what the test is for. Instead of a comment we should rename the test. I did that. > > Source/WebKit/gtk/webkit/webkitwebview.cpp:903 > > - if (gtk_widget_is_toplevel(toplevel) && gtk_window_has_toplevel_focus(GTK_WINDOW(toplevel))) { > > + if (gtkWidgetIsValidToplevel(toplevel)) { > > You have removed the has_toplevel_focus check, isn't it necessary anymore? > > > Source/WebKit2/UIProcess/API/gtk/WebKitUIClient.cpp:115 > > - if (gtk_widget_is_toplevel(window) && gtk_widget_get_visible(window)) { > > + if (gtkWidgetIsValidToplevel(window)) { > > I'd say the get_visible() check is still needed here. > > > Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:227 > > - if (gtk_widget_is_toplevel(toplevel) && gtk_window_has_toplevel_focus(GTK_WINDOW(toplevel))) { > > + if (gtkWidgetIsValidToplevel(toplevel)) { > > Here you removed the has_toplevel_focus check too. Indeed. A series of dumb mistakes. Thanks for pointing them out. I've fixed them. Created attachment 124894 [details]
Patch
Comment on attachment 124894 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=124894&action=review > Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:784 > + GtkWidget* toplevel = gtk_widget_get_toplevel(GTK_WIDGET(m_webView)); > + if (!widgetIsOnscreenToplevelWindow(toplevel)) > + toplevel = 0; > + > GtkWidget* dialog = gtk_file_chooser_dialog_new(_("Upload File"), > - GTK_WINDOW(gtk_widget_get_toplevel(GTK_WIDGET(m_webView))), > + GTK_WINDOW(toplevel), toplevel might be null here, you should use something like: toplevel ? GTK_WINDOW(toplevel) : 0 > Source/WebKit/gtk/tests/testwebview.c:367 > +static void test_webkit_web_view_in_offscreen_window_does_not_crash() Much better than a comment :-) Created attachment 124994 [details]
Patch
Comment on attachment 124994 [details]
Patch
LGTM
Comment on attachment 124994 [details]
Patch
Neat!
Comment on attachment 124994 [details] Patch Clearing flags on attachment: 124994 Committed r106559: <http://trac.webkit.org/changeset/106559> All reviewed patches have been landed. Closing bug. *** Bug 62090 has been marked as a duplicate of this bug. *** |