Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(155)

Issue 6732013: Refactor monitor_reconfigure to stop using system("xrandr"). (Closed)

Created:
7 years, 11 months ago by marcheu
Modified:
7 years, 9 months ago
Reviewers:
Daniel Erat
CC:
chromium-os-reviews_chromium.org, sosa
Visibility:
Public.

Description

Refactor monitor_reconfigure to stop using system("xrandr"). This removes the blinking on monitor plug/unplug. This also avoids crashes we get on intel because of vsync bugs. Change-Id: I04ad539a66ba7db1786a4c5d0d4eb7638e99cdd1 BUG=None. TEST=Plug in a second screen, check that things display on the external screen. Unplug the screen, check that things come back on the internal lcd. Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=88f70bf

Patch Set 1 #

Patch Set 2 : Rework monitor_reconfigure to remove system("xrandr"); #

Patch Set 3 : Fix comments. #

Total comments: 15

Patch Set 4 : Add missing dot. #

Patch Set 5 : Update the patch: address comments, make tests pass, free X structures. #

Total comments: 14

Patch Set 6 : Address comments #

Total comments: 1

Patch Set 7 : Fix comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+219 lines, -147 lines) Patch
M monitor_reconfigure_main.h View 1 2 3 4 5 6 1 chunk +27 lines, -14 lines 0 comments Download
M monitor_reconfigure_main.cc View 1 2 3 4 5 3 chunks +99 lines, -54 lines 0 comments Download
M resolution_selector.h View 1 2 3 4 5 4 chunks +19 lines, -9 lines 0 comments Download
M resolution_selector.cc View 5 chunks +13 lines, -13 lines 0 comments Download
M resolution_selector_test.cc View 1 2 3 4 5 3 chunks +61 lines, -57 lines 0 comments Download

Messages

Total messages: 8 (0 generated)
marcheu
7 years, 11 months ago (2011-03-24 22:51:48 UTC) #1
Daniel Erat
http://codereview.chromium.org/6732013/diff/10/monitor_reconfigure_main.cc File monitor_reconfigure_main.cc (right): http://codereview.chromium.org/6732013/diff/10/monitor_reconfigure_main.cc#newcode54 monitor_reconfigure_main.cc:54: external_resolution, make each of these be declared in a ...
7 years, 11 months ago (2011-03-24 23:02:11 UTC) #2
marcheu
Some remarks/questions. I'll fix the rest. http://codereview.chromium.org/6732013/diff/10/monitor_reconfigure_main.cc File monitor_reconfigure_main.cc (right): http://codereview.chromium.org/6732013/diff/10/monitor_reconfigure_main.cc#newcode164 monitor_reconfigure_main.cc:164: RROutput o = ...
7 years, 11 months ago (2011-03-24 23:17:53 UTC) #3
Daniel Erat
http://codereview.chromium.org/6732013/diff/10/monitor_reconfigure_main.cc File monitor_reconfigure_main.cc (right): http://codereview.chromium.org/6732013/diff/10/monitor_reconfigure_main.cc#newcode164 monitor_reconfigure_main.cc:164: RROutput o = output; On 2011/03/24 23:17:53, marcheu wrote: ...
7 years, 11 months ago (2011-03-24 23:39:46 UTC) #4
marcheu
On 2011/03/24 23:39:46, Daniel Erat wrote: > http://codereview.chromium.org/6732013/diff/10/monitor_reconfigure_main.cc > File monitor_reconfigure_main.cc (right): > > http://codereview.chromium.org/6732013/diff/10/monitor_reconfigure_main.cc#newcode164 ...
7 years, 11 months ago (2011-03-25 02:02:10 UTC) #5
Daniel Erat
Thanks, this is looking pretty good now. http://codereview.chromium.org/6732013/diff/7001/monitor_reconfigure_main.cc File monitor_reconfigure_main.cc (right): http://codereview.chromium.org/6732013/diff/7001/monitor_reconfigure_main.cc#newcode55 monitor_reconfigure_main.cc:55: ResolutionSelector::Mode lcd_resolution(0, ...
7 years, 11 months ago (2011-03-25 03:39:02 UTC) #6
marcheu
http://codereview.chromium.org/6732013/diff/7001/monitor_reconfigure_main.cc File monitor_reconfigure_main.cc (right): http://codereview.chromium.org/6732013/diff/7001/monitor_reconfigure_main.cc#newcode55 monitor_reconfigure_main.cc:55: ResolutionSelector::Mode lcd_resolution(0, 0, "", 0); On 2011/03/25 03:39:02, Daniel ...
7 years, 10 months ago (2011-03-25 17:39:42 UTC) #7
Daniel Erat
7 years, 10 months ago (2011-03-25 18:55:20 UTC) #8
LGTM!

http://codereview.chromium.org/6732013/diff/8001/monitor_reconfigure_main.h
File monitor_reconfigure_main.h (right):

http://codereview.chromium.org/6732013/diff/8001/monitor_reconfigure_main.h#n...
monitor_reconfigure_main.h:24: // We need to keep the Window to set the screen
size later
sorry, i meant some hint to callers about what 'window' actually is, like
"|window| is an arbitrary window on the screen to be configured." (assuming that
that's correct)

Powered by Google App Engine
This is Rietveld 408576698