Closed
Bug 1417890
Opened 7 years ago
Closed 7 years ago
Implement Wayland nsClipboard backend
Categories
(Core :: Widget: Gtk, enhancement, P2)
Core
Widget: Gtk
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: stransky, Assigned: stransky)
References
Details
Attachments
(4 files)
Implement Wayland nsClipboard backend
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 10•7 years ago
|
||
mozreview-review |
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 11•7 years ago
|
||
mozreview-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 12•7 years ago
|
||
mozreview-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 13•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 14•7 years ago
|
||
mozreview-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.
Assignee | ||
Comment 15•7 years ago
|
||
mozreview-review |
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.
Assignee | ||
Comment 16•7 years ago
|
||
mozreview-review |
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.
Assignee | ||
Comment 17•7 years ago
|
||
mozreview-review |
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Priority: -- → P2
Comment 22•7 years ago
|
||
mozreview-review |
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 23•7 years ago
|
||
mozreview-review |
Comment on attachment 8937433 [details]
Bug 1417890 - Implement nsRetrievalContextWayland,
https://reviewboard.mozilla.org/r/208098/#review217740
Attachment #8937433 -
Flags: review?(jhorak) → review+
Comment 24•7 years ago
|
||
mozreview-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+
Comment 25•7 years ago
|
||
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
Comment 26•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8406e5c06039
https://hg.mozilla.org/mozilla-central/rev/7269882141b8
https://hg.mozilla.org/mozilla-central/rev/86b56bad2991
https://hg.mozilla.org/mozilla-central/rev/2894b6be0c15
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in
before you can comment on or make changes to this bug.
Description
•