GNOME Bugzilla – Bug 769786
wayland: Add support for the xdg-foreign protocol
Last modified: 2016-08-22 13:04:58 UTC
This is needed for flatpak portals to be stacked correctly on Wayland. The corresponding Wayland protocol was released as part of wayland-protocols 1.6.
Created attachment 333167 [details] [review] wayland: Add support for the xdg-foreign protocol This commits adds support for exporting xdg_surface handles via xdg_exporter and importing them via xdg_importer. This bumps the required wayland-protocols version to 1.6.
Review of attachment 333167 [details] [review]: ::: src/Makefile.am @@ +78,3 @@ wayland/protocol/gtk-shell.xml \ wayland/protocol/gtk-primary-selection.xml \ + wayland/protocol/xdg-foregin.xml \ leftover? ::: src/wayland/meta-wayland-xdg-foreign.c @@ +132,3 @@ + char *handle; + + if (!surface->role || META_IS_WAYLAND_XDG_SURFACE (surface->role)) !META_IS_WAYLAND_XDG_SURFACE ? @@ +153,3 @@ + exported = g_new0 (MetaWaylandXdgExported, 1); + exported->foreign = foreign; + exported->surface = wl_resource_get_user_data (wl_surface_resource); exported->surface = surface ? Also, what happens if this surface is destroyed ? @@ +165,3 @@ + if (g_hash_table_insert (foreign->exported_surfaces, handle, exported)) + break; + g_free (handle); if g_hash_table_insert() fails, it frees the passed key so this is a double free @@ +168,3 @@ + } + + exported->handle = g_strdup (handle); looks like there's no need to dup the string, you could just point to the original buffer @@ +218,3 @@ + MetaWaylandSurface *surface = wl_resource_get_user_data (surface_resource); + + if (!surface->role || META_IS_WAYLAND_XDG_SURFACE (surface->role)) !META_IS_WAYLAND_XDG_SURFACE @@ +235,3 @@ + imported->parent_of = surface; + + if (surface) if surface was NULL we'd have crashed dereferencing surface->role above @@ +243,3 @@ + { + meta_window_set_transient_for (surface->window, + imported->exported->surface->window); is it guaranteed that exported->surface->window exists here ? @@ +327,3 @@ + imported->resource = xdg_imported_resource; + + exported->imported = g_list_append (exported->imported, imported); g_list_append is O(n) so, since the order doesn't matter here, you should use _prepend()
(In reply to Rui Matos from comment #2) > Review of attachment 333167 [details] [review] [review]: > > @@ +153,3 @@ > + exported = g_new0 (MetaWaylandXdgExported, 1); > + exported->foreign = foreign; > + exported->surface = wl_resource_get_user_data (wl_surface_resource); > > exported->surface = surface ? > > Also, what happens if this surface is destroyed ? Good point. Had omitted that part, only dealt with explicit unexporting and client destruction. Surface being destroyed is not enough as well, we need a "unmapped" kind of thing. I'll add a MetaWaylandSurface::unmapped signal for that. > > @@ +235,3 @@ > + imported->parent_of = surface; > + > + if (surface) > > if surface was NULL we'd have crashed dereferencing surface->role above surface can never be NULL since it'll always survive until the wl_surface resource is destroyed. > > @@ +243,3 @@ > + { > + meta_window_set_transient_for (surface->window, > + > imported->exported->surface->window); > > is it guaranteed that exported->surface->window exists here ? Added that (do we have a window) as a "is-mapped" check.
Created attachment 333540 [details] [review] MetaWaylandSurface: Add 'unmapped' signal Meant to be used by users of MetaWaylandSurface's that need to know when the surface was unmapped. So far only emitted by shell surfaces (surfaces with MetaWindow's).
Created attachment 333541 [details] [review] wayland: Add support for the xdg-foreign protocol This commits adds support for exporting xdg_surface handles via xdg_exporter and importing them via xdg_importer. This bumps the required wayland-protocols version to 1.6.
Review of attachment 333541 [details] [review]: almost good ::: src/wayland/meta-wayland-xdg-foreign.c @@ +125,3 @@ + g_free (exported->handle); + + g_hash_table_remove (foreign->exported_surfaces, exported->handle); this needs to be done before freeing ->handle above @@ +199,3 @@ + { + g_hash_table_insert (foreign->exported_surfaces, + g_strdup (handle), exported); is there really a need to have a handle copy here? @@ +338,3 @@ + + g_object_remove_weak_pointer (G_OBJECT (imported->parent_of), + (gpointer *) &imported->parent_of); this should be disconnecting the signal now @@ +345,3 @@ + } + + wl_resource_set_user_data (imported->resource, NULL); we're not freeing the MetaWaylandXdgImported struct anywhere, or am I missing it?
Review of attachment 333540 [details] [review]: ::: src/wayland/meta-wayland-surface.c @@ +317,3 @@ meta_window_unmanage (surface->window, timestamp); + + g_signal_emit (surface, surface_signals[SURFACE_UNMAPPED], 0); wouldn't it be better to do this in meta_wayland_surface_set_window() when going from surface->window existing to it being set to NULL ? doing it here doesn't catch xwayland surfaces being detached nor other calls to meta_window_unmanage(). None of these cases matter for xdg_foreign but it would be more correct I think
(In reply to Rui Matos from comment #6) > Review of attachment 333541 [details] [review] [review]: > > almost good > > ::: src/wayland/meta-wayland-xdg-foreign.c > @@ +125,3 @@ > + g_free (exported->handle); > + > + g_hash_table_remove (foreign->exported_surfaces, exported->handle); > > this needs to be done before freeing ->handle above Ah right. I changed the hast table to not free 'exported' automatically, moved the free (exported->handle) after removing from the table, and finally a free (exported); in the end. > > @@ +199,3 @@ > + { > + g_hash_table_insert (foreign->exported_surfaces, > + g_strdup (handle), exported); > > is there really a need to have a handle copy here? Yes, it seems so. g_hash_table_insert() will, if I read the docs correctly, always free the key when done with it; but I need to keep it alive so I can put it in 'exported'. > > @@ +338,3 @@ > + > + g_object_remove_weak_pointer (G_OBJECT (imported->parent_of), > + (gpointer *) &imported->parent_of); > > this should be disconnecting the signal now > > @@ +345,3 @@ > + } > + > + wl_resource_set_user_data (imported->resource, NULL); > > we're not freeing the MetaWaylandXdgImported struct anywhere, or am I > missing it? No, it was missing.
Created attachment 333605 [details] [review] MetaWaylandSurface: Add 'unmapped' signal Meant to be used by users of MetaWaylandSurface's that need to know when the surface was unmapped. So far only emitted by shell surfaces (surfaces with MetaWindow's).
Created attachment 333606 [details] [review] wayland: Add support for the xdg-foreign protocol This commits adds support for exporting xdg_surface handles via xdg_exporter and importing them via xdg_importer. This bumps the required wayland-protocols version to 1.6.
Review of attachment 333605 [details] [review]: looks good ::: src/wayland/meta-wayland-surface.c @@ +1102,3 @@ MetaWindow *window) { + gboolean was_unmapped = surface->window && !window; was_mapped instead?
Review of attachment 333606 [details] [review]: looks good. untested though ::: src/wayland/meta-wayland-xdg-foreign.c @@ +201,3 @@ + { + g_hash_table_insert (foreign->exported_surfaces, + g_strdup (handle), exported); if you didn't provide a key free function to g_hash_table_new_full() the dup wouldn't be needed. anyway, bikeshedding at this point
(In reply to Rui Matos from comment #11) > Review of attachment 333605 [details] [review] [review]: > > looks good > > ::: src/wayland/meta-wayland-surface.c > @@ +1102,3 @@ > MetaWindow *window) > { > + gboolean was_unmapped = surface->window && !window; > > was_mapped instead? The signal that is triggered is "unmapped" so thought to make the variable reflect that change made sense. (In reply to Rui Matos from comment #12) > Review of attachment 333606 [details] [review] [review]: > > looks good. untested though > > ::: src/wayland/meta-wayland-xdg-foreign.c > @@ +201,3 @@ > + { > + g_hash_table_insert (foreign->exported_surfaces, > + g_strdup (handle), exported); > > if you didn't provide a key free function to g_hash_table_new_full() the dup > wouldn't be needed. anyway, bikeshedding at this point True. Slighty less string duplication is probably slightly better in the end.
Attachment 333605 [details] pushed as aea7062 - MetaWaylandSurface: Add 'unmapped' signal Attachment 333606 [details] pushed as 6f8e686 - wayland: Add support for the xdg-foreign protocol