1282015 - [Wayland] - Implement Wayland clipboard

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect, P2)

Description

[Martin Stránský [:stransky] (ni? me)]

Clipboard does not work on Wayland right now.

Comment 1

[Martin Stránský [:stransky] (ni? me)]

An update - clipboard works somehow but needs fix, perhaps remove the X11 dependent code here. Firefox to Firefox clipboard -> Works
Firefox to Wayland apps (terminal) -> Works
Wayland apps (terminal) to Firefox -> Fails

Comment 2

[Martin Stránský [:stransky] (ni? me)]

The problematic part is here:

https://dxr.mozilla.org/mozilla-central/source/widget/gtk/nsClipboard.cpp#923

we Wait() for clipboard content by filtering X11 events and process only those related to clipboard.

There's a comment for it:

// Our own versions of gtk_clipboard_wait_for_contents and
// gtk_clipboard_wait_for_text, which don't run the event loop while
// waiting for the data.  This prevents a lot of problems related to
// dispatching events at unexpected times.

static GtkSelectionData *
wait_for_contents          (GtkClipboard *clipboard, GdkAtom target);

The question is how handle it at Wayland. It is still issue there if we run the loop or can we use that as a workaround for Wayland?

Comment 3

[Martin Stránský [:stransky] (ni? me)]

The best I can think of is to rewrite this X11 in wayland, to use wl_display_* routines. It means we'd need to add build time option to build/link again Wayland. Karl, what do you think?

Comment 4

[Karl Tomlinson (:karlt)]

