Closed Bug 1417892 Opened 6 years ago Closed 6 years ago

[Wayland] Implement selection (middle button) clipboard paste

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: stransky, Assigned: stransky)

References

(Blocks 1 open bug, )

Details

Attachments

(2 files)

Wayland does not provide selection clipboard but GDK emulates it. Let's see if we can also enable that in Firefox/Wayland port.
Blocks: wayland
No longer blocks: 1282015
Priority: -- → P3
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
Priority: P3 → P2
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 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)
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 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.
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.
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 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)
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).
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 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 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.
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/9df9a893c30b
https://hg.mozilla.org/mozilla-central/rev/82d43a8809c2
Status: NEW → RESOLVED
Closed: 6 years ago
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.