Merge monitor_reconfigure into powerd.

Merge monitor_reconfigure into powerd.

BUG=chromium-os:10321, chromium-os:11596, chromium-os:13430, chromium-os-partner:2313
TEST=remove /etc/udev/rules.d/99-monitor-hotplug.rules before testing; plug/unplug monitors, check that things work ok including backlight.

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=98eacc9

[marcheu, 2011-04-13 22:59:26]

First cut at powerd + monitor_reconfigure merge.

Please take a look, I'll port the tests from monitor_reconfigure later.

[Daniel Erat, 2011-04-13 23:33:26]

http://codereview.chromium.org/6854002/diff/3001/SConstruct
File SConstruct (right):

http://codereview.chromium.org/6854002/diff/3001/SConstruct#newcode149
SConstruct:149: tests.append(powerman_unittest_env.Program('powerman_unittest',
deps))
you should add the resolution selector test here, and check that it gets built
and run when you run emerge with FEATURES=test

http://codereview.chromium.org/6854002/diff/3001/drm-tool.cc
File drm-tool.cc (right):

http://codereview.chromium.org/6854002/diff/3001/drm-tool.cc#newcode19
drm-tool.cc:19: class DrmCallBack : public power_manager::UdevCallBack {
nit: i think that i've typically seen this written as Callback (lowercase 'b')

http://codereview.chromium.org/6854002/diff/3001/monitor_reconfigure.h
File monitor_reconfigure.h (right):

http://codereview.chromium.org/6854002/diff/3001/monitor_reconfigure.h#newcode1
monitor_reconfigure.h:1: // Copyright (c) 2010 The Chromium OS Authors. All
rights reserved.
nit: 2011

http://codereview.chromium.org/6854002/diff/3001/powerd_main.cc
File powerd_main.cc (right):

http://codereview.chromium.org/6854002/diff/3001/powerd_main.cc#newcode1
powerd_main.cc:1: // Copyright (c) 2010 The Chromium OS Authors. All rights
reserved.
nit: 2011

http://codereview.chromium.org/6854002/diff/3001/resolution_selector.cc
File resolution_selector.cc (right):

http://codereview.chromium.org/6854002/diff/3001/resolution_selector.cc#newcode1
resolution_selector.cc:1: // Copyright (c) 2010 The Chromium OS Authors. All
rights reserved.
nit: 2011

http://codereview.chromium.org/6854002/diff/3001/resolution_selector.h
File resolution_selector.h (right):

http://codereview.chromium.org/6854002/diff/3001/resolution_selector.h#newcode1
resolution_selector.h:1: // Copyright (c) 2010 The Chromium OS Authors. All
rights reserved.
nit: 2011

http://codereview.chromium.org/6854002/diff/3001/resolution_selector_test.cc
File resolution_selector_test.cc (right):

http://codereview.chromium.org/6854002/diff/3001/resolution_selector_test.cc#...
resolution_selector_test.cc:1: // Copyright (c) 2010 The Chromium OS Authors.
All rights reserved.
nit: 2011

http://codereview.chromium.org/6854002/diff/3001/udev_listener.cc
File udev_listener.cc (right):

http://codereview.chromium.org/6854002/diff/3001/udev_listener.cc#newcode5
udev_listener.cc:5: #include <glib.h>
nit: add a blank line after this

http://codereview.chromium.org/6854002/diff/3001/udev_listener.cc#newcode15
udev_listener.cc:15: {}
move the opening brace to the end of the previous line

http://codereview.chromium.org/6854002/diff/3001/udev_listener.cc#newcode18
udev_listener.cc:18: {
move to previous line

http://codereview.chromium.org/6854002/diff/3001/udev_listener.cc#newcode22
udev_listener.cc:22: udev = udev_new();
move this onto the same line as the declaration:

struct udev* udev = udev_new();

http://codereview.chromium.org/6854002/diff/3001/udev_listener.cc#newcode31
udev_listener.cc:31: LOG(ERROR) << "Can't create udev monitor.";
leaking |udev| here; use udev_unref()

http://codereview.chromium.org/6854002/diff/3001/udev_listener.cc#newcode41
udev_listener.cc:41: GIOChannel * channel;
delete space before '*'

http://codereview.chromium.org/6854002/diff/3001/udev_listener.cc#newcode42
udev_listener.cc:42: channel = g_io_channel_unix_new(fd);
combine with declaration

http://codereview.chromium.org/6854002/diff/3001/udev_listener.cc#newcode45
udev_listener.cc:45: LOG(WARNING) << "Udev listener waiting for events on
subsystem "
i don't see why this is a warning.  use LOG(INFO)

http://codereview.chromium.org/6854002/diff/3001/udev_listener.cc#newcode47
udev_listener.cc:47: 
leaking |udev| here.  can you unref it now, or do you need to hang on to it in
the UdevListener?

http://codereview.chromium.org/6854002/diff/3001/udev_listener.cc#newcode52
udev_listener.cc:52: GIOCondition condition,
fix indenting

http://codereview.chromium.org/6854002/diff/3001/udev_listener.h
File udev_listener.h (right):

http://codereview.chromium.org/6854002/diff/3001/udev_listener.h#newcode10
udev_listener.h:10: #include <string>
nit: this should come first, followed by a blank line

http://codereview.chromium.org/6854002/diff/3001/udev_listener.h#newcode15
udev_listener.h:15: public:
classes with virtual methods should have virtual destructors

http://codereview.chromium.org/6854002/diff/3001/udev_listener.h#newcode16
udev_listener.h:16: virtual void CallBack(GIOChannel* source, GIOCondition
condition) = 0;
nit: rename to something like Run()

http://codereview.chromium.org/6854002/diff/3001/udev_listener.h#newcode28
udev_listener.h:28: gpointer data);
nit: add blank line after this

http://codereview.chromium.org/6854002/diff/3001/udev_listener.h#newcode31
udev_listener.h:31: 
nit: delete extra blank line

[marcheu, 2011-04-14 02:23:55]

http://codereview.chromium.org/6854002/diff/3001/drm-tool.cc
File drm-tool.cc (right):

http://codereview.chromium.org/6854002/diff/3001/drm-tool.cc#newcode19
drm-tool.cc:19: class DrmCallBack : public power_manager::UdevCallBack {
On 2011/04/13 23:33:26, Daniel Erat wrote:
> nit: i think that i've typically seen this written as Callback (lowercase 'b')

Done.

http://codereview.chromium.org/6854002/diff/3001/monitor_reconfigure.h
File monitor_reconfigure.h (right):

http://codereview.chromium.org/6854002/diff/3001/monitor_reconfigure.h#newcode1
monitor_reconfigure.h:1: // Copyright (c) 2010 The Chromium OS Authors. All
rights reserved.
On 2011/04/13 23:33:26, Daniel Erat wrote:
> nit: 2011

Done.

http://codereview.chromium.org/6854002/diff/3001/powerd_main.cc
File powerd_main.cc (right):

http://codereview.chromium.org/6854002/diff/3001/powerd_main.cc#newcode1
powerd_main.cc:1: // Copyright (c) 2010 The Chromium OS Authors. All rights
reserved.
On 2011/04/13 23:33:26, Daniel Erat wrote:
> nit: 2011

Done.

http://codereview.chromium.org/6854002/diff/3001/resolution_selector.cc
File resolution_selector.cc (right):

http://codereview.chromium.org/6854002/diff/3001/resolution_selector.cc#newcode1
resolution_selector.cc:1: // Copyright (c) 2010 The Chromium OS Authors. All
rights reserved.
On 2011/04/13 23:33:26, Daniel Erat wrote:
> nit: 2011

Done.

http://codereview.chromium.org/6854002/diff/3001/resolution_selector.h
File resolution_selector.h (right):

http://codereview.chromium.org/6854002/diff/3001/resolution_selector.h#newcode1
resolution_selector.h:1: // Copyright (c) 2010 The Chromium OS Authors. All
rights reserved.
On 2011/04/13 23:33:26, Daniel Erat wrote:
> nit: 2011

Done.

http://codereview.chromium.org/6854002/diff/3001/resolution_selector_test.cc
File resolution_selector_test.cc (right):

http://codereview.chromium.org/6854002/diff/3001/resolution_selector_test.cc#...
resolution_selector_test.cc:1: // Copyright (c) 2010 The Chromium OS Authors.
All rights reserved.
On 2011/04/13 23:33:26, Daniel Erat wrote:
> nit: 2011

Done.

http://codereview.chromium.org/6854002/diff/3001/udev_listener.cc
File udev_listener.cc (right):

http://codereview.chromium.org/6854002/diff/3001/udev_listener.cc#newcode5
udev_listener.cc:5: #include <glib.h>
On 2011/04/13 23:33:26, Daniel Erat wrote:
> nit: add a blank line after this

Done.

http://codereview.chromium.org/6854002/diff/3001/udev_listener.cc#newcode15
udev_listener.cc:15: {}
On 2011/04/13 23:33:26, Daniel Erat wrote:
> move the opening brace to the end of the previous line

Done.

http://codereview.chromium.org/6854002/diff/3001/udev_listener.cc#newcode18
udev_listener.cc:18: {
On 2011/04/13 23:33:26, Daniel Erat wrote:
> move to previous line

Done.

http://codereview.chromium.org/6854002/diff/3001/udev_listener.cc#newcode22
udev_listener.cc:22: udev = udev_new();
On 2011/04/13 23:33:26, Daniel Erat wrote:
> move this onto the same line as the declaration:
> 
> struct udev* udev = udev_new();

Done.

http://codereview.chromium.org/6854002/diff/3001/udev_listener.cc#newcode31
udev_listener.cc:31: LOG(ERROR) << "Can't create udev monitor.";
On 2011/04/13 23:33:26, Daniel Erat wrote:
> leaking |udev| here; use udev_unref()

Done.

http://codereview.chromium.org/6854002/diff/3001/udev_listener.cc#newcode41
udev_listener.cc:41: GIOChannel * channel;
On 2011/04/13 23:33:26, Daniel Erat wrote:
> delete space before '*'

Done.

http://codereview.chromium.org/6854002/diff/3001/udev_listener.cc#newcode42
udev_listener.cc:42: channel = g_io_channel_unix_new(fd);
On 2011/04/13 23:33:26, Daniel Erat wrote:
> combine with declaration

Done.

http://codereview.chromium.org/6854002/diff/3001/udev_listener.cc#newcode45
udev_listener.cc:45: LOG(WARNING) << "Udev listener waiting for events on
subsystem "
On 2011/04/13 23:33:26, Daniel Erat wrote:
> i don't see why this is a warning.  use LOG(INFO)

Done.

http://codereview.chromium.org/6854002/diff/3001/udev_listener.cc#newcode47
udev_listener.cc:47: 
On 2011/04/13 23:33:26, Daniel Erat wrote:
> leaking |udev| here.  can you unref it now, or do you need to hang on to it in
> the UdevListener?

I added it to UdevListener class and delete in the destructor.

http://codereview.chromium.org/6854002/diff/3001/udev_listener.cc#newcode52
udev_listener.cc:52: GIOCondition condition,
On 2011/04/13 23:33:26, Daniel Erat wrote:
> fix indenting

Done.

http://codereview.chromium.org/6854002/diff/3001/udev_listener.h
File udev_listener.h (right):

http://codereview.chromium.org/6854002/diff/3001/udev_listener.h#newcode10
udev_listener.h:10: #include <string>
On 2011/04/13 23:33:26, Daniel Erat wrote:
> nit: this should come first, followed by a blank line

Done.

http://codereview.chromium.org/6854002/diff/3001/udev_listener.h#newcode15
udev_listener.h:15: public:
On 2011/04/13 23:33:26, Daniel Erat wrote:
> classes with virtual methods should have virtual destructors

Done.

http://codereview.chromium.org/6854002/diff/3001/udev_listener.h#newcode16
udev_listener.h:16: virtual void CallBack(GIOChannel* source, GIOCondition
condition) = 0;
On 2011/04/13 23:33:26, Daniel Erat wrote:
> nit: rename to something like Run()

Done.

http://codereview.chromium.org/6854002/diff/3001/udev_listener.h#newcode28
udev_listener.h:28: gpointer data);
On 2011/04/13 23:33:26, Daniel Erat wrote:
> nit: add blank line after this

Done.

http://codereview.chromium.org/6854002/diff/3001/udev_listener.h#newcode31
udev_listener.h:31: 
On 2011/04/13 23:33:26, Daniel Erat wrote:
> nit: delete extra blank line

Done.

[tfarina, 2011-04-14 02:36:05]

Just did a quickly look at some files as I have to sleep now. Please some
comments below.

http://codereview.chromium.org/6854002/diff/16/drm-tool.cc
File drm-tool.cc (right):

http://codereview.chromium.org/6854002/diff/16/drm-tool.cc#newcode13
drm-tool.cc:13: #include "base/scoped_ptr.h"
I don't see any uses of scoped_ptr here.

http://codereview.chromium.org/6854002/diff/16/drm-tool.cc#newcode14
drm-tool.cc:14: #include "base/string_util.h"
Why do you need this include?

http://codereview.chromium.org/6854002/diff/16/drm-tool.cc#newcode22
drm-tool.cc:22: (void) source;
hum? If it's to please the compiler, maybe /* source */ and /* condition */ on
the parameters instead?

http://codereview.chromium.org/6854002/diff/16/drm-tool.cc#newcode24
drm-tool.cc:24: printf("Udev drm callback\n");
Could you use some other macro from logging.h instead?

http://codereview.chromium.org/6854002/diff/16/drm-tool.cc#newcode28
drm-tool.cc:28: int main(int argc, char* argv[]) {
I'd change this to char** argv (just personal preference).

http://codereview.chromium.org/6854002/diff/16/drm-tool.cc#newcode38
drm-tool.cc:38: 
remove this blank line.

http://codereview.chromium.org/6854002/diff/16/powerd.h
File powerd.h (right):

http://codereview.chromium.org/6854002/diff/16/powerd.h#newcode23
powerd.h:23: #include "power_manager/monitor_reconfigure.h"
please, could you forward declare this instead?

http://codereview.chromium.org/6854002/diff/16/udev_listener.cc
File udev_listener.cc (right):

http://codereview.chromium.org/6854002/diff/16/udev_listener.cc#newcode9
udev_listener.cc:9: #include "power_manager/udev_listener.h"
this should be included first. Please move it to line 5.

http://codereview.chromium.org/6854002/diff/16/udev_listener.cc#newcode80
udev_listener.cc:80: 
remove this blank line.

[marcheu, 2011-04-14 02:52:10]

http://codereview.chromium.org/6854002/diff/16/drm-tool.cc
File drm-tool.cc (right):

http://codereview.chromium.org/6854002/diff/16/drm-tool.cc#newcode13
drm-tool.cc:13: #include "base/scoped_ptr.h"
On 2011/04/14 02:36:05, tfarina wrote:
> I don't see any uses of scoped_ptr here.

Done.

http://codereview.chromium.org/6854002/diff/16/drm-tool.cc#newcode14
drm-tool.cc:14: #include "base/string_util.h"
On 2011/04/14 02:36:05, tfarina wrote:
> Why do you need this include?

Removed.

http://codereview.chromium.org/6854002/diff/16/drm-tool.cc#newcode22
drm-tool.cc:22: (void) source;
On 2011/04/14 02:36:05, tfarina wrote:
> hum? If it's to please the compiler, maybe /* source */ and /* condition */ on
> the parameters instead?

Done.

http://codereview.chromium.org/6854002/diff/16/drm-tool.cc#newcode24
drm-tool.cc:24: printf("Udev drm callback\n");
On 2011/04/14 02:36:05, tfarina wrote:
> Could you use some other macro from logging.h instead?

Hmm, but other *tool.cc files use printf.

http://codereview.chromium.org/6854002/diff/16/drm-tool.cc#newcode28
drm-tool.cc:28: int main(int argc, char* argv[]) {
On 2011/04/14 02:36:05, tfarina wrote:
> I'd change this to char** argv (just personal preference).

All of chrome OS (including other tools) uses int main(int argc, char* argv[])

http://codereview.chromium.org/6854002/diff/16/drm-tool.cc#newcode38
drm-tool.cc:38: 
On 2011/04/14 02:36:05, tfarina wrote:
> remove this blank line.

Done.

http://codereview.chromium.org/6854002/diff/16/powerd.h
File powerd.h (right):

http://codereview.chromium.org/6854002/diff/16/powerd.h#newcode23
powerd.h:23: #include "power_manager/monitor_reconfigure.h"
On 2011/04/14 02:36:05, tfarina wrote:
> please, could you forward declare this instead?

Done.

http://codereview.chromium.org/6854002/diff/16/udev_listener.cc
File udev_listener.cc (right):

http://codereview.chromium.org/6854002/diff/16/udev_listener.cc#newcode9
udev_listener.cc:9: #include "power_manager/udev_listener.h"
On 2011/04/14 02:36:05, tfarina wrote:
> this should be included first. Please move it to line 5.

Why? The convention is to order the headers alphabetically.

http://codereview.chromium.org/6854002/diff/16/udev_listener.cc#newcode80
udev_listener.cc:80: 
On 2011/04/14 02:36:05, tfarina wrote:
> remove this blank line.

Done.

[tfarina, 2011-04-14 14:53:58]

http://codereview.chromium.org/6854002/diff/16/udev_listener.cc
File udev_listener.cc (right):

http://codereview.chromium.org/6854002/diff/16/udev_listener.cc#newcode9
udev_listener.cc:9: #include "power_manager/udev_listener.h"
On 2011/04/14 02:52:10, marcheu wrote:
> On 2011/04/14 02:36:05, tfarina wrote:
> > this should be included first. Please move it to line 5.
> 
> Why? The convention is to order the headers alphabetically.

See
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...

And:
http://www.chromium.org/developers/coding-style

Also, a quick look into the other files will show you that too ;)

[tfarina, 2011-04-14 15:11:41]

http://codereview.chromium.org/6854002/diff/1015/monitor_reconfigure.cc
File monitor_reconfigure.cc (right):

http://codereview.chromium.org/6854002/diff/1015/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:16: 
extra line.

http://codereview.chromium.org/6854002/diff/1015/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:25: static const char kLcdOutputName[] = "LVDS1";
please, move this to an unnamed namespace and remove the static keyword.

Like:

namespace {
...
}  // namespace

namespace power_manager { ... }

Could you also add some documentation to these variables? One line is fine ;)

http://codereview.chromium.org/6854002/diff/1015/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:50: listener_ = new UdevListener(callback_,
std::string("drm"));
std::string("..") is needed? Can't you pass just "drm" instead?

http://codereview.chromium.org/6854002/diff/1015/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:55: {
the left brace "{" should be on the end of the previous line.

http://codereview.chromium.org/6854002/diff/1015/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:108: 
extra line.

http://codereview.chromium.org/6854002/diff/1015/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:207: RROutput output, const XRROutputInfo* output_info,
please, when it doesn't fit, write one argument per line. As it's more readable.

http://codereview.chromium.org/6854002/diff/1015/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:241: }  // end namespace power_manager
nit: } // namespace power_manager

