Closed Bug 1299083 Opened 8 years ago Closed 7 years ago

[Wayland] - build config

Categories

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

Product:
Core
Component:
Widget: Gtk
Platform:
All
Linux
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox51 --- affected
firefox55 --- fixed

People

(Reporter: stransky, Assigned: stransky)

References

(Blocks 1 open bug)

Details

(Whiteboard: tpi:+)

Attachments

(1 file, 2 obsolete files)

Bug 1299083 - Add cairo-gtk3-wayland toolkit target to enable Gtk/Wayland build,
59 bytes, text/x-review-board-request
glandium
: review+
Details 
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.
Attached patch patch (obsolete) — — Splinter Review 
Mike, I believe you're the right person here. Thanks.
Attachment #8786286 - Flags: review?(mh+mozilla)
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)
(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.
> 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?
(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.
(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.
(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?
(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.
(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.
(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.
(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.
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
(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.
Priority: -- → P5
Whiteboard: tpi:+
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.
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)
(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)
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)
Summary: [Wayland] - enable build time option to enable Wayland → [Wayland] - implement libwayland-client wrapper
Attachment #8786286 - Attachment is obsolete: true
Go ahead.
Flags: needinfo?(mh+mozilla)
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 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 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.
Mike, ping? This bug is blocking all other Wayland bugs. Thanks.
Flags: needinfo?(mh+mozilla)
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)
Attachment #8839448 - Attachment is obsolete: true
Attachment #8839448 - Flags: review?(mh+mozilla)
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)
(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)
Summary: [Wayland] - implement libwayland-client wrapper → [Wayland] - build config
(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 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 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 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+
Fixed, Thanks!
Keywords: checkin-needed
Assignee: nobody → stransky
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
https://hg.mozilla.org/mozilla-central/rev/a0d700792ad3
Status: NEW → RESOLVED
Closed: 7 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.

Attachment

General

Creator:
Created:
Updated:
Size: