Refactor monitor_reconfigure to stop using system("xrandr").

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

[Daniel Erat, 2011-03-24 23:02:11]

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#ne...
monitor_reconfigure_main.cc:54: external_resolution,
make each of these be declared in a separate statement

http://codereview.chromium.org/6732013/diff/10/monitor_reconfigure_main.cc#ne...
monitor_reconfigure_main.cc:64: XGrabServer (display_);
no spaces between function names and opening parentheses (ditto for the Xlib
calls later in this method)

http://codereview.chromium.org/6732013/diff/10/monitor_reconfigure_main.cc#ne...
monitor_reconfigure_main.cc:164: RROutput o = output;
why do you need to make a copy here?

http://codereview.chromium.org/6732013/diff/10/monitor_reconfigure_main.cc#ne...
monitor_reconfigure_main.cc:181: );
move this to the end of the previous line

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

http://codereview.chromium.org/6732013/diff/10/monitor_reconfigure_main.h#new...
monitor_reconfigure_main.h:24: MonitorReconfigureMain(Display* display, Window
window,
nit: one argument per line

http://codereview.chromium.org/6732013/diff/10/monitor_reconfigure_main.h#new...
monitor_reconfigure_main.h:40: void SortModesByResolution(const RROutput output,
make RROutput a const reference instead of passing-by-value

http://codereview.chromium.org/6732013/diff/10/monitor_reconfigure_main.h#new...
monitor_reconfigure_main.h:46: const ResolutionSelector::Mode);
make the first and last arguments be const references.  give the Mode argument a
name.

http://codereview.chromium.org/6732013/diff/10/monitor_reconfigure_main.h#new...
monitor_reconfigure_main.h:47: bool SetScreenResolution(const
ResolutionSelector::Mode resolution);
make this into a reference

http://codereview.chromium.org/6732013/diff/10/monitor_reconfigure_main.h#new...
monitor_reconfigure_main.h:51: const XRROutputInfo* output_info);
make these be references too (sorry for the repetition)

http://codereview.chromium.org/6732013/diff/10/resolution_selector.h
File resolution_selector.h (right):

http://codereview.chromium.org/6732013/diff/10/resolution_selector.h#newcode23
resolution_selector.h:23: Mode(int width = 0, int height = 0, std::string name =
"", int id = 0)
the style guide doesn't allow default arguments:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments

http://codereview.chromium.org/6732013/diff/10/resolution_selector.h#newcode62
resolution_selector.h:62: // The returned strings contain |name| values from the
passed-in modes.
update the bit about strings here; it's no longer accurate

[marcheu, 2011-03-24 23:17:53]

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#ne...
monitor_reconfigure_main.cc:164: RROutput o = output;
On 2011/03/24 23:02:11, Daniel Erat wrote:
> why do you need to make a copy here?

XRRSetCrtcConfig has a non-const RROutput* parameter. The other option is remove
const from the parameters.

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

