[Wayland] copy -> paste between Firefox windows does not work

RESOLVED FIXED in Firefox 60

Status

()

defect
P2
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: stransky, Assigned: stransky)

Tracking

(Blocks 1 bug)

Trunk
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment)

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

Comment 1

a year 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.
Summary: [Wayland] copy -> paste from Firefox to Firefox does not work → [Wayland] copy -> paste between Firefox windows does not work
Comment hidden (mozreview-request)
Assignee

Updated

a year ago
Duplicate of this bug: 1396101

Comment 4

a year 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
Attachment #8947399 - Flags: review?(jhorak) → review-
Comment hidden (mozreview-request)

Comment 6

a year 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?
Attachment #8947399 - Flags: review?(jhorak) → review-
Assignee

Comment 7

a year 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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

a year 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
Attachment #8947399 - Flags: review?(jhorak) → review+

Comment 11

a year 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

Comment 12

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1f5f5fe0abea
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.