Closed Bug 1409716 Opened 2 years ago Closed 2 years ago

[Wayland] - Update wayland display setup

Categories

(Core :: Graphics, enhancement, P3)

All
Linux
enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: stransky, Assigned: stransky)

References

(Blocks 1 open bug)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files)

- remove support for broadway backend
- set up WAYLAND_DISPLAY variable
Priority: -- → P3
Whiteboard: [gfx-noted]
Comment on attachment 8919855 [details]
Bug 1409716 - Don't use DetectDisplay() for child process,

https://reviewboard.mozilla.org/r/190800/#review197516

::: commit-message-b09cf:1
(Diff revision 1)
> +Bug 1409716 - remove broadway backend, r?glandium

That is not all this patch does. So please either change the commit message to match what it does, or change the patch to match the commit message.
Attachment #8919855 - Flags: review?(mh+mozilla)
Comment on attachment 8919854 [details]
Bug 1409716 - Remove DetectDisplay() and use DISPLAY env variable or Gdk display manager to detect/open it,

https://reviewboard.mozilla.org/r/190798/#review197518

::: toolkit/xre/nsAppRunner.cpp:3973
(Diff revision 1)
>      if (saveDisplayArg) {
>        SaveWordToEnv("DISPLAY", nsDependentCString(display_name));
>      }
> -  } else {
> +  }
> +#ifdef MOZ_WAYLAND
> +  else if (GDK_IS_WAYLAND_DISPLAY(mGdkDisplay)) {

seems like there should be a !IsHeadless here too.
Attachment #8919854 - Flags: review?(mh+mozilla)
Comment on attachment 8919855 [details]
Bug 1409716 - Don't use DetectDisplay() for child process,

https://reviewboard.mozilla.org/r/190800/#review197516

> That is not all this patch does. So please either change the commit message to match what it does, or change the patch to match the commit message.

I decided to update the commit message :) Hope that's fine now.
Comment on attachment 8919854 [details]
Bug 1409716 - Remove DetectDisplay() and use DISPLAY env variable or Gdk display manager to detect/open it,

https://reviewboard.mozilla.org/r/190798/#review197932
Attachment #8919854 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8919855 [details]
Bug 1409716 - Don't use DetectDisplay() for child process,

https://reviewboard.mozilla.org/r/190800/#review197934

::: toolkit/xre/nsAppRunner.cpp:3030
(Diff revision 2)
> -      tryBroadway = true;
>    }
>  
> -  const char *display_name;
> -  if (tryX11 && (display_name = PR_GetEnv("DISPLAY"))) {
> -    return display_name;
> +  if (!display_name) {
> +    PR_fprintf(PR_STDERR,
> +               "Error: GDK_BACKEND does not match available displays\n");

The error message is misleading when you're not checking GDK_BACKEND.

Anyways, come to think of it, I'm not sure ignoring the value of GDK_BACKEND is the right thing to do. For example, one might try GDK_BACKEND=broadway and ... unexpectedly end up using X11.
Attachment #8919855 - Flags: review?(mh+mozilla)
Comment on attachment 8919855 [details]
Bug 1409716 - Don't use DetectDisplay() for child process,

https://reviewboard.mozilla.org/r/190800/#review197934

> The error message is misleading when you're not checking GDK_BACKEND.
> 
> Anyways, come to think of it, I'm not sure ignoring the value of GDK_BACKEND is the right thing to do. For example, one might try GDK_BACKEND=broadway and ... unexpectedly end up using X11.

We already ignore the GDK_BACKEND and that's reasong why this hack works:
https://dxr.mozilla.org/mozilla-central/rev/967c95cee709756596860ed2a3e6ac06ea3a053f/dom/ipc/ContentChild.cpp#607

OTOH I think we should handle GDK_BACKEND as Gdk does by simple "gdk_display_manager_open_display()" call when Wayland is enabled and use X11 exlusively otherwise.
Comment on attachment 8919855 [details]
Bug 1409716 - Don't use DetectDisplay() for child process,

https://reviewboard.mozilla.org/r/190800/#review197934

> We already ignore the GDK_BACKEND and that's reasong why this hack works:
> https://dxr.mozilla.org/mozilla-central/rev/967c95cee709756596860ed2a3e6ac06ea3a053f/dom/ipc/ContentChild.cpp#607
> 
> OTOH I think we should handle GDK_BACKEND as Gdk does by simple "gdk_display_manager_open_display()" call when Wayland is enabled and use X11 exlusively otherwise.

Hmm, that would cause problems when wayland-enabled build runs on X11 display.
(In reply to Mike Hommey [:glandium] from comment #9)
> Anyways, come to think of it, I'm not sure ignoring the value of GDK_BACKEND
> is the right thing to do. For example, one might try GDK_BACKEND=broadway
> and ... unexpectedly end up using X11.

After some consideration I think it's better to use Gdk to handle the GDK_BACKEND for X11/Wayland builds and for X11 only builds check the DISPLAY env. Also pass --display parameter to child process when was given on command line.
Attachment #8919854 - Flags: review+ → review?(mh+mozilla)
Comment on attachment 8919854 [details]
Bug 1409716 - Remove DetectDisplay() and use DISPLAY env variable or Gdk display manager to detect/open it,

https://reviewboard.mozilla.org/r/190798/#review199868
Attachment #8919854 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8919855 [details]
Bug 1409716 - Don't use DetectDisplay() for child process,

https://reviewboard.mozilla.org/r/190800/#review199870

::: dom/ipc/ContentChild.cpp:579
(Diff revision 3)
> -  // use the one from the environment on its own when deciding which backend
> -  // to use, and when starting under XWayland, it may choose to start with
> -  // the wayland backend instead of the x11 backend.
> +  // to gtk_init because it's not going to use the one from the environment
> +  // on its own when deciding which backend to use, and when starting under
> +  // XWayland, it may choose to start with the wayland backend
> +  // instead of the x11 backend.
>    // The DISPLAY environment variable is normally set by the parent process.
> +  // When --display command line argument is given load it from MOZ_GDK_DISPLAY.

Add a comment that MOZ_GDK_DISPLAY is set from nsAppRunner.cpp when --display is on the command line.
Attachment #8919855 - Flags: review?(mh+mozilla) → review+
Updated, Thanks!
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0266975a975e
Remove DetectDisplay() and use DISPLAY env variable or Gdk display manager to detect/open it, r=glandium
https://hg.mozilla.org/integration/autoland/rev/db01a5ff278c
Don't use DetectDisplay() for child process, r=glandium
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.