http://codereview.chromium.org/6854002/diff/1015/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:242: 
get rid of this line.

http://codereview.chromium.org/6854002/diff/1015/monitor_reconfigure.h
File monitor_reconfigure.h (right):

http://codereview.chromium.org/6854002/diff/1015/monitor_reconfigure.h#newcode32
monitor_reconfigure.h:32: };
DISALLOW_COPY_ ?

http://codereview.chromium.org/6854002/diff/1015/monitor_reconfigure.h#newcode95
monitor_reconfigure.h:95: 
DISALLOW_COPY_AND_ASSING? If not, remove this empty line.

http://codereview.chromium.org/6854002/diff/1015/resolution_selector.cc
File resolution_selector.cc (right):

http://codereview.chromium.org/6854002/diff/1015/resolution_selector.cc#newco...
resolution_selector.cc:14: 
extra line.

http://codereview.chromium.org/6854002/diff/1015/resolution_selector.cc#newco...
resolution_selector.cc:16: 
extra line.

http://codereview.chromium.org/6854002/diff/1015/resolution_selector_test.cc
File resolution_selector_test.cc (right):

http://codereview.chromium.org/6854002/diff/1015/resolution_selector_test.cc#...
resolution_selector_test.cc:1: // Copyright (c) 2011 The Chromium OS Authors.
All rights reserved.
please, rename this file to resolution_selector_unittest.cc. foo_test.cc is used
in window_manager.

$ ls | grep test

http://codereview.chromium.org/6854002/diff/1015/resolution_selector_test.cc#...
resolution_selector_test.cc:39: width, height, StringPrintf("%dx%d", width,
height), id));
pĺease, include base/stringprintf.h instead of string_util.h and call it as
base::StringPrintf.

http://codereview.chromium.org/6854002/diff/1015/resolution_selector_test.cc#...
resolution_selector_test.cc:155: #if 0
Do you need this code? If not, could you remove it?

http://codereview.chromium.org/6854002/diff/1015/resolution_selector_test.cc#...
resolution_selector_test.cc:169: 
nit: remove this blank line.

[marcheu, 2011-04-14 17:43:40]

Did the changes, still need to figure out why the resolution test doesn't run
yet.

http://codereview.chromium.org/6854002/diff/1015/monitor_reconfigure.cc
File monitor_reconfigure.cc (right):

http://codereview.chromium.org/6854002/diff/1015/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:16: 
On 2011/04/14 15:11:41, tfarina wrote:
> extra line.

Done.

http://codereview.chromium.org/6854002/diff/1015/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:25: static const char kLcdOutputName[] = "LVDS1";
On 2011/04/14 15:11:41, tfarina wrote:
> please, move this to an unnamed namespace and remove the static keyword.
> 
> Like:
> 
> namespace {
> ...
> }  // namespace
> 
> namespace power_manager { ... }
> 
> Could you also add some documentation to these variables? One line is fine ;)

