Closed Bug 1417890 Opened 2 years ago Closed 2 years ago

Implement Wayland nsClipboard backend

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: stransky, Assigned: stransky)

References

Details

Attachments

(4 files)

Implement Wayland nsClipboard backend
Blocks: 1417892
Comment on attachment 8937432 [details]
Bug 1417890 - rename nsRetrievalContext::WaitForClipboardContext() to nsRetrievalContext::GetClipboardData() and implement nsRetrievalContext::ReleaseClipboardData(),

https://reviewboard.mozilla.org/r/208096/#review215376

::: widget/gtk/nsClipboard.h:24
(Diff revision 1)
>  public:
> -    // Returned data must be released by free()
> +    virtual const char* GetClipboardData(const char* aMimeType,
> -    virtual const char* WaitForClipboardContext(const char* aMimeType,
> -                                                int32_t aWhichClipboard,
> +                                         int32_t aWhichClipboard,
> -                                                uint32_t* aContentLength) = 0;
> +                                         uint32_t* aContentLength) = 0;
> +    virtual void ReleaseClipboardData(const char* aClipboardData) = 0;

Does the ReleaseClipboardData has to be virutal there? Isn't the implementation is the same for X11 and wayland?

I see the idea behind that you want to manage memory better, question is if this needs to be method of nsRetrievalContext as long as it does not manipulate with object instance.

::: widget/gtk/nsClipboard.cpp:311
(Diff revision 1)
>                  continue;
>              }
>  
>              // Convert utf-8 into our unicode format.
> -            NS_ConvertUTF8toUTF16 ucs2string(rawData, clipboardDataLength);
> -            const char* clipboardData = (const char *)ToNewUnicode(ucs2string);
> +            NS_ConvertUTF8toUTF16 ucs2string(clipboardData, clipboardDataLength);
> +            const char* unicodeData = (const char *)ToNewUnicode(ucs2string);

Free the clipboardData as soon as it is not needed anymore.

::: widget/gtk/nsClipboard.cpp:337
(Diff revision 1)
>                                    &htmlBody, htmlBodyLen);
> -                free((void *)clipboardData);
>  
>                  // Try next data format?
>                  if (!htmlBodyLen)
>                      continue;

It is possible that you're leaking clipboardData here.
Attachment #8937432 - Flags: review?(jhorak) → review-
Comment on attachment 8937433 [details]
Bug 1417890 - Implement nsRetrievalContextWayland,

https://reviewboard.mozilla.org/r/208098/#review214846

::: widget/gtk/nsClipboardWayland.cpp:367
(Diff revision 1)
> +    }
> +
> +    GArray* dataStream = g_array_new(false, false, sizeof(guchar));
> +    int length;
> +
> +    #define BUFFER_SIZE 4096

If you prefer to use glib2 for the reading, check the https://developer.gnome.org/glib/stable/glib-IO-Channels.html object and its reading method g_io_channel_read_to_end ().

