Bug 77743 – [GTK] WebKitWebView does a lot of work during size_allocate when not mapped

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

The way GTK+ 3.x works now, GtkNotebook will unmap its children, but still call size_allocate on them. This means that the cost of resizing a GtkNotebook full of WebKitWebView (a web browser) is the cost of resizing every single WebView there. This is quite expensive and leads to very sluggish resizing of Epiphany.

[Martin Robinson]

Created attachment 125351 [details]
Patch

[WebKit Review Bot]

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

[Kalev Lember]

*** Bug 77760 has been marked as a duplicate of this bug. ***

[Carlos Garcia Campos]

(In reply to comment #0)
> The way GTK+ 3.x works now, GtkNotebook will unmap its children, but still call size_allocate on them. This means that the cost of resizing a GtkNotebook full of WebKitWebView (a web browser) is the cost of resizing every single WebView there. This is quite expensive and leads to very sluggish resizing of Epiphany.

I'm not sure what have changed between gtk 2 and 3 here, but gtk2 also maps/unmaps the children. It's done when switching tabs, gtk_widget_set_child_visible() is called with TRUE for the new tab and FALSE for the old one. GtkNotebook checks the visibility of the children before calling size_allocate for the child, like other containers. In this case, according to gtk_notebook_set_current_page() doc, children of a notebook should be visible, and that's why size_allocate is always called for all children. I think this patch is actually a workaround for the gtk issue (fixing a specific case of a WebView inside a GtkNotebook), and it should be fixed in GTK+, otherwise all gtk widgets should do the same just in case they are added to a GtkNotebook. I've filed a bug report to GTK+ and attached a patch there:

https://bugzilla.gnome.org/show_bug.cgi?id=669394

[Martin Robinson]

(In reply to comment #4)

> https://bugzilla.gnome.org/show_bug.cgi?id=669394

Thanks for the followup! Hopefully this bug is fixed upstream as well. I think we should still use the work-around, for old versions of GTK+ and also because it fixes issues on Windows.

[Carlos Garcia Campos]

What issues? If we land this, I would include a comment saying that it shouldn't be necessary, and it's a workaround for several issues in GTK+, so that if it's not needed in the future we can just remove it. The problem of this kind of workarounds is that they end up in the code forever and nobody remembers what the code is for and why it was added.

[Martin Robinson]

(In reply to comment #6)
> What issues? If we land this, I would include a comment saying that it shouldn't be necessary, and it's a workaround for several issues in GTK+, so that if it's not needed in the future we can just remove it. The problem of this kind of workarounds is that they end up in the code forever and nobody remembers what the code is for and why it was added.

This patch also fixes an issue on Windows (see the duplicated bug) where the backing store for the widget is created before it's mapped. Thus, the change cannot be reverted after the GTK+ bug is fixed, unless the backing store stuff changes.

[Gustavo Noronha (kov)]

(In reply to comment #6)
> What issues? If we land this, I would include a comment saying that it shouldn't be necessary, and it's a workaround for several issues in GTK+, so that if it's not needed in the future we can just remove it. The problem of this kind of workarounds is that they end up in the code forever and nobody remembers what the code is for and why it was added.

I would say our track record of know about the workarounds and removing them is pretty good, actually, but having a comment has always helped us =)

[Gustavo Noronha (kov)]

Comment on attachment 125351 [details]
Patch

Looks sensible to me.

[WebKit Review Bot]

Comment on attachment 125351 [details]
Patch

Clearing flags on attachment: 125351

Committed r106816: <http://trac.webkit.org/changeset/106816>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.