Done.

http://codereview.chromium.org/6854002/diff/1015/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:50: listener_ = new UdevListener(callback_,
std::string("drm"));
On 2011/04/14 15:11:41, tfarina wrote:
> std::string("..") is needed? Can't you pass just "drm" instead?

Done.

http://codereview.chromium.org/6854002/diff/1015/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:55: {
On 2011/04/14 15:11:41, tfarina wrote:
> the left brace "{" should be on the end of the previous line.

Done.

http://codereview.chromium.org/6854002/diff/1015/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:108: 
On 2011/04/14 15:11:41, tfarina wrote:
> extra line.

Done.

http://codereview.chromium.org/6854002/diff/1015/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:207: RROutput output, const XRROutputInfo* output_info,
On 2011/04/14 15:11:41, tfarina wrote:
> please, when it doesn't fit, write one argument per line. As it's more
readable.

Done.

http://codereview.chromium.org/6854002/diff/1015/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:241: }  // end namespace power_manager
On 2011/04/14 15:11:41, tfarina wrote:
> nit: } // namespace power_manager

Done.

http://codereview.chromium.org/6854002/diff/1015/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:242: 
On 2011/04/14 15:11:41, tfarina wrote:
> get rid of this line.

Done.

http://codereview.chromium.org/6854002/diff/1015/resolution_selector.cc
File resolution_selector.cc (right):

http://codereview.chromium.org/6854002/diff/1015/resolution_selector.cc#newco...
resolution_selector.cc:14: 
On 2011/04/14 15:11:41, tfarina wrote:
> extra line.

Done.

http://codereview.chromium.org/6854002/diff/1015/resolution_selector.cc#newco...
resolution_selector.cc:16: 
On 2011/04/14 15:11:41, tfarina wrote:
> extra line.

Done.

http://codereview.chromium.org/6854002/diff/1015/resolution_selector_test.cc
File resolution_selector_test.cc (right):

http://codereview.chromium.org/6854002/diff/1015/resolution_selector_test.cc#...
resolution_selector_test.cc:1: // Copyright (c) 2011 The Chromium OS Authors.
All rights reserved.
On 2011/04/14 15:11:41, tfarina wrote:
> please, rename this file to resolution_selector_unittest.cc. foo_test.cc is
used
> in window_manager.
> 
> $ ls | grep test

Done.

http://codereview.chromium.org/6854002/diff/1015/resolution_selector_test.cc#...
resolution_selector_test.cc:155: #if 0
On 2011/04/14 15:11:41, tfarina wrote:
> Do you need this code? If not, could you remove it?

Done.

http://codereview.chromium.org/6854002/diff/1015/resolution_selector_test.cc#...
resolution_selector_test.cc:169: 
On 2011/04/14 15:11:41, tfarina wrote:
> nit: remove this blank line.

Done.

[marcheu, 2011-04-14 18:37:46]

On 2011/04/14 17:43:40, marcheu wrote:
> Did the changes, still need to figure out why the resolution test doesn't run
> yet.

