Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1980163003:
Xorg+dummy: set resolution to first size in list (Closed)
DescriptionXorg+dummy: set resolution to first size in list
Change the display resolution to the first size in the list before
starting the session.
BUG=596125
Committed: https://crrev.com/f3e8936bc82eb317ec9c247bb3bc397048bb6c8c
Cr-Commit-Position: refs/heads/master@{#394578}
Patch Set 1 #
Total comments: 6
Patch Set 2 : More flags, less duplication #
Total comments: 7
Patch Set 3 : Fix up comments #Messages
Total messages: 17 (5 generated)
rkjnsn@chromium.org changed reviewers: + jamiewalch@chromium.org, lambroslambrou@chromium.org
https://codereview.chromium.org/1980163003/diff/1/remoting/host/linux/linux_m... File remoting/host/linux/linux_me2me_host.py (right): https://codereview.chromium.org/1980163003/diff/1/remoting/host/linux/linux_m... remoting/host/linux/linux_me2me_host.py:586: env=self.child_env) Should redirect stdout/stderr (as in the code below). https://codereview.chromium.org/1980163003/diff/1/remoting/host/linux/linux_m... remoting/host/linux/linux_me2me_host.py:588: if not self.server_supports_exact_resize: You currently set this to False for Xorg+dummy, which means this will early-return. But the code you added above is the same as line 608 below. Maybe refactor this a little? 'server_supports_exact_resize' feels like the wrong thing to test. We should be testing the level of XRANDR support. For example, xrandr -s will work just fine with Xorg+dummy, but the --newmode/--addmode commands might not work properly?
https://codereview.chromium.org/1980163003/diff/1/remoting/host/linux/linux_m... File remoting/host/linux/linux_me2me_host.py (right): https://codereview.chromium.org/1980163003/diff/1/remoting/host/linux/linux_m... remoting/host/linux/linux_me2me_host.py:586: env=self.child_env) On 2016/05/17 00:08:14, Lambros wrote: > Should redirect stdout/stderr (as in the code below). Is there a reason we wouldn't want an error message here to show up in the log file? https://codereview.chromium.org/1980163003/diff/1/remoting/host/linux/linux_m... remoting/host/linux/linux_me2me_host.py:588: if not self.server_supports_exact_resize: On 2016/05/17 00:08:14, Lambros wrote: > You currently set this to False for Xorg+dummy, which means this will > early-return. But the code you added above is the same as line 608 below. > > Maybe refactor this a little? 'server_supports_exact_resize' feels like the > wrong thing to test. We should be testing the level of XRANDR support. For > example, xrandr -s will work just fine with Xorg+dummy, but the > --newmode/--addmode commands might not work properly? I agree that ‘server_supports_exact_resize’ probably isn't the right test in the long run, but testing the level of RandR would definitely be wrong. Even once we have a version of dummy that supports exact resize, we will still be providing the initial resolutions through the config file, and won't want to do the --newmode/--addmode commands. Maybe we should have a "self.server_supports_randr" flag that controls setting the initial size, setting DPI, and spawning the watch-resolution logic, and separate logic that only runs in the Xvfb-randr case to handle adding resolutions.
https://codereview.chromium.org/1980163003/diff/1/remoting/host/linux/linux_m... File remoting/host/linux/linux_me2me_host.py (right): https://codereview.chromium.org/1980163003/diff/1/remoting/host/linux/linux_m... remoting/host/linux/linux_me2me_host.py:586: env=self.child_env) On 2016/05/17 21:58:45, rkjnsn wrote: > On 2016/05/17 00:08:14, Lambros wrote: > > Should redirect stdout/stderr (as in the code below). > > Is there a reason we wouldn't want an error message here to show up in the log > file? I suppressed the errors originally because Xvfb-randr always whines about some "CRT" problem, which is a false alarm. You could show the errors, I suppose. There is already lots of noise in the session logs anyway. But be consistent with all the xrandr commands (feel free to punt to a separate CL). https://codereview.chromium.org/1980163003/diff/1/remoting/host/linux/linux_m... remoting/host/linux/linux_me2me_host.py:588: if not self.server_supports_exact_resize: On 2016/05/17 21:58:45, rkjnsn wrote: > On 2016/05/17 00:08:14, Lambros wrote: > > You currently set this to False for Xorg+dummy, which means this will > > early-return. But the code you added above is the same as line 608 below. > > > > Maybe refactor this a little? 'server_supports_exact_resize' feels like the > > wrong thing to test. We should be testing the level of XRANDR support. For > > example, xrandr -s will work just fine with Xorg+dummy, but the > > --newmode/--addmode commands might not work properly? > > I agree that ‘server_supports_exact_resize’ probably isn't the right test in the > long run, but testing the level of RandR would definitely be wrong. Even once we > have a version of dummy that supports exact resize, we will still be providing > the initial resolutions through the config file, and won't want to do the > --newmode/--addmode commands. Maybe we should have a > "self.server_supports_randr" flag that controls setting the initial size, > setting DPI, and spawning the watch-resolution logic, and separate logic that > only runs in the Xvfb-randr case to handle adding resolutions. Sounds good.
On 2016/05/18 00:13:49, Lambros wrote: > https://codereview.chromium.org/1980163003/diff/1/remoting/host/linux/linux_m... > File remoting/host/linux/linux_me2me_host.py (right): > > https://codereview.chromium.org/1980163003/diff/1/remoting/host/linux/linux_m... > remoting/host/linux/linux_me2me_host.py:586: env=self.child_env) > On 2016/05/17 21:58:45, rkjnsn wrote: > > On 2016/05/17 00:08:14, Lambros wrote: > > > Should redirect stdout/stderr (as in the code below). > > > > Is there a reason we wouldn't want an error message here to show up in the log > > file? > > I suppressed the errors originally because Xvfb-randr always whines about some > "CRT" problem, which is a false alarm. > > You could show the errors, I suppose. There is already lots of noise in the > session logs anyway. But be consistent with all the xrandr commands (feel free > to punt to a separate CL). > > https://codereview.chromium.org/1980163003/diff/1/remoting/host/linux/linux_m... > remoting/host/linux/linux_me2me_host.py:588: if not > self.server_supports_exact_resize: > On 2016/05/17 21:58:45, rkjnsn wrote: > > On 2016/05/17 00:08:14, Lambros wrote: > > > You currently set this to False for Xorg+dummy, which means this will > > > early-return. But the code you added above is the same as line 608 below. > > > > > > Maybe refactor this a little? 'server_supports_exact_resize' feels like the > > > wrong thing to test. We should be testing the level of XRANDR support. For > > > example, xrandr -s will work just fine with Xorg+dummy, but the > > > --newmode/--addmode commands might not work properly? > > > > I agree that ‘server_supports_exact_resize’ probably isn't the right test in > the > > long run, but testing the level of RandR would definitely be wrong. Even once > we > > have a version of dummy that supports exact resize, we will still be providing > > the initial resolutions through the config file, and won't want to do the > > --newmode/--addmode commands. Maybe we should have a > > "self.server_supports_randr" flag that controls setting the initial size, > > setting DPI, and spawning the watch-resolution logic, and separate logic that > > only runs in the Xvfb-randr case to handle adding resolutions. > > Sounds good. Okay, I uploaded a new patch with more flags and less code duplication.
sergeyu@chromium.org changed reviewers: + sergeyu@chromium.org
couple drive-by nits https://codereview.chromium.org/1980163003/diff/20001/remoting/host/linux/lin... File remoting/host/linux/linux_me2me_host.py (right): https://codereview.chromium.org/1980163003/diff/20001/remoting/host/linux/lin... remoting/host/linux/linux_me2me_host.py:346: self.server_supports_exact_resize = False do I understand it correctly that the X server does support resize with dummy and it just need to be integrated with our host? If so, maybe rename this to exact_resize_supported? https://codereview.chromium.org/1980163003/diff/20001/remoting/host/linux/lin... remoting/host/linux/linux_me2me_host.py:497: # For now, we don't support exact resize with Xorg+dummy add . at the end of a comment please. https://codereview.chromium.org/1980163003/diff/20001/remoting/host/linux/lin... remoting/host/linux/linux_me2me_host.py:499: # But dummy does support RandR 1.0 same here
lgtm https://codereview.chromium.org/1980163003/diff/20001/remoting/host/linux/lin... File remoting/host/linux/linux_me2me_host.py (right): https://codereview.chromium.org/1980163003/diff/20001/remoting/host/linux/lin... remoting/host/linux/linux_me2me_host.py:346: self.server_supports_exact_resize = False On 2016/05/18 20:38:17, Sergey Ulanov wrote: > do I understand it correctly that the X server does support resize with dummy > and it just need to be integrated with our host? > If so, maybe rename this to exact_resize_supported? I think (correct me if I'm wrong), Xorg+dummy supports exact-resize only if we compile our own patched dummy driver?
https://codereview.chromium.org/1980163003/diff/20001/remoting/host/linux/lin... File remoting/host/linux/linux_me2me_host.py (right): https://codereview.chromium.org/1980163003/diff/20001/remoting/host/linux/lin... remoting/host/linux/linux_me2me_host.py:346: self.server_supports_exact_resize = False On 2016/05/18 20:56:28, Lambros wrote: > On 2016/05/18 20:38:17, Sergey Ulanov wrote: > > do I understand it correctly that the X server does support resize with dummy > > and it just need to be integrated with our host? > > If so, maybe rename this to exact_resize_supported? > > I think (correct me if I'm wrong), Xorg+dummy supports exact-resize only if we > compile our own patched dummy driver? Right. The current dummy driver does support resize between pre-specified resolutions, but currently provides no way to do change to a size that was not specified when the server was started. So it supports resize, but not exact resize. I'll be adding code to the host so we can do nearest match with it like we do on Windows and OS X, and then look at making a patched version that supports exact resize. https://codereview.chromium.org/1980163003/diff/20001/remoting/host/linux/lin... remoting/host/linux/linux_me2me_host.py:497: # For now, we don't support exact resize with Xorg+dummy On 2016/05/18 20:38:17, Sergey Ulanov wrote: > add . at the end of a comment please. Acknowledged. https://codereview.chromium.org/1980163003/diff/20001/remoting/host/linux/lin... remoting/host/linux/linux_me2me_host.py:499: # But dummy does support RandR 1.0 On 2016/05/18 20:38:17, Sergey Ulanov wrote: > same here Acknowledged.
On 2016/05/18 21:02:17, rkjnsn wrote: > https://codereview.chromium.org/1980163003/diff/20001/remoting/host/linux/lin... > File remoting/host/linux/linux_me2me_host.py (right): > > https://codereview.chromium.org/1980163003/diff/20001/remoting/host/linux/lin... > remoting/host/linux/linux_me2me_host.py:346: self.server_supports_exact_resize = > False > On 2016/05/18 20:56:28, Lambros wrote: > > On 2016/05/18 20:38:17, Sergey Ulanov wrote: > > > do I understand it correctly that the X server does support resize with > dummy > > > and it just need to be integrated with our host? > > > If so, maybe rename this to exact_resize_supported? > > > > I think (correct me if I'm wrong), Xorg+dummy supports exact-resize only if we > > compile our own patched dummy driver? > > Right. The current dummy driver does support resize between pre-specified > resolutions, but currently provides no way to do change to a size that was not > specified when the server was started. So it supports resize, but not exact > resize. I'll be adding code to the host so we can do nearest match with it like > we do on Windows and OS X, and then look at making a patched version that > supports exact resize. > > https://codereview.chromium.org/1980163003/diff/20001/remoting/host/linux/lin... > remoting/host/linux/linux_me2me_host.py:497: # For now, we don't support exact > resize with Xorg+dummy > On 2016/05/18 20:38:17, Sergey Ulanov wrote: > > add . at the end of a comment please. > > Acknowledged. > > https://codereview.chromium.org/1980163003/diff/20001/remoting/host/linux/lin... > remoting/host/linux/linux_me2me_host.py:499: # But dummy does support RandR 1.0 > On 2016/05/18 20:38:17, Sergey Ulanov wrote: > > same here > > Acknowledged. I reworded the comment on 497 to "We can't support exact resize with the current Xorg dummy driver.", which is hopefully more clear.
The CQ bit was checked by rkjnsn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lambroslambrou@chromium.org Link to the patchset: https://codereview.chromium.org/1980163003/#ps40001 (title: "Fix up comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1980163003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1980163003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Xorg+dummy: set resolution to first size in list Change the display resolution to the first size in the list before starting the session. BUG=596125 ========== to ========== Xorg+dummy: set resolution to first size in list Change the display resolution to the first size in the list before starting the session. BUG=596125 Committed: https://crrev.com/f3e8936bc82eb317ec9c247bb3bc397048bb6c8c Cr-Commit-Position: refs/heads/master@{#394578} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/f3e8936bc82eb317ec9c247bb3bc397048bb6c8c Cr-Commit-Position: refs/heads/master@{#394578} |
