[Wayland] Implement selection (middle button) clipboard paste

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: stransky, Assigned: stransky)

Tracking

(Blocks 1 bug)

Trunk
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed, firefox61 fixed)

Details

()

Attachments

(2 attachments)

Wayland does not provide selection clipboard but GDK emulates it. Let's see if we can also enable that in Firefox/Wayland port.
Assignee

Updated

2 years ago
Blocks: wayland
No longer blocks: 1282015
Priority: -- → P3
Assignee

Updated

Last year
Assignee: nobody → stransky
We need to implement "only" paste, selection copy works fine.
Summary: [Wayland] Implement selection (middle button) clipboard → [Wayland] Implement selection (middle button) clipboard paste
Assignee

Updated

Last year
Priority: P3 → P2

Comment 4

Last year
mozreview-review
Comment on attachment 8956423 [details]
Bug 1417892 - Added gtk-primary-selection-client-protocol header/source files from Gtk+ project,

https://reviewboard.mozilla.org/r/225318/#review233774

::: commit-message-51200:1
(Diff revision 1)
> +Bug 1417892 - Added gtk-primary-selection-client-protocol header/source files from Gtk+ project, r?jhorak

Please file a bug on Gnome Bugzilla with proposal to make this exported and add the link to it to this bug.
Attachment #8956423 - Flags: review?(jhorak) → review+

Comment 5

Last year
mozreview-review
Comment on attachment 8956424 [details]
Bug 1417892 - Implement primary clipboard selection under Wayland,

https://reviewboard.mozilla.org/r/225320/#review233802

::: widget/gtk/nsClipboardWayland.cpp:161
(Diff revision 1)
>  
> +void
> +nsRetrievalContextWayland::SetPrimaryDataOffer(
> +    gtk_primary_selection_offer *aPrimaryDataOffer)
> +{
> +    mPrimaryDataOffer = aPrimaryDataOffer;

Free mPrimaryDataOffer if set previously.
Attachment #8956424 - Flags: review?(jhorak)
Assignee

Comment 6

Last year
mozreview-review-reply
Comment on attachment 8956423 [details]
Bug 1417892 - Added gtk-primary-selection-client-protocol header/source files from Gtk+ project,

https://reviewboard.mozilla.org/r/225318/#review233774

> Please file a bug on Gnome Bugzilla with proposal to make this exported and add the link to it to this bug.

Filed as https://gitlab.gnome.org/GNOME/gtk/issues/92
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

Last year
Blocks: 1438131
Assignee

Updated

Last year
Blocks: 1449490
Assignee

Comment 10

Last year
mozreview-review
Comment on attachment 8956424 [details]
Bug 1417892 - Implement primary clipboard selection under Wayland,

https://reviewboard.mozilla.org/r/225320/#review237460

::: widget/gtk/nsClipboardWayland.cpp:239
(Diff revision 3)
> +    nsDataOffer* dataOffer =
> +        static_cast<nsDataOffer*>(g_hash_table_lookup(mActiveOffers,
> +                                                      aWaylandDataOffer));
> +    NS_ASSERTION(dataOffer, "We're missing clipboard dat offer!");
> +    if (dataOffer) {
> +        g_hash_table_remove(mActiveOffers, dataOffer);

I have to use g_hash_table_remove(...,aWaylandDataOffer) here instead of dataOffer.
Assignee

Comment 11

Last year
mozreview-review
Comment on attachment 8956424 [details]
Bug 1417892 - Implement primary clipboard selection under Wayland,

https://reviewboard.mozilla.org/r/225320/#review237464

::: widget/gtk/nsClipboardWayland.cpp:341
(Diff revision 3)
>  
>  static void
> +primary_selection_data_offer (void                                *data,
> +                              struct gtk_primary_selection_device *gtk_primary_selection_device,
> +                              struct gtk_primary_selection_offer  *gtk_primary_offer)
> +{

gtk_primary_offer can be null here which means we need to remove all primary offers we have.
Assignee

Comment 12

Last year
mozreview-review
Comment on attachment 8956424 [details]
Bug 1417892 - Implement primary clipboard selection under Wayland,

https://reviewboard.mozilla.org/r/225320/#review237470

::: widget/gtk/nsClipboardWayland.h:87
(Diff revision 3)
>      wl_display                 *mDisplay;
>      wl_seat                    *mSeat;
>      wl_data_device_manager     *mDataDeviceManager;
> -    wl_data_offer              *mDataOffer;
> +    gtk_primary_selection_device_manager *mPrimarySelectionDataDeviceManager;
>      wl_keyboard                *mKeyboard;
> -    nsTArray<GdkAtom>           mTargetMIMETypes;
> +    bool                        mKeyboardFocus;

mKeyboardFocus is unused.

Comment 13

Last year
mozreview-review
Comment on attachment 8956424 [details]
Bug 1417892 - Implement primary clipboard selection under Wayland,

https://reviewboard.mozilla.org/r/225320/#review237456

::: widget/gtk/nsClipboardWayland.h:20
(Diff revision 3)
>  #include <gdk/gdkwayland.h>
>  #include <nsTArray.h>
>  
>  struct FastTrackClipboard;
>  
> +typedef enum {

This seems to be unused in later code.

::: widget/gtk/nsClipboardWayland.h:25
(Diff revision 3)
> +typedef enum {
> +    DATA_OFFER_CLIPBOARD,
> +    DATA_OFFER_PRIMARY
> +} DataOfferType;
> +
> +class nsDataOffer

Don't add ns prefix if the class does not implement nsISupport.

::: widget/gtk/nsClipboardWayland.h:31
(Diff revision 3)
> +    wl_data_offer*                GetWaylandOffer();
> +    gtk_primary_selection_offer*  GetPrimaryOffer();

Both methods seems to be unused.

::: widget/gtk/nsClipboardWayland.h:44
(Diff revision 3)
> +    wl_data_offer*                mWaylandDataOffer;
> +    gtk_primary_selection_offer*  mPrimaryDataOffer;

This is rather strange, you put multiple variables into the class but you're using only one of them in the instance. 

Making a parent class which defines the common interface and implement a subclass for WaylandDataOffer, PrimaryDataOffer (and possibly upcoming DragAndDropData offer) would be more natural.

::: widget/gtk/nsClipboardWayland.cpp:85
(Diff revision 3)
> +    if (mPrimaryDataOffer) {
> +        gtk_primary_selection_offer_receive(mPrimaryDataOffer, aMimeType,
> +                                            pipe_fd[1]);
> +    } else {
> +        wl_data_offer_receive(mWaylandDataOffer, aMimeType, pipe_fd[1]);
> +    }

This could be implemented by the subclasses of DataOffer.

::: widget/gtk/nsClipboardWayland.cpp:209
(Diff revision 3)
> +  nsDataOffer* dataOffer =
> +      static_cast<nsDataOffer*>(g_hash_table_lookup(mActiveOffers,
> +                                                    aWaylandDataOffer));

You could extract this to nsRetrievalContext::FindDataOfferInHashtable(void * key) returning (ns)DataOffer. This can be reused in all RegisterDataOffer and Set*DataOffer methods.

::: widget/gtk/nsClipboardWayland.cpp:232
(Diff revision 3)
> +      g_hash_table_insert(mActiveOffers, aPrimaryDataOffer, dataOffer);
> +  }
> +}
> +
> +void
> +nsRetrievalContextWayland::SetClipboardDataOffer(wl_data_offer *aWaylandDataOffer)

You are testing a

::: widget/gtk/nsClipboardWayland.cpp:232
(Diff revision 3)
> +      g_hash_table_insert(mActiveOffers, aPrimaryDataOffer, dataOffer);
> +  }
> +}
> +
> +void
> +nsRetrievalContextWayland::SetClipboardDataOffer(wl_data_offer *aWaylandDataOffer)

