1215104 - [Wayland] nsWindow update for non-X11 backend

Status: RESOLVED FIXED

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

Description

[Martin Stránský [:stransky] (ni? me)]

+++ This bug was initially created as a clone of Bug #635134 +++

We need to update nsWindow for non-X11 backends.

Comment 1

[Martin Stránský [:stransky] (ni? me)]

Attachment: initial patch

An initial patch for nsWindow. We'll also need to enable container window on Wayland when CSD changes land.

Comment 2

[Martin Stránský [:stransky] (ni? me)]

Attachment: nsWindow patch

There's an updated patch for nsWindow. It's pretty much all we need for Wayland support.

Comment 3

[Karl Tomlinson (:karlt)]

Comment on attachment 8763587 [details] [diff] [review]
nsWindow patch

Please include commit messages in patches submitted for review.

>+#include <sys/time.h>

What needs this?

>+    explicit CurrentX11TimeGetter(GdkWindow* aWindow, bool aIsX11Display)

The GdkWindow already knows the kind of display, so keep the
CurrentX11TimeGetter() interface simple and use GDK_IS_X11_WINDOW().

>+            return g_get_monotonic_time()/1000;

Can you point me at the information that means this is the correct reference
time, please?

What happens with remote connections?

>     // gdk_x11_display_get_user_time tracks button and key presses,
>     // DESKTOP_STARTUP_ID used to start the app, drop events from external
>     // drags, WM_DELETE_WINDOW delete events, but not usually mouse motion nor
>     // button and key releases.  Therefore use the most recent of
>     // gdk_x11_display_get_user_time and the last time that we have seen.
>+    GdkDisplay *display = gdk_display_get_default();
>+    if (!GDK_IS_X11_DISPLAY(display))
>+        return sLastUserInputTime;
>+

This code doesn't belong with this comment, so leave the comment with the
associated code.

But I guess sLastUserInputTime is just a placeholder for a Wayland
implementation, so please either explain in comment why a complete
implementation is not required for Wayland or file a follow-up bug
and reference it in a comment here.

>     guint32 timestamp =
>             gdk_x11_display_get_user_time(gdk_display_get_default());

Please use |display| instead of getting it again.

>-            gtk_widget_set_double_buffered(widgets[i], FALSE);
>+            if (mIsX11Display)
>+                gtk_widget_set_double_buffered(widgets[i], FALSE);

Why is it not necessary to disable double buffering with Wayland?
Please put this in a separate commit, with a code comment explaining why.

Comment 4

[Martin Stránský [:stransky] (ni? me)]

Attachment: X display patch

Thanks, there's a patch with basic X display patch which also disables SHM on nsWindow. I'll address your comments in another one.

I don't think we should call new nsShmImage() with invalid (null) mXDisplay, mXWindow, mXVisual, mXDepth variables.

Comment 6

[Martin Stránský [:stransky] (ni? me)]

Comment on attachment 8764160 [details] [diff] [review]
X display patch

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=12c4650dbb7a

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/628f6daa21f2
Update nsWindow for non-X11 backend. r=karlt

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/628f6daa21f2

Comment 9

[Martin Stránský [:stransky] (ni? me)]

Attachment: start-up notification patch

We need to disable start-up notification on Wayland. I know it can be disabled build-time but it may be useful to have one build setup for X11 and Wayland.

Comment 10

[Karl Tomlinson (:karlt)]

Comment on attachment 8766253 [details] [diff] [review]
start-up notification patch

>+    if (mIsX11Display) {

With all the pieces of code being disabled for Wayland, we need a way to track what remains to make the Wayland port feature comparable to the X11 port.
This will be necessary before enabling Wayland by default when available can be considered.

In this case, bug 726479 already exists.  I've marked that blocking bug 635134.

Please include a comment above this line to indicate that bug report for non-X11 ports.

>+        if (sn_launchee_context_get_id_has_timestamp(ctx)) {
>+            gdk_x11_window_set_user_time(gdkWindow, sn_launchee_context_get_timestamp(ctx));

Please wrap after ','.

Comment 11

[Martin Stránský [:stransky] (ni? me)]

Attachment: start-up notification patch for check-in

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1daada2033bf

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/248114b54a90
Disable start-up notifications for non-X11 backends, r=karlt

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/248114b54a90

Comment 14

[Jeremy Nix]

I'm getting the following error after this patch.

3:53.70 /opt/thunderbird_build/comm-central/mozilla/widget/gtk/nsWindow.cpp:1352:9: error: ‘mIsX11Display’ was not declared in this scope
3:53.70      if (mIsX11Display) {

It looks like the "mIsX11Display" variable is not static and therefore can't be used in a static method.

Comment 15

[Martin Stránský [:stransky] (ni? me)]

Attachment: static-display.patch

I had to be blind to check mIsX11Display attribute in non-member and static function. Sorry for that. Please use this patch, it replaces the wrong variable with default display check. Karl, can you please review this one? Thanks.

Comment 16

[Karl Tomlinson (:karlt)]

Comment on attachment 8776006 [details] [diff] [review]
static-display.patch

>Bug 1215104 - use gdk_display_get_default() instead of mIsX11Display attribute, r=?karlt

Thanks.  Please use 1286482 for the bug number, so we can track uplift to 50.

Comment 17

[u575260]

ftw mozilla 2017!
https://bugzilla.mozilla.org/show_bug.cgi?id=635134

Comment 18

[Martin Stránský [:stransky] (ni? me)]

Let's close this one and file new ones for separated issues.