Closed
Bug 1215078
Opened 8 years ago
Closed 8 years ago
[Wayland] - display set up
Categories
(Core :: Graphics, enhancement)
Tracking
()
RESOLVED
FIXED
mozilla46
People
(Reporter: stransky, Assigned: stransky)
References
(Depends on 1 open bug)
Details
(Whiteboard: [gfx-noted])
Attachments
(1 file, 3 obsolete files)
3.87 KB,
patch
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #635134 +++ We need to open a proper display for Wayland session in nsAppRunner.cpp.
Assignee | ||
Comment 1•8 years ago
|
||
Display detection for Wayland session.
Attachment #8674183 -
Flags: review?(karlt)
Updated•8 years ago
|
Whiteboard: [gfx-noted]
Comment 2•8 years ago
|
||
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.
Attachment #8674183 -
Flags: review?(karlt)
Assignee | ||
Comment 3•8 years ago
|
||
(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.
Assignee | ||
Comment 4•8 years ago
|
||
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.
Assignee: nobody → stransky
Attachment #8674183 -
Attachment is obsolete: true
Attachment #8704780 -
Flags: review?(karlt)
Comment 5•8 years ago
|
||
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.
Attachment #8704780 -
Flags: review?(karlt) → review+
Comment 6•8 years ago
|
||
(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.
Assignee | ||
Comment 7•8 years ago
|
||
Thanks, there's the updated one. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=094658c1b9fd
Attachment #8704780 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8705110 -
Attachment description: v.3 patch for check-in → v.3 patch
Comment 8•8 years ago
|
||
Comment on attachment 8705110 [details] [diff] [review] v.3 patch Thanks. Can you restore the commit message and capitalize DetectDisplay() please?
Assignee | ||
Comment 9•8 years ago
|
||
There's the patch for check-in, thanks.
Attachment #8705110 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 10•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4214449db2db
Keywords: checkin-needed
Comment 11•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4214449db2db
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in
before you can comment on or make changes to this bug.
Description
•