Martin Stránský [:stransky] (ni? me) Assignee
	Description • 6 years ago

Copy/paste between Firefox windows does not work under Wayland.

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 1 • 6 years ago

Looks like we need to do the copy->paste completely without Gtk+ in this case, as the paste is blocking and we can't supply the data then.

	Jan Horak [:jhorak]
	Comment 4 • 6 years ago
mozreview-review

Comment on attachment 8947399 [details]
Bug 1434572 - [Wayland] Implement Gtk+ clipboard shortcut for copy->paste between Firefox windows,

https://reviewboard.mozilla.org/r/217118/#review223308

::: widget/gtk/nsClipboardWayland.cpp:383
(Diff revision 1)
> +     * getter callback nsClipboard::SelectionGetEvent().
> +     */
> +    GdkAtom selection = GetSelectionAtom(aWhichClipboard);
> +    if (gdk_selection_owner_get(selection)) {
> +        fastTrackClipboardData clipboardData = { nullptr, 0 };
> +        gtk_clipboard_request_contents(gtk_clipboard_get(selection),

It's not clear that this is synchronous call (when the callback is passed), because if it isn't, then you're losing scope to clipboardData. Looking to the gtk clipboard sources it would be better to use gtk_clipboard_wait_for_contents [1] and do the copy in this method to avoid unpleasent surprise when gtk decide to make gtk_clipboard_request_contents really async.

[1] https://developer.gnome.org/gtk3/stable/gtk3-Clipboards.html#gtk-clipboard-wait-for-contents

	Jan Horak [:jhorak]
	Comment 6 • 6 years ago
mozreview-review

Comment on attachment 8947399 [details]
Bug 1434572 - [Wayland] Implement Gtk+ clipboard shortcut for copy->paste between Firefox windows,

https://reviewboard.mozilla.org/r/217118/#review225626

::: widget/gtk/nsClipboardWayland.cpp:366
(Diff revisions 1 - 2)
>  wayland_clipboard_contents_received(GtkClipboard     *clipboard,
>                                      GtkSelectionData *selection_data,
>                                      gpointer          data)
>  {
> -    fastTrackClipboardData* clipboardData =
> -        static_cast<fastTrackClipboardData*>(data);
> +    FastTrackClipboard* fastTrack = static_cast<FastTrackClipboard*>(data);
> +    nsRetrievalContextWayland* context = fastTrack->mRetrievalContex;

This can be shortened as:
`fastTrack->mRetrievalContex->TransferFastTrackClipboard(...)`

::: widget/gtk/nsClipboardWayland.cpp:367
(Diff revisions 1 - 2)
>                                      GtkSelectionData *selection_data,
>                                      gpointer          data)
>  {
> -    fastTrackClipboardData* clipboardData =
> -        static_cast<fastTrackClipboardData*>(data);
> -
> +    FastTrackClipboard* fastTrack = static_cast<FastTrackClipboard*>(data);
> +    nsRetrievalContextWayland* context = fastTrack->mRetrievalContex;
> +    context->TransferFastTrackClipboard(fastTrack, selection_data);

No need to pass FastTracklipboard struct pointer when only request number in TransferFastTrackClipboard is used.

::: widget/gtk/nsClipboardWayland.cpp:384
(Diff revisions 1 - 2)
> +            mClipboardData = reinterpret_cast<char*>(
> +                g_malloc(sizeof(char)*mClipboardDataLength));
> +            memcpy(mClipboardData, gtk_selection_data_get_data(aSelectionData),
> +                   sizeof(char)*mClipboardDataLength);
> +        }
> +    }

Maybe add some warning for case the request numbers does not match?

::: widget/gtk/nsClipboardWayland.cpp:445
(Diff revisions 1 - 2)
> -    gsize  dataLength = 0;
>  
> -    g_io_channel_set_encoding(channel, nullptr, &error);
> +        g_io_channel_set_encoding(channel, nullptr, &error);
> -    if (!error) {
> +        if (!error) {
> -        g_io_channel_read_to_end(channel, &clipboardData, &dataLength, &error);
> +            gsize length = 0;
> +            g_io_channel_read_to_end(channel, &mClipboardData, &length, &error);

Can't we match length and mClipboardDataLength types and use mClipboarDataLength directly?

	Martin Stránský [:stransky] (ni? me) Assignee
	Comment 7 • 6 years ago
mozreview-review-reply

Comment on attachment 8947399 [details]
Bug 1434572 - [Wayland] Implement Gtk+ clipboard shortcut for copy->paste between Firefox windows,

https://reviewboard.mozilla.org/r/217118/#review225626

> Can't we match length and mClipboardDataLength types and use mClipboarDataLength directly?

No, g_io_channel_read_to_end() uses int64* for size but our clipboard data length is int32.

	Jan Horak [:jhorak]
	Comment 10 • 6 years ago
mozreview-review

Comment on attachment 8947399 [details]
Bug 1434572 - [Wayland] Implement Gtk+ clipboard shortcut for copy->paste between Firefox windows,

https://reviewboard.mozilla.org/r/217118/#review227142

	Pulsebot
	Comment 11 • 6 years ago

Pushed by stransky@redhat.com:
https://hg.mozilla.org/integration/autoland/rev/1f5f5fe0abea
[Wayland] Implement Gtk+ clipboard shortcut for copy->paste between Firefox windows, r=jhorak

	Natalia Csoregi [:nataliaCs]
	Comment 12 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/1f5f5fe0abea