You are testing a

::: widget/gtk/nsClipboardWayland.cpp:248
(Diff revision 3)
> +
> +void
> +nsRetrievalContextWayland::SetPrimaryDataOffer(
> +      gtk_primary_selection_offer *aPrimaryDataOffer)
> +{
> +    if (aPrimaryDataOffer == nullptr) {

You're testing aPrimaryDataOffer for not being null. Is this also  required for SetClipboardDataOffer?
Attachment #8956424 - Flags: review?(jhorak)
Assignee

Comment 14

Last year
mozreview-review-reply
Comment on attachment 8956424 [details]
Bug 1417892 - Implement primary clipboard selection under Wayland,

https://reviewboard.mozilla.org/r/225320/#review237456

> You're testing aPrimaryDataOffer for not being null. Is this also  required for SetClipboardDataOffer?

No, ClipboardDataOffer can't be null, there's always a clipboard content available. aPrimaryDataOffer can be null because the selection can be removed (emptied).
Assignee

Comment 15

Last year
mozreview-review-reply
Comment on attachment 8956424 [details]
Bug 1417892 - Implement primary clipboard selection under Wayland,

https://reviewboard.mozilla.org/r/225320/#review237456

> You could extract this to nsRetrievalContext::FindDataOfferInHashtable(void * key) returning (ns)DataOffer. This can be reused in all RegisterDataOffer and Set*DataOffer methods.

I preffer to keep this as it's because the g_hash_table_lookup() is paired with g_hash_table_insert/remove.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 18

Last year
mozreview-review
Comment on attachment 8956424 [details]
Bug 1417892 - Implement primary clipboard selection under Wayland,

https://reviewboard.mozilla.org/r/225320/#review239172

::: widget/gtk/nsClipboardWayland.cpp:250
(Diff revision 4)
> +nsRetrievalContextWayland::SetClipboardDataOffer(wl_data_offer *aWaylandDataOffer)
> +{
> +    DataOffer* dataOffer =
> +        static_cast<DataOffer*>(g_hash_table_lookup(mActiveOffers,
> +                                                    aWaylandDataOffer));
> +    NS_ASSERTION(dataOffer, "We're missing clipboard dat offer!");

typo: s/dat /data /

::: widget/gtk/nsClipboardWayland.cpp:268
(Diff revision 4)
> +        mPrimaryOffer = nullptr;
> +    } else {
> +        DataOffer* dataOffer =
> +            static_cast<DataOffer*>(g_hash_table_lookup(mActiveOffers,
> +                                                        aPrimaryDataOffer));
> +        NS_ASSERTION(dataOffer, "We're missing primary dat offer!");

dtto

::: widget/gtk/nsClipboardWayland.cpp:271
(Diff revision 4)
> +            static_cast<DataOffer*>(g_hash_table_lookup(mActiveOffers,
> +                                                        aPrimaryDataOffer));
> +        NS_ASSERTION(dataOffer, "We're missing primary dat offer!");
> +        if (dataOffer) {
> +            g_hash_table_remove(mActiveOffers, aPrimaryDataOffer);
> +            mPrimaryOffer = new PrimaryDataOffer(aPrimaryDataOffer);

It's unclear why you create a new instance here while you reuse dataOffer in the nsRetrievalContextWayland::SetClipboardDataOffer implementation.
Attachment #8956424 - Flags: review?(jhorak)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 21

Last year
mozreview-review
Comment on attachment 8956424 [details]
Bug 1417892 - Implement primary clipboard selection under Wayland,

https://reviewboard.mozilla.org/r/225320/#review239200

::: widget/gtk/nsClipboardWayland.cpp:571
(Diff revision 4)
>      mInitialized = true;
>  }
>  
>  nsRetrievalContextWayland::~nsRetrievalContextWayland(void)
>  {
> -    g_free(mTextPlainLocale);
> +    g_hash_table_destroy(mActiveOffers);

You should free all elements in the hash_table. They should not contain any, but anyway [1]
[1] https://developer.gnome.org/glib/stable/glib-Hash-Tables.html#g-hash-table-destroy

Alternatively use g_hash_table_new_full to create hash table with the destroy function or at least add assert for g_hash_table_size() == 0 there that we can check we're not leaking memory there.
Assignee

Comment 22

Last year
mozreview-review-reply
Comment on attachment 8956424 [details]
Bug 1417892 - Implement primary clipboard selection under Wayland,

https://reviewboard.mozilla.org/r/225320/#review239200

> You should free all elements in the hash_table. They should not contain any, but anyway [1]
> [1] https://developer.gnome.org/glib/stable/glib-Hash-Tables.html#g-hash-table-destroy
> 
> Alternatively use g_hash_table_new_full to create hash table with the destroy function or at least add assert for g_hash_table_size() == 0 there that we can check we're not leaking memory there.

This is already a part of patch for Bug 1438131 (Wayland drop implementation). I'll paste it there.

Comment 23

Last year
mozreview-review
Comment on attachment 8956424 [details]
Bug 1417892 - Implement primary clipboard selection under Wayland,

https://reviewboard.mozilla.org/r/225320/#review239230
Attachment #8956424 - Flags: review?(jhorak) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 30

Last year
Pushed by stransky@redhat.com:
https://hg.mozilla.org/integration/autoland/rev/9df9a893c30b
Added gtk-primary-selection-client-protocol header/source files from Gtk+ project, r=jhorak
https://hg.mozilla.org/integration/autoland/rev/82d43a8809c2
Implement primary clipboard selection under Wayland, r=jhorak

Comment 31

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/9df9a893c30b
https://hg.mozilla.org/mozilla-central/rev/82d43a8809c2
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8956423 [details]
Bug 1417892 - Added gtk-primary-selection-client-protocol header/source files from Gtk+ project,

Approval Request Comment
[Feature/Bug causing the regression]: none (it's implementing missing functionality)
[User impact if declined]: Non working clipboard on Wayland, non functional wayland builds as we need that for Bug 1449490.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: none
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: Mozilla does not build/ship this code, it's used by Linux distros only to build Wayland-enabled builds.
[String changes made/needed]: none
Attachment #8956423 - Flags: approval-mozilla-beta?
Comment on attachment 8956424 [details]
Bug 1417892 - Implement primary clipboard selection under Wayland,

Approval Request Comment
[Feature/Bug causing the regression]: none (it's implementing missing functionality)
[User impact if declined]: Non working clipboard on Wayland, non functional wayland builds as we need it for Bug 1449490.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: none
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: Mozilla does not build/ship this code, it's used by Linux distros only to build Wayland-enabled builds.
[String changes made/needed]: none
Attachment #8956424 - Flags: approval-mozilla-beta?
This feels like feature work, not something we should be uplifting last minute.
(In reply to Julien Cristau [:jcristau] from comment #34)
> This feels like feature work, not something we should be uplifting last
> minute.

It's actually a missing basic functionality, this is something what works on Gtk/Linux for ages and it's broken on Wayland builds. But anyway, I request the uplift mainly because Bug 1449490.
This is very low risk as this code is not built by mozilla but it's used by distros - it would be great to have it in ESR60.
Comment on attachment 8956424 [details]
Bug 1417892 - Implement primary clipboard selection under Wayland,

NPOTB, used by other distros, Beta60+
Attachment #8956424 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8956423 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.