Closed
Bug 1299083
Opened 8 years ago
Closed 8 years ago
[Wayland] - build config
Categories
(Core :: Widget: Gtk, enhancement, P5)
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: stransky, Assigned: stransky)
References
(Blocks 1 open bug)
Details
(Whiteboard: tpi:+)
Attachments
(1 file, 2 obsolete files)
We will need some Wayland specific code, at least for clipboard and Xremote implementation.
Let's add build time option to enable build/link against Wayland libraries.
Assignee | ||
Comment 1•8 years ago
|
||
Mike, I believe you're the right person here. Thanks.
Attachment #8786286 -
Flags: review?(mh+mozilla)
Comment 2•8 years ago
|
||
Comment on attachment 8786286 [details] [diff] [review]
patch
Review of attachment 8786286 [details] [diff] [review]:
-----------------------------------------------------------------
::: old-configure.in
@@ +2856,5 @@
> + MOZ_ENABLE_WAYLAND=)
> +
> + if test "$MOZ_ENABLE_WAYLAND"
> + then
> + PKG_CHECK_MODULES(MOZ_WAYLAND, wayland-client)
What do you need from wayland-client that can't be done in a display-agnostic way through gtk? Why wouldn't you improve gtk instead?
Either way, we don't add things to old.configure.in that can be done in python configure anymore. Ping me or chmanchester if you need guidance for that, but please consider not needing wayland-client at all first.
Attachment #8786286 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #2)
> Comment on attachment 8786286 [details] [diff] [review]
> patch
>
> Review of attachment 8786286 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> ::: old-configure.in
> @@ +2856,5 @@
> > + MOZ_ENABLE_WAYLAND=)
> > +
> > + if test "$MOZ_ENABLE_WAYLAND"
> > + then
> > + PKG_CHECK_MODULES(MOZ_WAYLAND, wayland-client)
>
> What do you need from wayland-client that can't be done in a
> display-agnostic way through gtk? Why wouldn't you improve gtk instead?
We need to use some X11/Wayland capabilities which are not provided (intentionally) by Gtk. One of them is synchronous clipboard.
I guess it's doable to load libwayland-client run-time but we need wayland headers to compile against.
Comment 4•8 years ago
|
||
> We need to use some X11/Wayland capabilities which are not provided (intentionally) by Gtk
This is getting offtopic here, but why? Gtk has X11-only features. I guess it has wayland-only features as well. Why intentionally reject support for something toolkit related across the different displays? Is the problem the "synchronous" part?
Comment 5•8 years ago
|
||
(In reply to Martin Stránský from comment #3)
> I guess it's doable to load libwayland-client run-time but we need wayland
> headers to compile against.
If you dlopen libwayland-client, you might as well define the types you need in the file that does the dlopen.
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #4)
> > We need to use some X11/Wayland capabilities which are not provided (intentionally) by Gtk
>
> This is getting offtopic here, but why? Gtk has X11-only features. I guess
> it has wayland-only features as well. Why intentionally reject support for
> something toolkit related across the different displays? Is the problem the
> "synchronous" part?
Yes, Gtk does not support "synchronous" clipboard intentionally. There's a X11 hack for it and we need something similar for Wayland.
Assignee | ||
Comment 7•8 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #5)
> (In reply to Martin Stránský from comment #3)
> > I guess it's doable to load libwayland-client run-time but we need wayland
> > headers to compile against.
>
> If you dlopen libwayland-client, you might as well define the types you need
> in the file that does the dlopen.
I don't understand. I need to include Wayland headers build time but dlopen() is a run-time thing, correct? How can I include Wayland headers and build Wayland specific code by such way?
Comment 8•8 years ago
|
||
(In reply to Martin Stránský from comment #6)
> (In reply to Mike Hommey [:glandium] from comment #4)
> > > We need to use some X11/Wayland capabilities which are not provided (intentionally) by Gtk
> >
> > This is getting offtopic here, but why? Gtk has X11-only features. I guess
> > it has wayland-only features as well. Why intentionally reject support for
> > something toolkit related across the different displays? Is the problem the
> > "synchronous" part?
>
> Yes, Gtk does not support "synchronous" clipboard intentionally. There's a
> X11 hack for it and we need something similar for Wayland.
That doesn't really answer the question.
(In reply to Martin Stránský from comment #7)
> (In reply to Mike Hommey [:glandium] from comment #5)
> > (In reply to Martin Stránský from comment #3)
> > > I guess it's doable to load libwayland-client run-time but we need wayland
> > > headers to compile against.
> >
> > If you dlopen libwayland-client, you might as well define the types you need
> > in the file that does the dlopen.
>
> I don't understand. I need to include Wayland headers build time but
> dlopen() is a run-time thing, correct? How can I include Wayland headers and
> build Wayland specific code by such way?
If you dlopen(), then you will need to define function types manually, you don't need headers for that. If there are also types to be defined, you might as well define them alongside, and not require headers at all.
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #8)
> (In reply to Martin Stránský from comment #6)
> > (In reply to Mike Hommey [:glandium] from comment #4)
> > > > We need to use some X11/Wayland capabilities which are not provided (intentionally) by Gtk
> > >
> > > This is getting offtopic here, but why? Gtk has X11-only features. I guess
> > > it has wayland-only features as well. Why intentionally reject support for
> > > something toolkit related across the different displays? Is the problem the
> > > "synchronous" part?
> >
> > Yes, Gtk does not support "synchronous" clipboard intentionally. There's a
> > X11 hack for it and we need something similar for Wayland.
>
> That doesn't really answer the question.
Sorry but I don't understand your question then.
> (In reply to Martin Stránský from comment #7)
> > (In reply to Mike Hommey [:glandium] from comment #5)
> > > (In reply to Martin Stránský from comment #3)
> > > > I guess it's doable to load libwayland-client run-time but we need wayland
> > > > headers to compile against.
> > >
> > > If you dlopen libwayland-client, you might as well define the types you need
> > > in the file that does the dlopen.
> >
> > I don't understand. I need to include Wayland headers build time but
> > dlopen() is a run-time thing, correct? How can I include Wayland headers and
> > build Wayland specific code by such way?
>
> If you dlopen(), then you will need to define function types manually, you
> don't need headers for that. If there are also types to be defined, you
> might as well define them alongside, and not require headers at all.
See wayland-client-protocol.h for instance, I use ~20% of the code here and I don't want to redefine them in Firefox:
https://people.freedesktop.org/~whot/wayland-doxygen/wayland/Client/wayland-client-protocol_8h_source.html
We can define the wl_proxy_marshal_constructor() and maybe some proxy calls to the library but not all the macros (wrappers and defines) from wayland headers.
Comment 10•8 years ago
|
||
(In reply to Martin Stránský from comment #9)
> (In reply to Mike Hommey [:glandium] from comment #8)
> > (In reply to Martin Stránský from comment #6)
> > > (In reply to Mike Hommey [:glandium] from comment #4)
> > > > > We need to use some X11/Wayland capabilities which are not provided (intentionally) by Gtk
> > > >
> > > > This is getting offtopic here, but why? Gtk has X11-only features. I guess
> > > > it has wayland-only features as well. Why intentionally reject support for
> > > > something toolkit related across the different displays? Is the problem the
> > > > "synchronous" part?
> > >
> > > Yes, Gtk does not support "synchronous" clipboard intentionally. There's a
> > > X11 hack for it and we need something similar for Wayland.
> >
> > That doesn't really answer the question.
>
> Sorry but I don't understand your question then.
Why does Gtk not support "synchronous" clipboard intentionally? And why shouldn't it be changed to support "synchronous" clipboard? is the question.
Assignee | ||
Comment 11•8 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #10)
> Why does Gtk not support "synchronous" clipboard intentionally? And why
> shouldn't it be changed to support "synchronous" clipboard? is the question.
Gtk does not support true synchronous clipboard because such model has some disadvantages. Application can freeze when waiting for clipboard data which are provided by another application for instance.
Gtk provides semi-synchronous clipboard instead of it - Gtk waits until the clipboard content is available and runs glib loop while waiting. But it's unusable in Firefox because such waiting can fire timeouts, rendering events and so on.
One can say Firefox should also use asyc clipboard - and that may be implemented some day - but I bet we would need Wayland specific code sooner or later as we also need some extra adjustments for X11.
Comment 12•8 years ago
|
||
So why would it be desirable for Firefox to use a synchronous clipboard api for wayland? And more specifically, why should that land in mozilla-central?
Component: Build Config → Widget: Gtk
Assignee | ||
Comment 13•8 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #12)
> So why would it be desirable for Firefox to use a synchronous clipboard api
> for wayland? And more specifically, why should that land in mozilla-central?
I don't say it's desirable. It would be great to have async clipboard...but Firefox recently supports only the synchronous one on all platforms (Linux, Windows, Mac) because it's easier to do so. So we need to implement it also on Wayland or rewrite complete Firefox API for clipboard for all platforms.
Updated•8 years ago
|
Priority: -- → P5
Whiteboard: tpi:+
Assignee | ||
Comment 14•8 years ago
|
||
GDK_WINDOWING_WAYLAND seems to be working as compile time option. If we manage to get symbols out of wayland library we don't need the configure option.
Assignee | ||
Comment 16•8 years ago
|
||
Mike, what would be the best approach to implement the dlopen mechanism you was talking about? Something like widget/gtk/mozgtk?
Flags: needinfo?(mh+mozilla)
Comment 17•8 years ago
|
||
(In reply to Martin Stránský from comment #16)
> Mike, what would be the best approach to implement the dlopen mechanism you
> was talking about? Something like widget/gtk/mozgtk?
I don't know what you're talking about.
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 18•8 years ago
|
||
I'm considering to implement Wayland dll wrapper similarly to the one we have for ffmpeg:
https://dxr.mozilla.org/mozilla-central/source/dom/media/platforms/ffmpeg/FFmpegLibWrapper.h
Mike, as you may review that code, do you agree with that approach?
Flags: needinfo?(mh+mozilla)
Assignee | ||
Updated•8 years ago
|
Summary: [Wayland] - enable build time option to enable Wayland → [Wayland] - implement libwayland-client wrapper
Assignee | ||
Updated•8 years ago
|
Attachment #8786286 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Assignee | ||
Comment 21•8 years ago
|
||
mozreview-review |
Comment on attachment 8839448 [details]
Bug 1299083 - Implement libwayland-client wrapper,
https://reviewboard.mozilla.org/r/114096/#review115580
::: widget/gtk/WaylandLibWrapper.h:46
(Diff revision 1)
> + PRLibrary* mWaylandLib;
> +};
> +
> +extern struct WaylandLibWrapper MozWaylandWrapper;
> +
> +// Redefine our wrapped code
Redefine core wayland functions.
We need to replace the core function calls inside wayland headers, because lots of wayland functions are just wrappers like:
wl_shm_create_pool(...)
{
struct wl_proxy *id;
id = wl_proxy_marshal_constructor((struct wl_proxy *) wl_shm,
WL_SHM_CREATE_POOL, &wl_shm_pool_interface, NULL, fd, size);
return (struct wl_shm_pool *) id;
}
where wl_proxy_marshal() does the real work and need to be wrapped.
Comment 22•8 years ago
|
||
mozreview-review |
Comment on attachment 8839448 [details]
Bug 1299083 - Implement libwayland-client wrapper,
https://reviewboard.mozilla.org/r/114096/#review117636
::: widget/gtk/WaylandLibWrapper.cpp:11
(Diff revision 1)
> +#include "WaylandLibWrapper.h"
> +#include "mozilla/PodOperations.h"
> +#include "mozilla/Types.h"
> +#include "prlink.h"
> +
> +struct WaylandLibWrapper MozWaylandWrapper;
Using a global variable makes this initialized even if wayland is not used. Plus, there is no graceful fallback if somehow the initialization fails. Using wayland functions then would likely crash on a null deref, which is not exactly nice.
Attachment #8839448 -
Flags: review?(mh+mozilla)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 24•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8839448 [details]
Bug 1299083 - Implement libwayland-client wrapper,
https://reviewboard.mozilla.org/r/114096/#review117636
> Using a global variable makes this initialized even if wayland is not used. Plus, there is no graceful fallback if somehow the initialization fails. Using wayland functions then would likely crash on a null deref, which is not exactly nice.
Thanks for looking at it.
Some kind of global object has to be used as the wayland lib is shared acros various modules (clipboard, mozcontainer, wayland renderer). I choosed the implicit initialization of global object because we don't know where the init point should be yet.
All wayland code should be braced by GDK_WINDOWING_WAYLAND define to make sure the code builds on old Gtk3 without wayland support - so I added fallback code for this case and also when wayland is not present on the actual running system.
Assignee | ||
Comment 25•8 years ago
|
||
Mike, ping? This bug is blocking all other Wayland bugs. Thanks.
Flags: needinfo?(mh+mozilla)
Assignee | ||
Comment 26•8 years ago
|
||
Actually we can't replace the calls in wayland headers as it defines and also uses the wayland functions. We may need to use some kind of linker lazy binding or so.
Flags: needinfo?(mh+mozilla)
Assignee | ||
Updated•8 years ago
|
Attachment #8839448 -
Attachment is obsolete: true
Attachment #8839448 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 27•8 years ago
|
||
Mike, we will need (as a first step) to check if Wayland is even available (by pkg-config --exists gtk+-wayland-3.0) and set proper build variable, for instance MOZ_WAYLAND or so. We need that to disable build of wayland only sources like clipboard/rendering modules.
Since the old-configure.in is deprecated what's the best way how to set this build flag?
Flags: needinfo?(mh+mozilla)
Comment 28•8 years ago
|
||
(In reply to Martin Stránský from comment #27)
> Since the old-configure.in is deprecated what's the best way how to set this
> build flag?
Everything gtk is still in old-configure.in, so it's fine to put it there.
Flags: needinfo?(mh+mozilla)
Assignee | ||
Updated•8 years ago
|
Summary: [Wayland] - implement libwayland-client wrapper → [Wayland] - build config
Comment hidden (mozreview-request) |
Assignee | ||
Comment 30•8 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #28)
> (In reply to Martin Stránský from comment #27)
> > Since the old-configure.in is deprecated what's the best way how to set this
> > build flag?
>
> Everything gtk is still in old-configure.in, so it's fine to put it there.
Thanks, I managed to put it to new configure and I hope that's fine. We actually need to control compile time only and we don't need link time dependency as wayland client library is pulled by Gtk itself.
Comment 31•8 years ago
|
||
mozreview-review |
Comment on attachment 8854392 [details]
Bug 1299083 - Add cairo-gtk3-wayland toolkit target to enable Gtk/Wayland build,
https://reviewboard.mozilla.org/r/126342/#review129252
I think I'd prefer to add a --enable-default-toolkit=cairo-gtk3-wayland instead.
::: toolkit/moz.configure:213
(Diff revision 1)
> +def wayland(value):
> + if value:
> + return True
> +
> +# Check for Wayland headers
> +wayland_headers = pkg_check_modules('MOZ_WAYLAND', 'gtk+-wayland-3.0 >= 3.20',
why version 3.20?
::: toolkit/moz.configure:214
(Diff revision 1)
> + if value:
> + return True
> +
> +# Check for Wayland headers
> +wayland_headers = pkg_check_modules('MOZ_WAYLAND', 'gtk+-wayland-3.0 >= 3.20',
> + when=wayland)
you could inline the wayland function like:
when=depends_if('--enable-wayland')(lambda _: True))
::: toolkit/moz.configure:216
(Diff revision 1)
> +
> +# Check for Wayland headers
> +wayland_headers = pkg_check_modules('MOZ_WAYLAND', 'gtk+-wayland-3.0 >= 3.20',
> + when=wayland)
> +
> +set_config('MOZ_WAYLAND', wayland)
you should depend on the result of wayland_headers here. Something like
set_config('MOZ_WAYLAND', depends_if(wayland_headers)(lambda _: True))
::: toolkit/moz.configure:217
(Diff revision 1)
> +# Check for Wayland headers
> +wayland_headers = pkg_check_modules('MOZ_WAYLAND', 'gtk+-wayland-3.0 >= 3.20',
> + when=wayland)
> +
> +set_config('MOZ_WAYLAND', wayland)
> +set_define('MOZ_WAYLAND', wayland)
ditto
::: toolkit/moz.configure:218
(Diff revision 1)
> +wayland_headers = pkg_check_modules('MOZ_WAYLAND', 'gtk+-wayland-3.0 >= 3.20',
> + when=wayland)
> +
> +set_config('MOZ_WAYLAND', wayland)
> +set_define('MOZ_WAYLAND', wayland)
> +imply_option('--enable-wayland', wayland)
You don't need this. I wish it threw an error instead of silently going through.
Attachment #8854392 -
Flags: review?(mh+mozilla)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 33•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8854392 [details]
Bug 1299083 - Add cairo-gtk3-wayland toolkit target to enable Gtk/Wayland build,
https://reviewboard.mozilla.org/r/126342/#review129252
Okay
> why version 3.20?
We should target 3.22 actualy which is the stable Gtk3 version. Older Gtk3 releases have various Gtk/Wayland bugs like mising popups and so.
> you could inline the wayland function like:
> when=depends_if('--enable-wayland')(lambda _: True))
moved to cairo-gtk3-wayland so I used a wayland() function.
Comment 34•8 years ago
|
||
mozreview-review |
Comment on attachment 8854392 [details]
Bug 1299083 - Add cairo-gtk3-wayland toolkit target to enable Gtk/Wayland build,
https://reviewboard.mozilla.org/r/126342/#review129684
::: commit-message-b5d8b:1
(Diff revision 2)
> +Bug 1299083 - Add --enable-wayland to control Wayland support build, r?glandium
Please change the commit message to match the patch.
Please also mention why you chose version 3.22 in the commit message.
::: toolkit/moz.configure:143
(Diff revision 2)
> # b2g/moz.configure).
> platform_choices = tuple(value)
> else:
> platform_choices = ('cairo-android',)
> else:
> - platform_choices = ('cairo-gtk3',)
> + platform_choices = ('cairo-gtk3', 'cairo-gtk3-wayland',)
No need for the final comma here.
Attachment #8854392 -
Flags: review?(mh+mozilla) → review+
Comment hidden (mozreview-request) |
Updated•8 years ago
|
Assignee: nobody → stransky
Comment 37•8 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a0d700792ad3
Add cairo-gtk3-wayland toolkit target to enable Gtk/Wayland build, r=glandium
Keywords: checkin-needed
Comment 38•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•