Bug 63451 - [GTK] Embedded GtkWidgets are not drawn
Summary: [GTK] Embedded GtkWidgets are not drawn
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-27 06:54 PDT by Dan Vrátil
Modified: 2012-02-09 08:39 PST (History)
8 users (show)

See Also:


Attachments
A simple test case (3.46 KB, text/plain)
2011-06-27 06:54 PDT, Dan Vrátil
no flags Details
WIP patch (3.68 KB, patch)
2011-09-23 10:20 PDT, Gustavo Noronha (kov)
no flags Details | Formatted Diff | Diff
Fix (2.60 KB, patch)
2012-02-07 04:39 PST, Dan Vrátil
no flags Details | Formatted Diff | Diff
Patch (6.02 KB, patch)
2012-02-08 01:52 PST, Dan Vrátil
no flags Details | Formatted Diff | Diff
Fixed patch (5.64 KB, patch)
2012-02-08 01:59 PST, Dan Vrátil
pnormand: commit-queue-
Details | Formatted Diff | Diff
Fixed patch (5.74 KB, patch)
2012-02-08 04:24 PST, Dan Vrátil
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dan Vrátil 2011-06-27 06:54:20 PDT
Created attachment 98716 [details]
A simple test case

Embedded widgets that are created in 'create-plugin-widget' signal handler are not drawn in WebKitGTK+. In the webview there is an empty space allocated with proper dimensions, but the widget is not displayed and it does not respond to any events (clicking, moving mouse over it...).

When you connect to widget's 'notify' signal, the widget reports it's being correctly resized and 'visibile' is set to TRUE, so WebKit does know about it, it just has some problems displaying/drawing it.
Comment 1 Peter Hatina 2011-07-07 00:19:34 PDT
The issue seems to be caused by not placing embedded widget into any container. If the test case gets updated in this way, everything works - instead of returning only a simple label from the "create_plugin_widget", return a top-level window containing the widget and this window will be shown.
Comment 2 Danilo Cesar Lemes de Paula 2011-09-21 15:53:24 PDT
(In reply to comment #1)
> The issue seems to be caused by not placing embedded widget into any container. If the test case gets updated in this way, everything works - instead of returning only a simple label from the "create_plugin_widget", return a top-level window containing the widget and this window will be shown.

I tried that approach, however I've  only got a new top-level window, and not a embedded widget as expected.
Comment 3 Danilo Cesar Lemes de Paula 2011-09-23 10:09:48 PDT
Source/WebCore/platform/gtk/GtkPluginWidget.cpp::void GtkPluginWidget::paint(GraphicsContext* context, const IntRect& rect)

At least in my tests, if (!context->gdkExposeEvent()) is executed, and returning, to the widget will never get painted... I'm investigating, but if you guys find any other information, that would be cool.
Comment 4 Gustavo Noronha (kov) 2011-09-23 10:20:08 PDT
Created attachment 108488 [details]
WIP patch

This is a simple patch I started writing to make the code gtk3-compliant (i.e. not use gdkExposeEvent) and test this issue. Haven't gotten the widget to show up yet, though.
Comment 5 Dan Vrátil 2012-02-07 04:39:27 PST
Created attachment 125816 [details]
Fix

Proposed patch that fixes this issue by adding the plugin widget to 
WebView container and forcing these widgets to be drawn as the very last.

Additionally, when GtkPluginWidget is destroyed it also removes the child
widget from the WebView container. Otherwise the widgets remain displayed
even when a new page is loaded in the webview.

This functionality is quite important for the Evolution WebKit port. Is it
possible to estimate when this could get to a stable release?
Comment 6 WebKit Review Bot 2012-02-07 10:47:56 PST
Attachment 125816 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/platform/gtk/GtkPluginWidge..." exit_code: 1

Source/WebCore/platform/gtk/GtkPluginWidget.cpp:46:  Declaration has space between type name and * in GtkWidget *parent  [whitespace/declaration] [3]
Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:486:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Martin Robinson 2012-02-07 10:54:40 PST
Comment on attachment 125816 [details]
Fix

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

Great work! If you attach a proper ChangeLog to this patch and fix the nits below, we can commit it directly from the bug. See http://www.webkit.org/coding/contributing.html. I think you can skip writing tests for this change...just delete that line from the ChangeLog.

Do you mind testing what happens when you remove the contents of the GtkPluginWidget::paint method entirely? If the real expose event is drawing the child widgets, perhaps we don't have to make fake expose events at all.

> Source/WebCore/platform/gtk/GtkPluginWidget.cpp:49
> +    GtkWidget *parent;
> +
> +    parent = gtk_widget_get_parent(platformWidget());
> +    gtk_container_remove(GTK_CONTAINER(parent), platformWidget());

This should just be one line:

gtk_container_remove(GTK_CONTAINER(gtk_widget_get_praent(platformWidget()), platformWidget());

>> Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:486
>> +        gtk_container_add (GTK_CONTAINER (getViewFromFrame(m_frame)), gtkWidget);
> 
> Extra space before ( in function call  [whitespace/parens] [4]

Please fix the style issue here.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:664
> +    /* Force child widgets to be drawn as last */

Might want to expand this comment a bit:

// Chaining up to the parent forces child widgets to be drawn.
Comment 8 Martin Robinson 2012-02-07 10:55:13 PST
Oh, by the way, if we commit this patch I'll merge into the stable branch for the next stable release.
Comment 9 Dan Vrátil 2012-02-08 01:52:16 PST
Created attachment 126030 [details]
Patch
Comment 10 WebKit Review Bot 2012-02-08 01:54:24 PST
Attachment 126030 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebKit/gtk/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Source/WebKit/gtk/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 4 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Dan Vrátil 2012-02-08 01:54:58 PST
Hi,

indeed the paint() method in GtkPluginWidget is not needed anymore.

Hopefully it's all OK now.
Comment 12 Dan Vrátil 2012-02-08 01:59:08 PST
Created attachment 126032 [details]
Fixed patch

Sorry, another try.
Comment 13 Philippe Normand 2012-02-08 02:09:28 PST
Comment on attachment 126032 [details]
Fixed patch

Attachment 126032 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11449322
Comment 14 Dan Vrátil 2012-02-08 04:24:35 PST
Created attachment 126053 [details]
Fixed patch
Comment 15 Martin Robinson 2012-02-08 07:35:49 PST
Comment on attachment 126053 [details]
Fixed patch

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

> Source/WebKit/gtk/webkit/webkitwebview.cpp:672
> +    /* Chaining up to the parent forces child widgets to be drawn. */
> +    GTK_WIDGET_CLASS(webkit_web_view_parent_class)->draw(widget, cr);
> +

You removed the ::paint method for GTK+ 2.x, but you did not chain up in webkit_web_view_expose. We use C++ style comments usually. :) No worries though. I will fix those two minor issues and land this patch. Thank you for taking the time to investigate this issue!
Comment 16 Martin Robinson 2012-02-09 08:39:14 PST
Committed r107249: <http://trac.webkit.org/changeset/107249>