Okay, I had to wire it in the ebuild actually. Then I'm satisfied with the code,
PTAL.

[tfarina, 2011-04-14 18:54:18]

http://codereview.chromium.org/6854002/diff/7015/monitor_reconfigure.cc
File monitor_reconfigure.cc (right):

http://codereview.chromium.org/6854002/diff/7015/monitor_reconfigure.cc#newcode5
monitor_reconfigure.cc:5: #include "power_manager/backlight_controller.h"
While you are here, move this include down, to line 15.

http://codereview.chromium.org/6854002/diff/7015/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:23: // Namf of the internal output.
please, no two spaces inside this namespace. Also, typo: Namf -> Name?

http://codereview.chromium.org/6854002/diff/7015/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:25: // The screen DPI we return to X11.
I'd suggest: "we pass" or "we send back"

http://codereview.chromium.org/6854002/diff/7015/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:30: namespace power_manager {
add a blank line above.

http://codereview.chromium.org/6854002/diff/7015/resolution_selector_unittest.cc
File resolution_selector_unittest.cc (right):

http://codereview.chromium.org/6854002/diff/7015/resolution_selector_unittest...
resolution_selector_unittest.cc:12: #include "base/string_util.h"
string_util.h -> stringprint.h ? base::StringPrintf below?

http://codereview.chromium.org/6854002/diff/7015/udev_listener.cc
File udev_listener.cc (right):

http://codereview.chromium.org/6854002/diff/7015/udev_listener.cc#newcode7
udev_listener.cc:7: #include <glib.h>
I'd remove this include as it's already included in the header file.

http://codereview.chromium.org/6854002/diff/7015/udev_listener.cc#newcode10
udev_listener.cc:10: #include "base/platform_thread.h"
where do you use this?

http://codereview.chromium.org/6854002/diff/7015/udev_listener.cc#newcode81
udev_listener.cc:81: 
remove.

[marcheu, 2011-04-14 19:01:51]

Should have fixed all nits.

http://codereview.chromium.org/6854002/diff/7015/monitor_reconfigure.cc
File monitor_reconfigure.cc (right):

http://codereview.chromium.org/6854002/diff/7015/monitor_reconfigure.cc#newcode5
monitor_reconfigure.cc:5: #include "power_manager/backlight_controller.h"
On 2011/04/14 18:54:18, tfarina wrote:
> While you are here, move this include down, to line 15.

Done.

http://codereview.chromium.org/6854002/diff/7015/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:23: // Namf of the internal output.
On 2011/04/14 18:54:18, tfarina wrote:
> please, no two spaces inside this namespace. Also, typo: Namf -> Name?

Done.

http://codereview.chromium.org/6854002/diff/7015/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:25: // The screen DPI we return to X11.
On 2011/04/14 18:54:18, tfarina wrote:
> I'd suggest: "we pass" or "we send back"

Done.

http://codereview.chromium.org/6854002/diff/7015/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:30: namespace power_manager {
On 2011/04/14 18:54:18, tfarina wrote:
> add a blank line above.

Done.

http://codereview.chromium.org/6854002/diff/7015/resolution_selector_unittest.cc
File resolution_selector_unittest.cc (right):

http://codereview.chromium.org/6854002/diff/7015/resolution_selector_unittest...
resolution_selector_unittest.cc:12: #include "base/string_util.h"
On 2011/04/14 18:54:18, tfarina wrote:
> string_util.h -> stringprint.h ? base::StringPrintf below?

Done.

http://codereview.chromium.org/6854002/diff/7015/udev_listener.cc
File udev_listener.cc (right):

http://codereview.chromium.org/6854002/diff/7015/udev_listener.cc#newcode7
udev_listener.cc:7: #include <glib.h>
On 2011/04/14 18:54:18, tfarina wrote:
> I'd remove this include as it's already included in the header file.

Done.

http://codereview.chromium.org/6854002/diff/7015/udev_listener.cc#newcode10
udev_listener.cc:10: #include "base/platform_thread.h"
On 2011/04/14 18:54:18, tfarina wrote:
> where do you use this?

Removed.

http://codereview.chromium.org/6854002/diff/7015/udev_listener.cc#newcode81
udev_listener.cc:81: 
On 2011/04/14 18:54:18, tfarina wrote:
> remove.

Done.

[Daniel Erat, 2011-04-14 19:07:24]

I started writing these before the latest upload but they're still relevant.

http://codereview.chromium.org/6854002/diff/7015/monitor_reconfigure.h
File monitor_reconfigure.h (right):

http://codereview.chromium.org/6854002/diff/7015/monitor_reconfigure.h#newcode5
monitor_reconfigure.h:5: #ifndef POWER_MANAGER_MONITOR_RECONFIGURE_MAIN_H_
nit: this should just be POWER_MANAGER_MONITOR_RECONFIGURE_H_

http://codereview.chromium.org/6854002/diff/7015/monitor_reconfigure.h#newcode29
monitor_reconfigure.h:29: void Run(GIOChannel* source, GIOCondition condition);
nit: add virtual (per style guide)