http://codereview.chromium.org/6732013/diff/10/monitor_reconfigure_main.h#new...
monitor_reconfigure_main.h:40: void SortModesByResolution(const RROutput output,
On 2011/03/24 23:02:11, Daniel Erat wrote:
> make RROutput a const reference instead of passing-by-value

It's an int, do we care?

[Daniel Erat, 2011-03-24 23:39:46]

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#ne...
monitor_reconfigure_main.cc:164: RROutput o = output;
On 2011/03/24 23:17:53, marcheu wrote:
> On 2011/03/24 23:02:11, Daniel Erat wrote:
> > why do you need to make a copy here?
> 
> XRRSetCrtcConfig has a non-const RROutput* parameter. The other option is
remove
> const from the parameters.

How about const_cast?

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

http://codereview.chromium.org/6732013/diff/10/monitor_reconfigure_main.h#new...
monitor_reconfigure_main.h:40: void SortModesByResolution(const RROutput output,
On 2011/03/24 23:17:53, marcheu wrote:
> On 2011/03/24 23:02:11, Daniel Erat wrote:
> > make RROutput a const reference instead of passing-by-value
> 
> It's an int, do we care?

Okay, please remove the 'const' then.

[marcheu, 2011-03-25 02:02:10]

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#ne...
> monitor_reconfigure_main.cc:164: RROutput o = output;
> On 2011/03/24 23:17:53, marcheu wrote:
> > On 2011/03/24 23:02:11, Daniel Erat wrote:
> > > why do you need to make a copy here?
> > 
> > XRRSetCrtcConfig has a non-const RROutput* parameter. The other option is
> remove
> > const from the parameters.
> 
> How about const_cast?
> 
> http://codereview.chromium.org/6732013/diff/10/monitor_reconfigure_main.h
> File monitor_reconfigure_main.h (right):
> 
>
http://codereview.chromium.org/6732013/diff/10/monitor_reconfigure_main.h#new...
> monitor_reconfigure_main.h:40: void SortModesByResolution(const RROutput
output,
> On 2011/03/24 23:17:53, marcheu wrote:
> > On 2011/03/24 23:02:11, Daniel Erat wrote:
> > > make RROutput a const reference instead of passing-by-value
> > 
> > It's an int, do we care?
> 
> Okay, please remove the 'const' then.

Ok, I addressed all comments, also made tests pass and freed the X structures.
Let me know what you think!

[Daniel Erat, 2011-03-25 03:39:02]

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#...
monitor_reconfigure_main.cc:55: ResolutionSelector::Mode lcd_resolution(0, 0,
"", 0);
i think that it's okay to make an additional c'tor that takes no arguments and
initializes the object to these default values.

http://codereview.chromium.org/6732013/diff/7001/monitor_reconfigure_main.cc#...
monitor_reconfigure_main.cc:91: if (!lcd_resolution.name.empty())
i think that something went horribly wrong here and in the next if statement :-)

http://codereview.chromium.org/6732013/diff/7001/monitor_reconfigure_main.cc#...
monitor_reconfigure_main.cc:115: external_output_info_ = second_output_info;
can you add a check earlier in this method to make sure that it's only called
once (e.g. CHECK(!lcd_output_info_);)?  we'll leak memory otherwise if someone
screws up down the line and calls it twice.

http://codereview.chromium.org/6732013/diff/7001/monitor_reconfigure_main.cc#...
monitor_reconfigure_main.cc:177: int screen = DefaultScreen(display_);
this variable looks like it isn't being used

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

http://codereview.chromium.org/6732013/diff/7001/monitor_reconfigure_main.h#n...
monitor_reconfigure_main.h:25: Window window,
can you add a comment about what the 'window' parameter is for?  it's not
obvious to me from looking at the header.

http://codereview.chromium.org/6732013/diff/7001/monitor_reconfigure_main.h#n...
monitor_reconfigure_main.h:27: virtual ~MonitorReconfigureMain() {
not your fault, but mind removing the 'virtual' here?

http://codereview.chromium.org/6732013/diff/7001/monitor_reconfigure_main.h#n...
monitor_reconfigure_main.h:28: XRRFreeOutputInfo(lcd_output_info_);
do you need to check for NULL here first, or is XRRFreeOutputInfo() fine with
it?

[marcheu, 2011-03-25 17:39:42]

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#...
monitor_reconfigure_main.cc:55: ResolutionSelector::Mode lcd_resolution(0, 0,
"", 0);
On 2011/03/25 03:39:02, Daniel Erat wrote:
> i think that it's okay to make an additional c'tor that takes no arguments and
> initializes the object to these default values.

Done.

http://codereview.chromium.org/6732013/diff/7001/monitor_reconfigure_main.cc#...
monitor_reconfigure_main.cc:91: if (!lcd_resolution.name.empty())
On 2011/03/25 03:39:02, Daniel Erat wrote:
> i think that something went horribly wrong here and in the next if statement
:-)

Indeed. Oops.

http://codereview.chromium.org/6732013/diff/7001/monitor_reconfigure_main.cc#...
monitor_reconfigure_main.cc:115: external_output_info_ = second_output_info;
On 2011/03/25 03:39:02, Daniel Erat wrote:
> can you add a check earlier in this method to make sure that it's only called
> once (e.g. CHECK(!lcd_output_info_);)?  we'll leak memory otherwise if someone
> screws up down the line and calls it twice.

Done.

http://codereview.chromium.org/6732013/diff/7001/monitor_reconfigure_main.cc#...
monitor_reconfigure_main.cc:177: int screen = DefaultScreen(display_);
On 2011/03/25 03:39:02, Daniel Erat wrote:
> this variable looks like it isn't being used

Done.

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

http://codereview.chromium.org/6732013/diff/7001/monitor_reconfigure_main.h#n...
monitor_reconfigure_main.h:25: Window window,
On 2011/03/25 03:39:02, Daniel Erat wrote:
> can you add a comment about what the 'window' parameter is for?  it's not
> obvious to me from looking at the header.

Done.

http://codereview.chromium.org/6732013/diff/7001/monitor_reconfigure_main.h#n...
monitor_reconfigure_main.h:27: virtual ~MonitorReconfigureMain() {
On 2011/03/25 03:39:02, Daniel Erat wrote:
> not your fault, but mind removing the 'virtual' here?

Done.

http://codereview.chromium.org/6732013/diff/7001/monitor_reconfigure_main.h#n...
monitor_reconfigure_main.h:28: XRRFreeOutputInfo(lcd_output_info_);
On 2011/03/25 03:39:02, Daniel Erat wrote:
> do you need to check for NULL here first, or is XRRFreeOutputInfo() fine with
> it?

It's just a free() so NULL is a valid parameter.

[Daniel Erat, 2011-03-25 18:55:20]

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)