1215078 - [Wayland] - display set up

Status: RESOLVED FIXED

(Core :: Graphics, enhancement)

Description

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

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

We need to open a proper display for Wayland session in nsAppRunner.cpp.

Comment 1

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

Attachment: display-detect.patch

Display detection for Wayland session.

Comment 2

[Karl Tomlinson (:karlt)]

Comment on attachment 8674183 [details] [diff] [review]
display-detect.patch

In the current state, I assume the X11 port works better than the Wayland port
so we should be preferring an X11 display before over a Wayland display.  If
GDK_BACKEND indicates that the X11 backend should not be used but the Wayland
backend should, then the Wayland backend can be used.  Once Wayland support is
as good as X11 support then we can prefer Wayland for consistency with GDK.

>+static const char *detectDisplay(bool *aIsX11Display)

If GDK's logic were desirable, then it would have been nice to use similar logic in selecting the display.

  gdk_display_manager_open_display() tries each backend comma-separated in
  GDK_BACKEND, to see whether it is allowed and whether it can be opened using
  the display name if provided or the environment variable if not.

  If a backend in GDK_BACKEND is '*', then all backends are tried in the
  default order determined by GDK.

  If GDK_BACKEND is not set, then all allowed backends are tried in the order
  specified by the application.

However, I'm not fond of GDK's approach of trying to open each display type in
turn until one is opened successfully.  I prefer a clear mechanism for
deciding which display to attempt to open, as in the patch here, and reporting
an error if that fails.

Still I think we can support interpretation of GDK_BACKEND a bit more
consistently with what GDK does:

  1. Set |any| if GDK_BACKEND is not set or contains '*'.

  2. If DISPLAY is non-empty and |any| is set or GDK_BACKEND contains "x11",
     then return the value of DISPLAY.

  3. Similarly for wayland and broadway.

>+      return(PR_GetEnv("WAYLAND_DISPLAY"));

No parentheses required around the return value.

>   const char *display_name = gdk_get_display_arg_name();
>-  if (display_name) {
>-    SaveWordToEnv("DISPLAY", nsDependentCString(display_name));
>-  } else {

We need to keep this when the --display arg selects an X11 display so that
other server connections in this process and child processes use the same
display.

We can consider this option unsupported for other display types, for now at
least.

>-  XInitThreads();
>+  if (isX11Display) {
>+    XInitThreads();
>+  }

It's fine to call XInitThreads even if the display is not X11, which may
simplify things.

Comment 3

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

(In reply to Karl Tomlinson (ni?:karlt) from comment #2)
> >   const char *display_name = gdk_get_display_arg_name();
> >-  if (display_name) {
> >-    SaveWordToEnv("DISPLAY", nsDependentCString(display_name));
> >-  } else {
> 
> We need to keep this when the --display arg selects an X11 display so that
> other server connections in this process and child processes use the same
> display.

If XInitThreads can be called for non-X11 displays then --display arg can be used for all backends. The whole exercise here was to call XInitThreads() for X11 only.

Comment 4

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

Attachment: display v.2

Thanks, this one should fix that.  

Why do we need to set the "DISPLAY" value when display is set on command line by "--display"? For child processes? With this patch the DISPLAY is set for X11 only but --display is accepted regardless the display type.

Comment 5

[Karl Tomlinson (:karlt)]

Comment on attachment 8704780 [details] [diff] [review]
display v.2

>+static const char *detectDisplay(void)

 "static const char* DetectDisplay()"
to follow Gecko style please.

>+  if (!backend || (backend && strstr(backend, "*"))) {

No need to null check backend twice.  Just

  if (!backend || strstr(backend, "*")) {

>+  PR_fprintf(PR_STDERR, "Error: GDK_BACKEND does not match available displays\n");
>+  return NULL;

return nullptr;

>+    display_name = detectDisplay();
>     if (!display_name) {
>       PR_fprintf(PR_STDERR, "Error: no display specified\n");

With the message printed by DetectDisplay(), there is no longer any need for
this PR_fprintf, so please remove this one.

>   {
>     mGdkDisplay = gdk_display_open(display_name);
>     if (!mGdkDisplay) {
>       PR_fprintf(PR_STDERR, "Error: cannot open display: %s\n", display_name);
>       return 1;
>     }
>     gdk_display_manager_set_default_display (gdk_display_manager_get(),
>                                              mGdkDisplay);
>-    if (!GDK_IS_X11_DISPLAY(mGdkDisplay))
>+    if (GDK_IS_X11_DISPLAY(mGdkDisplay)) {
>+      if (saveDisplayArg)
>+        SaveWordToEnv("DISPLAY", nsDependentCString(display_name));
>+    } else {
>       mDisableRemote = true;
>+    }
>   }
> #endif

Please use {} to brace around the SaveWordToEnv line, which is now Gecko
style.  This is similar to what I meant in
https://bugzilla.mozilla.org/show_bug.cgi?id=635134#c20 so can you also remove
the outermost braces quoted here (added in response to that comment), please?
Preprocessor conditionals are not usually braced, but runtime conditionals are.

Comment 6

[Karl Tomlinson (:karlt)]

(In reply to Martin Stránský from comment #4)
> Why do we need to set the "DISPLAY" value when display is set on command
> line by "--display"? For child processes?

Yes, child processes, and also other X server connections in the same process,
as used in X11Error, XRemoteClient, and webrtc screen/window sharing.

Comment 7

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

Attachment: v.3 patch

Thanks, there's the updated one.

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

Comment 8

[Karl Tomlinson (:karlt)]

Comment on attachment 8705110 [details] [diff] [review]
v.3 patch

Thanks.  Can you restore the commit message and capitalize DetectDisplay() please?

Comment 9

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

Attachment: v.3 patch for check-in

There's the patch for check-in, thanks.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4214449db2db

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4214449db2db