http://codereview.chromium.org/6854002/diff/7015/monitor_reconfigure.h#newcode37
monitor_reconfigure.h:37: class MonitorReconfigureMain {
rename this class to MonitorReconfigure to match the filename?

(i'd suggest a name like DisplayModeSelector but don't have really strong
feelings about this, so feel free to ignore)

http://codereview.chromium.org/6854002/diff/7015/monitor_reconfigure.h#newcode93
monitor_reconfigure.h:93: // Udev linstener.
s/linstener/listener/

http://codereview.chromium.org/6854002/diff/7015/monitor_reconfigure.h#newcode95
monitor_reconfigure.h:95: 
nit: delete extra blank line

[Daniel Erat, 2011-04-14 19:18:46]

http://codereview.chromium.org/6854002/diff/7021/monitor_reconfigure.cc
File monitor_reconfigure.cc (right):

http://codereview.chromium.org/6854002/diff/7021/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:11: #include "base/command_line.h"
don't think this is used

http://codereview.chromium.org/6854002/diff/7021/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:13: #include "base/string_util.h"
don't think this is used

http://codereview.chromium.org/6854002/diff/7021/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:17: using std::map;
don't think this is used

http://codereview.chromium.org/6854002/diff/7021/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:19: using std::string;
don't think this is used

http://codereview.chromium.org/6854002/diff/7021/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:41: listener_(NULL) {}
also initialize display_, window_, lcd_output_, and external_output_info_? 
(ditto for other c'tor)

http://codereview.chromium.org/6854002/diff/7021/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:49: MonitorReconfigureMain::~MonitorReconfigureMain() {
if (display_)
  XCloseDisplay(display_);

?

http://codereview.chromium.org/6854002/diff/7021/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:53: callback_ = new MonitorReconfigureCallback(this);
DCHECK(callback_ == NULL) first to make sure that this is only called once?

http://codereview.chromium.org/6854002/diff/7021/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:59: lcd_output_ = 0;
DCHECK(display_ == NULL) first to make sure that this is only called once?

http://codereview.chromium.org/6854002/diff/7021/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:109: // FIXME: here set the gamma ramp.
nit: change to TODO(marcheu)

http://codereview.chromium.org/6854002/diff/7021/powerd_unittest.cc
File powerd_unittest.cc (right):

http://codereview.chromium.org/6854002/diff/7021/powerd_unittest.cc#newcode15
powerd_unittest.cc:15: #include "power_manager/mock_monitor_reconfigure.h"
nit: move before mock_video_detector.h

http://codereview.chromium.org/6854002/diff/7021/udev_listener.h
File udev_listener.h (right):

http://codereview.chromium.org/6854002/diff/7021/udev_listener.h#newcode23
udev_listener.h:23: UdevListener(UdevCallback* callback, std::string subsystem);
nit: const std::string&

http://codereview.chromium.org/6854002/diff/7021/udev_listener.h#newcode38
udev_listener.h:38: };
include "base/basictypes.h" and add DISALLOW_COPY_AND_ASSIGN(UdevListener); at
the end of the private section

[marcheu, 2011-04-14 19:54:57]

http://codereview.chromium.org/6854002/diff/7015/monitor_reconfigure.h
File monitor_reconfigure.h (right):

http://codereview.chromium.org/6854002/diff/7015/monitor_reconfigure.h#newcode5
monitor_reconfigure.h:5: #ifndef POWER_MANAGER_MONITOR_RECONFIGURE_MAIN_H_
On 2011/04/14 19:07:24, Daniel Erat wrote:
> nit: this should just be POWER_MANAGER_MONITOR_RECONFIGURE_H_

Done.

http://codereview.chromium.org/6854002/diff/7015/monitor_reconfigure.h#newcode29
monitor_reconfigure.h:29: void Run(GIOChannel* source, GIOCondition condition);
On 2011/04/14 19:07:24, Daniel Erat wrote:
> nit: add virtual (per style guide)

Done.

http://codereview.chromium.org/6854002/diff/7015/monitor_reconfigure.h#newcode37
monitor_reconfigure.h:37: class MonitorReconfigureMain {
On 2011/04/14 19:07:24, Daniel Erat wrote:
> rename this class to MonitorReconfigure to match the filename?
> 
> (i'd suggest a name like DisplayModeSelector but don't have really strong
> feelings about this, so feel free to ignore)

Done.

http://codereview.chromium.org/6854002/diff/7015/monitor_reconfigure.h#newcode93
monitor_reconfigure.h:93: // Udev linstener.
On 2011/04/14 19:07:24, Daniel Erat wrote:
> s/linstener/listener/

Done.

http://codereview.chromium.org/6854002/diff/7015/monitor_reconfigure.h#newcode95
monitor_reconfigure.h:95: 
On 2011/04/14 19:07:24, Daniel Erat wrote:
> nit: delete extra blank line

Done.

http://codereview.chromium.org/6854002/diff/7015/resolution_selector_unittest.cc
File resolution_selector_unittest.cc (right):

http://codereview.chromium.org/6854002/diff/7015/resolution_selector_unittest...
resolution_selector_unittest.cc:12: #include "base/string_util.h"
On 2011/04/14 19:01:51, marcheu wrote:
> On 2011/04/14 18:54:18, tfarina wrote:
> > string_util.h -> stringprint.h ? base::StringPrintf below?
> 
> Done.

Actually I take that back, it doesn't compile with that, so I reverted.

http://codereview.chromium.org/6854002/diff/7021/monitor_reconfigure.cc
File monitor_reconfigure.cc (right):

http://codereview.chromium.org/6854002/diff/7021/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:11: #include "base/command_line.h"
On 2011/04/14 19:18:46, Daniel Erat wrote:
> don't think this is used

Done.

http://codereview.chromium.org/6854002/diff/7021/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:13: #include "base/string_util.h"
On 2011/04/14 19:18:46, Daniel Erat wrote:
> don't think this is used

Done.

http://codereview.chromium.org/6854002/diff/7021/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:17: using std::map;
On 2011/04/14 19:18:46, Daniel Erat wrote:
> don't think this is used

Done.

http://codereview.chromium.org/6854002/diff/7021/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:19: using std::string;
On 2011/04/14 19:18:46, Daniel Erat wrote:
> don't think this is used

Done.

http://codereview.chromium.org/6854002/diff/7021/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:41: listener_(NULL) {}
On 2011/04/14 19:18:46, Daniel Erat wrote:
> also initialize display_, window_, lcd_output_, and external_output_info_? 
> (ditto for other c'tor)

Done.

http://codereview.chromium.org/6854002/diff/7021/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:49: MonitorReconfigureMain::~MonitorReconfigureMain() {
On 2011/04/14 19:18:46, Daniel Erat wrote:
> if (display_)
>   XCloseDisplay(display_);
> 
> ?

Indeed, but I'll put this at the end of Run() since that's where XOpenDisplay
happens.

http://codereview.chromium.org/6854002/diff/7021/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:53: callback_ = new MonitorReconfigureCallback(this);
On 2011/04/14 19:18:46, Daniel Erat wrote:
> DCHECK(callback_ == NULL) first to make sure that this is only called once?

Done.

http://codereview.chromium.org/6854002/diff/7021/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:59: lcd_output_ = 0;
On 2011/04/14 19:18:46, Daniel Erat wrote:
> DCHECK(display_ == NULL) first to make sure that this is only called once?

No, the idea here is to keep the class around and call Run() many times if we
need to. For example Run() gets called on HandleResume().

http://codereview.chromium.org/6854002/diff/7021/monitor_reconfigure.cc#newco...
monitor_reconfigure.cc:109: // FIXME: here set the gamma ramp.
On 2011/04/14 19:18:46, Daniel Erat wrote:
> nit: change to TODO(marcheu)

Done.

http://codereview.chromium.org/6854002/diff/7021/powerd_unittest.cc
File powerd_unittest.cc (right):

http://codereview.chromium.org/6854002/diff/7021/powerd_unittest.cc#newcode15
powerd_unittest.cc:15: #include "power_manager/mock_monitor_reconfigure.h"
On 2011/04/14 19:18:46, Daniel Erat wrote:
> nit: move before mock_video_detector.h

Done.

http://codereview.chromium.org/6854002/diff/7021/udev_listener.h
File udev_listener.h (right):

http://codereview.chromium.org/6854002/diff/7021/udev_listener.h#newcode23
udev_listener.h:23: UdevListener(UdevCallback* callback, std::string subsystem);
On 2011/04/14 19:18:46, Daniel Erat wrote:
> nit: const std::string&

Done.

http://codereview.chromium.org/6854002/diff/7021/udev_listener.h#newcode38
udev_listener.h:38: };
On 2011/04/14 19:18:46, Daniel Erat wrote:
> include "base/basictypes.h" and add DISALLOW_COPY_AND_ASSIGN(UdevListener); at
> the end of the private section

Done.

[tfarina, 2011-04-14 19:58:29]

http://codereview.chromium.org/6854002/diff/7015/resolution_selector_unittest.cc
File resolution_selector_unittest.cc (right):

http://codereview.chromium.org/6854002/diff/7015/resolution_selector_unittest...
resolution_selector_unittest.cc:12: #include "base/string_util.h"
On 2011/04/14 19:54:57, marcheu wrote:
> On 2011/04/14 19:01:51, marcheu wrote:
> > On 2011/04/14 18:54:18, tfarina wrote:
> > > string_util.h -> stringprint.h ? base::StringPrintf below?
> > 
> > Done.
> 
> Actually I take that back, it doesn't compile with that, so I reverted.

OK, it should be using a revision of base that doesn't have stringprintf.h yet.

[tfarina, 2011-04-14 20:03:57]

http://codereview.chromium.org/6854002/diff/6023/monitor_reconfigure.h
File monitor_reconfigure.h (right):

http://codereview.chromium.org/6854002/diff/6023/monitor_reconfigure.h#newcode9
monitor_reconfigure.h:9: #include <string>
you can get rid of this include.

http://codereview.chromium.org/6854002/diff/6023/monitor_reconfigure.h#newcode15
monitor_reconfigure.h:15: #include "power_manager/backlight_controller.h"
Please, forward declare this.

http://codereview.chromium.org/6854002/diff/6023/monitor_reconfigure.h#newcode17
monitor_reconfigure.h:17: #include "power_manager/udev_listener.h"
please, forward declare this.

http://codereview.chromium.org/6854002/diff/6023/monitor_reconfigure.h#newcode95
monitor_reconfigure.h:95: };
Is there a reason to this class to not have a DISALLOW here too?

http://codereview.chromium.org/6854002/diff/6023/udev_listener.h
File udev_listener.h (right):

http://codereview.chromium.org/6854002/diff/6023/udev_listener.h#newcode8
udev_listener.h:8: #include <string>
this should be included after libudev.h since it's a C++ include.

http://codereview.chromium.org/6854002/diff/6023/udev_listener.h#newcode17
udev_listener.h:17: class UdevCallback {
Thinking about this now, I'd rename this to UdevDelegate.

http://codereview.chromium.org/6854002/diff/6023/udev_listener.h#newcode23
udev_listener.h:23: class UdevListener {
And maybe this to UdevController? I know it's listener because of EventHandler,
but on chromium we tend to use Listener for interfaces.

[marcheu, 2011-04-14 20:41:23]

http://codereview.chromium.org/6854002/diff/6023/monitor_reconfigure.h
File monitor_reconfigure.h (right):

http://codereview.chromium.org/6854002/diff/6023/monitor_reconfigure.h#newcode9
monitor_reconfigure.h:9: #include <string>
On 2011/04/14 20:03:57, tfarina wrote:
> you can get rid of this include.

Done.

http://codereview.chromium.org/6854002/diff/6023/monitor_reconfigure.h#newcode15
monitor_reconfigure.h:15: #include "power_manager/backlight_controller.h"
On 2011/04/14 20:03:57, tfarina wrote:
> Please, forward declare this.

Done.

http://codereview.chromium.org/6854002/diff/6023/monitor_reconfigure.h#newcode17
monitor_reconfigure.h:17: #include "power_manager/udev_listener.h"
On 2011/04/14 20:03:57, tfarina wrote:
> please, forward declare this.

MonitorReconfigureCallback inherits UdevCallback so I don't think we can do
this.

http://codereview.chromium.org/6854002/diff/6023/monitor_reconfigure.h#newcode95
monitor_reconfigure.h:95: };
On 2011/04/14 20:03:57, tfarina wrote:
> Is there a reason to this class to not have a DISALLOW here too?

Done.

http://codereview.chromium.org/6854002/diff/6023/udev_listener.h
File udev_listener.h (right):

http://codereview.chromium.org/6854002/diff/6023/udev_listener.h#newcode8
udev_listener.h:8: #include <string>
On 2011/04/14 20:03:57, tfarina wrote:
> this should be included after libudev.h since it's a C++ include.

Done.

http://codereview.chromium.org/6854002/diff/6023/udev_listener.h#newcode17
udev_listener.h:17: class UdevCallback {
On 2011/04/14 20:03:57, tfarina wrote:
> Thinking about this now, I'd rename this to UdevDelegate.

Done.

http://codereview.chromium.org/6854002/diff/6023/udev_listener.h#newcode23
udev_listener.h:23: class UdevListener {
On 2011/04/14 20:03:57, tfarina wrote:
> And maybe this to UdevController? I know it's listener because of
EventHandler,
> but on chromium we tend to use Listener for interfaces.

Done.

[Daniel Erat, 2011-04-14 20:48:37]

LGTM after a few nits!

http://codereview.chromium.org/6854002/diff/24/resolution_selector.cc
File resolution_selector.cc (right):

http://codereview.chromium.org/6854002/diff/24/resolution_selector.cc#newcode11
resolution_selector.cc:11: using std::string;
nit: delete this

http://codereview.chromium.org/6854002/diff/24/resolution_selector_unittest.cc
File resolution_selector_unittest.cc (right):

http://codereview.chromium.org/6854002/diff/24/resolution_selector_unittest.c...
resolution_selector_unittest.cc:17: using std::string;
nit: delete this

http://codereview.chromium.org/6854002/diff/24/udev_controller.cc
File udev_controller.cc (right):

http://codereview.chromium.org/6854002/diff/24/udev_controller.cc#newcode55
udev_controller.cc:55: GIOCondition condition,
nit: fix indenting

http://codereview.chromium.org/6854002/diff/24/udev_controller.h
File udev_controller.h (right):

http://codereview.chromium.org/6854002/diff/24/udev_controller.h#newcode7
udev_controller.h:7: 
nit: delete extra blank line

[marcheu, 2011-04-14 20:59:54]

Patch set should say round 7, but my fat fingers slipped. oops.

http://codereview.chromium.org/6854002/diff/24/resolution_selector.cc
File resolution_selector.cc (right):

http://codereview.chromium.org/6854002/diff/24/resolution_selector.cc#newcode11
resolution_selector.cc:11: using std::string;
On 2011/04/14 20:48:37, Daniel Erat wrote:
> nit: delete this

Done.

http://codereview.chromium.org/6854002/diff/24/resolution_selector_unittest.cc
File resolution_selector_unittest.cc (right):

http://codereview.chromium.org/6854002/diff/24/resolution_selector_unittest.c...
resolution_selector_unittest.cc:17: using std::string;
On 2011/04/14 20:48:37, Daniel Erat wrote:
> nit: delete this

Done.

http://codereview.chromium.org/6854002/diff/24/udev_controller.cc
File udev_controller.cc (right):

http://codereview.chromium.org/6854002/diff/24/udev_controller.cc#newcode55
udev_controller.cc:55: GIOCondition condition,
On 2011/04/14 20:48:37, Daniel Erat wrote:
> nit: fix indenting

Done.

http://codereview.chromium.org/6854002/diff/24/udev_controller.h
File udev_controller.h (right):

http://codereview.chromium.org/6854002/diff/24/udev_controller.h#newcode7
udev_controller.h:7: 
On 2011/04/14 20:48:37, Daniel Erat wrote:
> nit: delete extra blank line

Done.

[tfarina, 2011-04-15 15:00:37]

http://codereview.chromium.org/6854002/diff/1021/resolution_selector.h
File resolution_selector.h (right):

http://codereview.chromium.org/6854002/diff/1021/resolution_selector.h#newcode23
resolution_selector.h:23: Mode(int width, int height, std::string name, int id)
Could you move the implementation to the source file?

http://codereview.chromium.org/6854002/diff/1021/resolution_selector.h#newcode53
resolution_selector.h:53: static const int kMaxProjectorPixels;
Is this constant going to be used by someone else? I didn't check. If not, could
you move to the source file into an unnamed namespace?

http://codereview.chromium.org/6854002/diff/1021/resolution_selector.h#newcode55
resolution_selector.h:55: ResolutionSelector() {}
please, move the implementation to the source file.

http://codereview.chromium.org/6854002/diff/1021/resolution_selector.h#newcode63
resolution_selector.h:63: return mode_a.width * mode_a.height > mode_b.width *
mode_b.height;
Could you move the implementation to the source file?

http://codereview.chromium.org/6854002/diff/1021/udev_controller.h
File udev_controller.h (right):

http://codereview.chromium.org/6854002/diff/1021/udev_controller.h#newcode10
udev_controller.h:10: #include <string>
please, add a line between libudev.h and string.

http://codereview.chromium.org/6854002/diff/1021/udev_controller.h#newcode26
udev_controller.h:26: bool Init();
please add a line above.

http://codereview.chromium.org/6854002/diff/1021/udev_controller.h#newcode38
udev_controller.h:38: std::string subsystem_;
I'd suggest moving this to line 35 (so it matches with the order in the ctor).
Also, could you document this variable?

[marcheu, 2011-04-15 17:32:23]

http://codereview.chromium.org/6854002/diff/1021/resolution_selector.h
File resolution_selector.h (right):

http://codereview.chromium.org/6854002/diff/1021/resolution_selector.h#newcode23
resolution_selector.h:23: Mode(int width, int height, std::string name, int id)
On 2011/04/15 15:00:37, tfarina wrote:
> Could you move the implementation to the source file?

Done.

http://codereview.chromium.org/6854002/diff/1021/resolution_selector.h#newcode53
resolution_selector.h:53: static const int kMaxProjectorPixels;
On 2011/04/15 15:00:37, tfarina wrote:
> Is this constant going to be used by someone else? I didn't check. If not,
could
> you move to the source file into an unnamed namespace?

No it's used by the unittest. Please look at the code next time.

http://codereview.chromium.org/6854002/diff/1021/resolution_selector.h#newcode55
resolution_selector.h:55: ResolutionSelector() {}
On 2011/04/15 15:00:37, tfarina wrote:
> please, move the implementation to the source file.

Done.

http://codereview.chromium.org/6854002/diff/1021/resolution_selector.h#newcode63
resolution_selector.h:63: return mode_a.width * mode_a.height > mode_b.width *
mode_b.height;
On 2011/04/15 15:00:37, tfarina wrote:
> Could you move the implementation to the source file?

Done.

http://codereview.chromium.org/6854002/diff/1021/udev_controller.h
File udev_controller.h (right):

http://codereview.chromium.org/6854002/diff/1021/udev_controller.h#newcode10
udev_controller.h:10: #include <string>
On 2011/04/15 15:00:37, tfarina wrote:
> please, add a line between libudev.h and string.

Done.

http://codereview.chromium.org/6854002/diff/1021/udev_controller.h#newcode26
udev_controller.h:26: bool Init();
On 2011/04/15 15:00:37, tfarina wrote:
> please add a line above.

Done.

http://codereview.chromium.org/6854002/diff/1021/udev_controller.h#newcode38
udev_controller.h:38: std::string subsystem_;
On 2011/04/15 15:00:37, tfarina wrote:
> I'd suggest moving this to line 35 (so it matches with the order in the ctor).
> Also, could you document this variable?

Done.