(In reply to Martin Stránský from comment #2)
> The question is how handle it at Wayland. It is still issue there if we run
> the loop or can we use that as a workaround for Wayland?

Nested event loops are still a problem.
I think it is worth avoiding nested event loops.

Unfortunately, fixing Gecko to make the APIs asynchronous would be
considerable work, I imagine.

(In reply to Martin Stránský from comment #3)
> The best I can think of is to rewrite this X11 in wayland, to use
> wl_display_* routines. It means we'd need to add build time option to
> build/link again Wayland. Karl, what do you think?

Yes, I expect this is the best way to progress.
I doubt GDK provides API to search through pending events.

I guess wayland routines or gdk_wayland_ will be required elsewhere eventually
anyway, and so adding the build time option seems reasonable to me.

If the build time option becomes a problem, then dlsym can be reconsidered
later.

Comment 5

[Martin Stránský [:stransky] (ni? me)]

Attachment: clipboard split patch

This patch is a first step to the implementation. The problem is with data retrieval so I moved RetrievalContext out and do X11 specific implementation in a separated file. We can have a run-time switch then to decide which clipboard to use. 

I'm going to also provide the Wayland version of RetrievalContext, proof of concept is here: https://github.com/stransky/redhat/blob/master/wayland_clip.cpp

Comment 6

[Martin Stránský [:stransky] (ni? me)]

Attachment: wayland clipboard patch

There's a complete patch for clipboard on X11/Wayland.

Due to missing build-time option it hardcodes the libwayland-client.so library but that's a minor issue here and can be addressed later.

The patch creates a nsRetrievalContext class which handles clipboard content by two main methods HasDataMatchingFlavors() and GetClipboardContent().

It also redesigns the nsClipboard::GetData() and use only one data type for content/strings.

Karl, I'd like to know if you agree with the overall conception here.

I can't test the Wayland part thoroughly due to Bug 1289251 so I'm going to address that one first.

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 8791749 [details] [diff] [review]
wayland clipboard patch

This is harder than I expected and it is all Gecko's fault for assuming that
async IPC reads appear synchronous.

We miss out on all the code GTK provides to unify the different systems into a
GtkSelectionData and to perform sync copies within the process, but I don't
think we have a choice.  On Wayland, the
GDK_SELECTION_NOTIFY/gdk_selection_property_get model exists only between GDK
and GTK.  The Wayland GDK backend jumps through hoops to provide this model
for GTK, placing events on the display.  Perhaps it may be possible to use a
different GdkDisplay for selection conversion, but I assume the async use of
GInputStream in the Wayland GDK backend depends on the current Glib main
context running, and I doubt that switching out the main context for another would work out well.

Merger of wait_for_contents and wait_for_text sounds good, and keeping the code for magic conversion of text and image data shared in nsClipboard::GetData() is
also good.  nsClipboard::HasDataMatchingFlavors() should do something similar
for consistency and code sharing.  A nsRetrievalContext::GetTargets() method
returning an array or list of target GdkAtoms should make
nsRetrievalContext::HasDataMatchingFlavors() unnecessary.

Renaming GetClipboardContent to WaitForClipboardContext would be more explicit
about what is happening.

One difference between the use of the existing RetrievalContext and the new
nsRetrievalContext is that the former was created for each retrieval.  That
provided sane behavior through multiple contexts when
gtk_clipboard_request_contents() times out in which case there could be a new
context when gtk_clipboard_request_contents() is called again.
Would such a model be practical with Wayland or is it valuable to keep a
singleton context.  If the singleton model is better, then different user data
will need to be passed to each gtk_clipboard_request_contents() call.

Does Store() need to be on the retrieval context?  It would be nice to drop
nsRetrievalContext's nsISupports inheritance for nsIObserver.
The ref-counting could probably also be dropped if a singleton.
nsClipboard::Store() is not used elsewhere.

XPCOM interfaces are best used only when JS might need access to the objects.
In other situations their use generally makes code more complicated than
necessary.  I suspect using allocated arrays and/or glib helpers would be much
simpler here than nsIInputStream.

Can you point us to the relevant documentation for the Wayland client library
and processes here, please?  I'm having trouble finding documentation for
wl_data_offer_receive, for example.  I think I might still need someone else
to review the Wayland code though.

Comment 8

[u575260]

ftw mozilla 2017!
https://bugzilla.mozilla.org/show_bug.cgi?id=635134

Comment 9

[Martin Stránský [:stransky] (ni? me)]

Thanks for the feedback. 

I uploaded the path to https://github.com/stransky/gecko-dev and it's already in https://firefox-flatpak.mojefedora.cz/org.mozilla.FirefoxNightlyWayland.flatpak.

Comment 10

[Martin Stránský [:stransky] (ni? me)]

> One difference between the use of the existing RetrievalContext and the new
> nsRetrievalContext is that the former was created for each retrieval.  That
> provided sane behavior through multiple contexts when
> gtk_clipboard_request_contents() times out in which case there could be a new
> context when gtk_clipboard_request_contents() is called again.
> Would such a model be practical with Wayland or is it valuable to keep a
> singleton context.  If the singleton model is better, then different user
> data
> will need to be passed to each gtk_clipboard_request_contents() call.

This confuses me a bit. How does help to have a different nsRetrievalContext()/user data for each gtk_clipboard_request_contents() call when the wait_for_contents() is actually synchronous and doesn't quit until the recent gtk_clipboard_request_contents() is finished - that's ensured by the context->Wait() call.

https://dxr.mozilla.org/mozilla-central/rev/f41930a869a84af81df1a88d8e82323ff3a6509a/widget/gtk/nsClipboard.cpp#1003

> Can you point us to the relevant documentation for the Wayland client library
> and processes here, please?  I'm having trouble finding documentation for
> wl_data_offer_receive, for example.  I think I might still need someone else
> to review the Wayland code though.

Jonas Ådahl promised to look at it.

Comment 11

[Martin Stránský [:stransky] (ni? me)]

Raising priority as it's a blocker for Firefox Wayland build.

Comment 12

[Karl Tomlinson (:karlt)]

(In reply to Martin Stránský [:stransky] from comment #10)
> > One difference between the use of the existing RetrievalContext and the
> > new nsRetrievalContext is that the former was created for each retrieval.
> > That provided sane behavior through multiple contexts when
> > gtk_clipboard_request_contents() times out in which case there could be a
> > new context when gtk_clipboard_request_contents() is called again.  Would
> > such a model be practical with Wayland or is it valuable to keep a
> > singleton context.  If the singleton model is better, then different user
> > data will need to be passed to each gtk_clipboard_request_contents() call.
> 
> This confuses me a bit. How does help to have a different
> nsRetrievalContext()/user data for each gtk_clipboard_request_contents()
> call when the wait_for_contents() is actually synchronous and doesn't quit
> until the recent gtk_clipboard_request_contents() is finished - that's
> ensured by the context->Wait() call.

Wait() can timeout before clipboard_contents_received() is called in response
to the gtk_clipboard_request_contents() call.

That permits the following order of events, for example

1. First WaitForContents() calls gtk_clipboard_request_contents().
2. First Wait() call times out.
3. First WaitForContents() returns null.
4. Second WaitForContents() calls gtk_clipboard_request_contents().
5. Second Wait() call waits.
6. clipboard_contents_received() is called in response to first
   gtk_clipboard_request_contents().
7. Second WaitForContents() returns data intended for first WaitForContents().

Comment 13

[Martin Stránský [:stransky] (ni? me)]

You're right, I didn't realize that the waiting clipboard_contents_received() can be delivered from running glib main loop and not only from our dedicated Wait() loop.

Comment 14

[Martin Stránský [:stransky] (ni? me)]

(In reply to Karl Tomlinson (:karlt) from comment #7)
> One difference between the use of the existing RetrievalContext and the new
> nsRetrievalContext is that the former was created for each retrieval.  That
> provided sane behavior through multiple contexts when
> gtk_clipboard_request_contents() times out in which case there could be a new
> context when gtk_clipboard_request_contents() is called again.
> Would such a model be practical with Wayland or is it valuable to keep a
> singleton context.  If the singleton model is better, then different user
> data
> will need to be passed to each gtk_clipboard_request_contents() call.

I think it's better to keep the singleton context and pass new data to gtk_clipboard_request_contents() as we don't need such thing for Wayland.
I'm going to update the patch at Bug 1417874.

Comment 15

[Martin Stránský [:stransky] (ni? me)]

Fixed by Bug 1417890 now.