::: widget/gtk/nsClipboardWayland.cpp:207
(Diff revision 2)
> +
> +void
> +nsRetrievalContextWayland::ConfigureKeyboard(wl_seat_capability caps)
> +{
> +  if (caps & WL_SEAT_CAPABILITY_KEYBOARD) {
> +      mKeyboard = wl_seat_get_keyboard(mSeat);

You most likely need to call wl_keyboard_destroy(mKeyboard) somewhere if this succeed.

::: widget/gtk/nsClipboardWayland.cpp:358
(Diff revision 2)
> +    struct pollfd fds;
> +    fds.fd = pipe_fd[0];
> +    fds.events = POLLIN;
> +
> +    // Choose some reasonable timeout here
> +    int ret = poll(&fds, 1, kClipboardTimeout*1000);

The poll() timeout is in milliseconds, so with kClipboardTimeout = 500000 we would wait there for 500 seconds, that's too much :). Better to divide by 1000.

::: widget/gtk/nsClipboardWayland.cpp:375
(Diff revision 2)
> +    if (!error) {
> +        g_io_channel_read_to_end(channel, &clipboardData, &dataLength, &error);
> +    }
> +
> +    if (error) {
> +        g_error_free(error);

please add something like:
NS_WARNING(nsPrintfCString("Unexpected error when reading clipboard data: %s", error->message));
to keep us informed.
Attachment #8937433 - Flags: review?(jhorak) → review-
Comment on attachment 8937434 [details]
Bug 1417890 - Use nsRetrievalContextWayland for Wayland displays,

https://reviewboard.mozilla.org/r/208100/#review215382

::: widget/gtk/nsClipboardWayland.h:24
(Diff revision 3)
>      nsRetrievalContextWayland();
>  
>      virtual const char* GetClipboardData(const char* aMimeType,
>                                           int32_t aWhichClipboard,
>                                           uint32_t* aContentLength) override;
> -    virtual void ReleaseClipboardData(const char* aClipboardData);
> +    virtual void ReleaseClipboardData(const char* aClipboardData) override;

Please move the implementation to the nsRetrievalContext class since it is has the same implementation in X11 and Wayland.

::: widget/gtk/nsClipboardX11.h:22
(Diff revision 3)
>      enum State { INITIAL, COMPLETED, TIMED_OUT };
>  
>      virtual const char* GetClipboardData(const char* aMimeType,
>                                           int32_t aWhichClipboard,
>                                           uint32_t* aContentLength) override;
> -    virtual void ReleaseClipboardData(const char* aClipboardData);
> +    virtual void ReleaseClipboardData(const char* aClipboardData) override;

dtto
Attachment #8937434 - Flags: review?(jhorak) → review-
Comment on attachment 8937435 [details]
Bug 1417890 - Install nsClipboard hook at nsWindow init,

https://reviewboard.mozilla.org/r/208102/#review215384
Attachment #8937435 - Flags: review?(jhorak) → review+
Comment on attachment 8937432 [details]
Bug 1417890 - rename nsRetrievalContext::WaitForClipboardContext() to nsRetrievalContext::GetClipboardData() and implement nsRetrievalContext::ReleaseClipboardData(),

https://reviewboard.mozilla.org/r/208096/#review215766

::: widget/gtk/nsClipboard.h:24
(Diff revision 1)
>  public:
> -    // Returned data must be released by free()
> +    virtual const char* GetClipboardData(const char* aMimeType,
> -    virtual const char* WaitForClipboardContext(const char* aMimeType,
> -                                                int32_t aWhichClipboard,
> +                                         int32_t aWhichClipboard,
> -                                                uint32_t* aContentLength) = 0;
> +                                         uint32_t* aContentLength) = 0;
> +    virtual void ReleaseClipboardData(const char* aClipboardData) = 0;

ReleaseClipboardData() uses different memory release methods (free() / g_free()) due to wayland specific implementation.
Comment on attachment 8937432 [details]
Bug 1417890 - rename nsRetrievalContext::WaitForClipboardContext() to nsRetrievalContext::GetClipboardData() and implement nsRetrievalContext::ReleaseClipboardData(),

https://reviewboard.mozilla.org/r/208096/#review215772

::: widget/gtk/nsClipboard.cpp:311
(Diff revision 1)
>                  continue;
>              }
>  
>              // Convert utf-8 into our unicode format.
> -            NS_ConvertUTF8toUTF16 ucs2string(rawData, clipboardDataLength);
> -            const char* clipboardData = (const char *)ToNewUnicode(ucs2string);
> +            NS_ConvertUTF8toUTF16 ucs2string(clipboardData, clipboardDataLength);
> +            const char* unicodeData = (const char *)ToNewUnicode(ucs2string);

After discussions se decided to leave it as it is.
Comment on attachment 8937433 [details]
Bug 1417890 - Implement nsRetrievalContextWayland,

https://reviewboard.mozilla.org/r/208098/#review215776

::: widget/gtk/nsClipboardWayland.cpp:207
(Diff revision 2)
> +
> +void
> +nsRetrievalContextWayland::ConfigureKeyboard(wl_seat_capability caps)
> +{
> +  if (caps & WL_SEAT_CAPABILITY_KEYBOARD) {
> +      mKeyboard = wl_seat_get_keyboard(mSeat);

wl_keyboard_destroy(mKeyboard) is called when we lost keyboard at nsRetrievalContextWayland::ConfigureKeyboard().

when seat_handle_capabilities(wl_seat_capability ...) callback is called without WL_SEAT_CAPABILITY_KEYBOARD flag it means we lost it and it's released.
Comment on attachment 8937434 [details]
Bug 1417890 - Use nsRetrievalContextWayland for Wayland displays,

https://reviewboard.mozilla.org/r/208100/#review215782

::: widget/gtk/nsClipboardWayland.h:24
(Diff revision 3)
>      nsRetrievalContextWayland();
>  
>      virtual const char* GetClipboardData(const char* aMimeType,
>                                           int32_t aWhichClipboard,
>                                           uint32_t* aContentLength) override;
> -    virtual void ReleaseClipboardData(const char* aClipboardData);
> +    virtual void ReleaseClipboardData(const char* aClipboardData) override;

We can't do that as we have different memory release methods for wayland/X11.
Priority: -- → P2
Comment on attachment 8937432 [details]
Bug 1417890 - rename nsRetrievalContext::WaitForClipboardContext() to nsRetrievalContext::GetClipboardData() and implement nsRetrievalContext::ReleaseClipboardData(),

https://reviewboard.mozilla.org/r/208096/#review217738
Attachment #8937432 - Flags: review?(jhorak) → review+
Comment on attachment 8937433 [details]
Bug 1417890 - Implement nsRetrievalContextWayland,

https://reviewboard.mozilla.org/r/208098/#review217740
Attachment #8937433 - Flags: review?(jhorak) → review+
Comment on attachment 8937434 [details]
Bug 1417890 - Use nsRetrievalContextWayland for Wayland displays,

https://reviewboard.mozilla.org/r/208100/#review217742
Attachment #8937434 - Flags: review?(jhorak) → review+
Pushed by stransky@redhat.com:
https://hg.mozilla.org/integration/autoland/rev/8406e5c06039
rename nsRetrievalContext::WaitForClipboardContext() to nsRetrievalContext::GetClipboardData() and implement nsRetrievalContext::ReleaseClipboardData(), r=jhorak
https://hg.mozilla.org/integration/autoland/rev/7269882141b8
Implement nsRetrievalContextWayland, r=jhorak
https://hg.mozilla.org/integration/autoland/rev/86b56bad2991
Use nsRetrievalContextWayland for Wayland displays, r=jhorak
https://hg.mozilla.org/integration/autoland/rev/2894b6be0c15
Install nsClipboard hook at nsWindow init, r=jhorak
Depends on: 1500484
You need to log in before you can comment on or make changes to this bug.