Add DeviceMonitorLinux in BrowserMainLoop.

Add DeviceMonitorLinux in BrowserMainLoop.
The DeviceMonitorLinux detects device change and sends the signal to SystemMonitor.

This patch depends on http://codereview.chromium.org/10836004/

BUG=137799
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=149266

[tommi (sloooow) - chröme, 2012-07-30 11:07:03]

http://codereview.chromium.org/10829073/diff/1/content/browser/browser_main_l...
File content/browser/browser_main_loop.cc (right):

http://codereview.chromium.org/10829073/diff/1/content/browser/browser_main_l...
content/browser/browser_main_loop.cc:353:
devices_monitor_.reset(DevicesMonitor::Create());
inside an #else?

http://codereview.chromium.org/10829073/diff/1/content/browser/browser_main_l...
File content/browser/browser_main_loop.h (right):

http://codereview.chromium.org/10829073/diff/1/content/browser/browser_main_l...
content/browser/browser_main_loop.h:47: class DevicesMonitor;
"devices monitor" doesn't sound right.  Even though it can monitor multiple
devices, I think we should name it DeviceMonitor.

http://codereview.chromium.org/10829073/diff/1/content/browser/browser_main_l...
content/browser/browser_main_loop.h:81: static content::DevicesMonitor*
GetDevicesMonitor();
Why do we need this getter?

http://codereview.chromium.org/10829073/diff/1/content/browser/browser_main_l...
content/browser/browser_main_loop.h:111: scoped_ptr<DevicesMonitor>
devices_monitor_;
should this be in an #else?  This is for posix what SystemMessageWindowWin is
for Windows, right?

http://codereview.chromium.org/10829073/diff/1/content/browser/devices_monito...
File content/browser/devices_monitor/devices_monitor.h (right):

http://codereview.chromium.org/10829073/diff/1/content/browser/devices_monito...
content/browser/devices_monitor/devices_monitor.h:23: virtual void
Start(base::SystemMonitor::DevicesChangedObserver* observer) = 0;
I'm not grokking why we need to support observers here.  Can't we push
notifications into SystemMonitor in the same way as SystemMessageWindowWin does
on Windows?

http://codereview.chromium.org/10829073/diff/1/content/browser/devices_monito...
File content/browser/devices_monitor/devices_monitor_linux.cc (right):

http://codereview.chromium.org/10829073/diff/1/content/browser/devices_monito...
content/browser/devices_monitor/devices_monitor_linux.cc:27: static const
SubsystemMap subsystem_map[] = {
kSubsystemMap

http://codereview.chromium.org/10829073/diff/1/content/browser/devices_monito...
content/browser/devices_monitor/devices_monitor_linux.cc:28:
{base::SystemMonitor::DEVTYPE_AUDIO_CAPTURE, kAudioSubsystem, NULL},
space after { and before }

http://codereview.chromium.org/10829073/diff/1/content/browser/devices_monito...
content/browser/devices_monitor/devices_monitor_linux.cc:32: static const int
subsystem_map_size =
kSubsystemMapSize

http://codereview.chromium.org/10829073/diff/1/content/browser/devices_monito...
content/browser/devices_monitor/devices_monitor_linux.cc:41: return new
DevicesMonitorLinux();
Here, we could (should?) post a task to the IO thread to call Start() using
BrowserThread::IO.

Also, perhaps it would be a good idea to monitor the IO thread for termination
and do cleanup when the IO thread goes away.

http://codereview.chromium.org/10829073/diff/1/content/browser/devices_monito...
content/browser/devices_monitor/devices_monitor_linux.cc:95:
base::SystemMonitor::Get()->ProcessDevicesChanged(device_type);
I see we don't use the observer map here and I don't think we really need it. 
What do you think about simply starting the monitor when Create() is called and
skip the observer map?

Is there perhaps some noticeable performance cost with monitoring the devices
that I'm not aware of?

http://codereview.chromium.org/10829073/diff/1/content/browser/devices_monito...
File content/browser/devices_monitor/devices_monitor_mac.h (right):

http://codereview.chromium.org/10829073/diff/1/content/browser/devices_monito...
content/browser/devices_monitor/devices_monitor_mac.h:14: class
DevicesMonitorMac : public DevicesMonitor {
do we need to add these empty classes now or can it wait until we have actual
implementations?  I think we might have an iteration or two on the linux
implementation before the interfaces are fully finalized and it would be good to
not have to go and maintain these empty classes.
Also, removing the empty classes makes this review shorter. :)

[wjia(left Chromium), 2012-07-30 18:28:42]

The new patch starts device monitoring when the browser is started on linux.

[tommi (sloooow) - chröme, 2012-07-30 19:45:11]

Great! Lgtm with a few minor things fixed.

http://codereview.chromium.org/10829073/diff/11001/content/browser/browser_ma...
File content/browser/browser_main_loop.cc (right):

http://codereview.chromium.org/10829073/diff/11001/content/browser/browser_ma...
content/browser/browser_main_loop.cc:70: #include
"content/browser/device_monitor_linux.h"
fyi - you might want to send a note to the developer that added the OS_OPENBSD
guard above a note just in case your change works for openbsd as well.

http://codereview.chromium.org/10829073/diff/11001/content/browser/device_mon...
File content/browser/device_monitor_linux.cc (right):

http://codereview.chromium.org/10829073/diff/11001/content/browser/device_mon...
content/browser/device_monitor_linux.cc:28: static const SubsystemMap
subsystem_map[] = {
kSubsystemMap

http://codereview.chromium.org/10829073/diff/11001/content/browser/device_mon...
content/browser/device_monitor_linux.cc:29:
{base::SystemMonitor::DEVTYPE_AUDIO_CAPTURE, kAudioSubsystem, NULL},
space after { and before }

http://codereview.chromium.org/10829073/diff/11001/content/browser/device_mon...
content/browser/device_monitor_linux.cc:33: static const int subsystem_map_size
=
You don't need this variable. Just use arraysize(kSubsystemMap) instead.

http://codereview.chromium.org/10829073/diff/11001/content/browser/device_mon...
content/browser/device_monitor_linux.cc:60: for (int i = 0; i <
subsystem_map_size; ++i) {
arraysize(kSubsystemMap)

http://codereview.chromium.org/10829073/diff/11001/content/browser/device_mon...
content/browser/device_monitor_linux.cc:83: for (int i = 0; i <
subsystem_map_size; ++i) {
arraysize(kSubsystemMap)

http://codereview.chromium.org/10829073/diff/11001/content/browser/device_mon...
File content/browser/device_monitor_linux.h (right):

http://codereview.chromium.org/10829073/diff/11001/content/browser/device_mon...
content/browser/device_monitor_linux.h:31: virtual void
WillDestroyCurrentMessageLoop() OVERRIDE;
nit: make this private since it's not supposed to be a part of the public
interface of the class.

[wjia(left Chromium), 2012-07-30 21:00:13]

Robert - FYI

http://codereview.chromium.org/10829073/diff/11001/content/browser/browser_ma...
File content/browser/browser_main_loop.cc (right):

http://codereview.chromium.org/10829073/diff/11001/content/browser/browser_ma...
content/browser/browser_main_loop.cc:70: #include
"content/browser/device_monitor_linux.h"
On 2012/07/30 19:45:11, tommi wrote:
> fyi - you might want to send a note to the developer that added the OS_OPENBSD
> guard above a note just in case your change works for openbsd as well.

Done.

http://codereview.chromium.org/10829073/diff/11001/content/browser/device_mon...
File content/browser/device_monitor_linux.cc (right):

http://codereview.chromium.org/10829073/diff/11001/content/browser/device_mon...
content/browser/device_monitor_linux.cc:28: static const SubsystemMap
subsystem_map[] = {
On 2012/07/30 19:45:11, tommi wrote:
> kSubsystemMap

Done.

http://codereview.chromium.org/10829073/diff/11001/content/browser/device_mon...
content/browser/device_monitor_linux.cc:29:
{base::SystemMonitor::DEVTYPE_AUDIO_CAPTURE, kAudioSubsystem, NULL},
On 2012/07/30 19:45:11, tommi wrote:
> space after { and before }

Done.

http://codereview.chromium.org/10829073/diff/11001/content/browser/device_mon...
content/browser/device_monitor_linux.cc:33: static const int subsystem_map_size
=
On 2012/07/30 19:45:11, tommi wrote:
> You don't need this variable. Just use arraysize(kSubsystemMap) instead.

Done.

http://codereview.chromium.org/10829073/diff/11001/content/browser/device_mon...
content/browser/device_monitor_linux.cc:60: for (int i = 0; i <
subsystem_map_size; ++i) {
On 2012/07/30 19:45:11, tommi wrote:
> arraysize(kSubsystemMap)

Done.

http://codereview.chromium.org/10829073/diff/11001/content/browser/device_mon...
content/browser/device_monitor_linux.cc:83: for (int i = 0; i <
subsystem_map_size; ++i) {
On 2012/07/30 19:45:11, tommi wrote:
> arraysize(kSubsystemMap)

Done.

http://codereview.chromium.org/10829073/diff/11001/content/browser/device_mon...
File content/browser/device_monitor_linux.h (right):

http://codereview.chromium.org/10829073/diff/11001/content/browser/device_mon...
content/browser/device_monitor_linux.h:31: virtual void
WillDestroyCurrentMessageLoop() OVERRIDE;
On 2012/07/30 19:45:11, tommi wrote:
> nit: make this private since it's not supposed to be a part of the public
> interface of the class.

Done.

[Lei Zhang, 2012-07-31 00:04:58]

If I knew you were going to do this, I would have done for you with an alternate
patch instead of r148698, and saved you some work. Oh well.

Some nits below.

http://codereview.chromium.org/10829073/diff/3005/content/browser/device_moni...
File content/browser/device_monitor_linux.cc (right):

http://codereview.chromium.org/10829073/diff/3005/content/browser/device_moni...
content/browser/device_monitor_linux.cc:9: #include <libudev.h>
nit: Chromium style separates C headers from C++ headers.

http://codereview.chromium.org/10829073/diff/3005/content/browser/device_moni...
content/browser/device_monitor_linux.cc:24: static const char kAudioSubsystem[]
= "sound";
nit: you don't need static inside an anonymous namespace.

http://codereview.chromium.org/10829073/diff/3005/content/browser/device_moni...
content/browser/device_monitor_linux.cc:52: // We want to be notified of IO
message loop destruction to delete udev_.
nit, |udev_|.

http://codereview.chromium.org/10829073/diff/3005/content/browser/device_moni...
content/browser/device_monitor_linux.cc:76: if (device) {
|device| should never be NULL. You can put a DCHECK here instead of the if
statement.

http://codereview.chromium.org/10829073/diff/3005/content/browser/device_moni...
content/browser/device_monitor_linux.cc:81: if
(subsystem.compare(kSubsystemMap[i].subsystem) == 0) {
nit: you can just write if (subsystem == kSubsystemMap[i].subsystem) {

[wjia(left Chromium), 2012-07-31 01:20:01]

PTAL.

http://codereview.chromium.org/10829073/diff/3005/content/browser/device_moni...
File content/browser/device_monitor_linux.cc (right):

http://codereview.chromium.org/10829073/diff/3005/content/browser/device_moni...
content/browser/device_monitor_linux.cc:9: #include <libudev.h>
On 2012/07/31 00:04:58, Lei Zhang wrote:
> nit: Chromium style separates C headers from C++ headers.

Done.

http://codereview.chromium.org/10829073/diff/3005/content/browser/device_moni...
content/browser/device_monitor_linux.cc:24: static const char kAudioSubsystem[]
= "sound";
On 2012/07/31 00:04:58, Lei Zhang wrote:
> nit: you don't need static inside an anonymous namespace.

Done.

http://codereview.chromium.org/10829073/diff/3005/content/browser/device_moni...
content/browser/device_monitor_linux.cc:52: // We want to be notified of IO
message loop destruction to delete udev_.
On 2012/07/31 00:04:58, Lei Zhang wrote:
> nit, |udev_|.

Done.

http://codereview.chromium.org/10829073/diff/3005/content/browser/device_moni...
content/browser/device_monitor_linux.cc:76: if (device) {
On 2012/07/31 00:04:58, Lei Zhang wrote:
> |device| should never be NULL. You can put a DCHECK here instead of the if
> statement.

Done.

http://codereview.chromium.org/10829073/diff/3005/content/browser/device_moni...
content/browser/device_monitor_linux.cc:81: if
(subsystem.compare(kSubsystemMap[i].subsystem) == 0) {
On 2012/07/31 00:04:58, Lei Zhang wrote:
> nit: you can just write if (subsystem == kSubsystemMap[i].subsystem) {

Done.

[Lei Zhang, 2012-07-31 02:40:12]

http://codereview.chromium.org/10829073/diff/6005/content/browser/device_moni...
File content/browser/device_monitor_linux.cc (right):

http://codereview.chromium.org/10829073/diff/6005/content/browser/device_moni...
content/browser/device_monitor_linux.cc:78: base::SystemMonitor::DeviceType
device_type =
Given the subsystems you are filtering for, you shouldn't ever end up getting
DEVTYPE_UNKNOWN in the end, right?

http://codereview.chromium.org/10829073/diff/6005/content/browser/device_moni...
File content/browser/device_monitor_linux.h (right):

http://codereview.chromium.org/10829073/diff/6005/content/browser/device_moni...
content/browser/device_monitor_linux.h:34: virtual void Initialize();
This doesn't need to be virtual?

[wjia(left Chromium), 2012-07-31 02:53:10]

http://codereview.chromium.org/10829073/diff/6005/content/browser/device_moni...
File content/browser/device_monitor_linux.cc (right):

http://codereview.chromium.org/10829073/diff/6005/content/browser/device_moni...
content/browser/device_monitor_linux.cc:78: base::SystemMonitor::DeviceType
device_type =
On 2012/07/31 02:40:12, Lei Zhang wrote:
> Given the subsystems you are filtering for, you shouldn't ever end up getting
> DEVTYPE_UNKNOWN in the end, right?

That's correct. A DCHECK has been added.

http://codereview.chromium.org/10829073/diff/6005/content/browser/device_moni...
File content/browser/device_monitor_linux.h (right):

http://codereview.chromium.org/10829073/diff/6005/content/browser/device_moni...
content/browser/device_monitor_linux.h:34: virtual void Initialize();
On 2012/07/31 02:40:12, Lei Zhang wrote:
> This doesn't need to be virtual?

removed.

[Lei Zhang, 2012-07-31 02:54:19]

lgtm

[wjia(left Chromium), 2012-07-31 03:01:45]

add jam@ for owner's stamp.

[jam, 2012-07-31 15:46:05]

http://codereview.chromium.org/10829073/diff/13008/content/browser/browser_ma...
File content/browser/browser_main_loop.h (right):

http://codereview.chromium.org/10829073/diff/13008/content/browser/browser_ma...
content/browser/browser_main_loop.h:47: #if defined(OS_LINUX)
nit: can you move this after the class/struct section, so that the ifdef'd parts
are at the end (i.e. just like how we put includes inside ifdefs after the rest
of the other includes). also while you're at it, move the struct below after the
class WebKitThread

http://codereview.chromium.org/10829073/diff/13008/content/browser/device_mon...
File content/browser/device_monitor_linux.cc (right):

http://codereview.chromium.org/10829073/diff/13008/content/browser/device_mon...
content/browser/device_monitor_linux.cc:39: : io_loop_(NULL) {
we don't store MessageLoop pointers as a rule, since they're not safe (this is
why we have MessageLoopProxy, and TaskRunner). in this file though, it doesn't
appear you actually need to store it. the check in the destructor can be
replaced with dcheck (!BT::IsMLValid(io))

[wjia(left Chromium), 2012-07-31 18:19:07]

PTAL.

http://codereview.chromium.org/10829073/diff/13008/content/browser/browser_ma...
File content/browser/browser_main_loop.h (right):

http://codereview.chromium.org/10829073/diff/13008/content/browser/browser_ma...
content/browser/browser_main_loop.h:47: #if defined(OS_LINUX)
On 2012/07/31 15:46:05, John Abd-El-Malek wrote:
> nit: can you move this after the class/struct section, so that the ifdef'd
parts
> are at the end (i.e. just like how we put includes inside ifdefs after the
rest
> of the other includes). also while you're at it, move the struct below after
the
> class WebKitThread

Done.

http://codereview.chromium.org/10829073/diff/13008/content/browser/device_mon...
File content/browser/device_monitor_linux.cc (right):

http://codereview.chromium.org/10829073/diff/13008/content/browser/device_mon...
content/browser/device_monitor_linux.cc:39: : io_loop_(NULL) {
On 2012/07/31 15:46:05, John Abd-El-Malek wrote:
> we don't store MessageLoop pointers as a rule, since they're not safe (this is
> why we have MessageLoopProxy, and TaskRunner). in this file though, it doesn't
> appear you actually need to store it. the check in the destructor can be
> replaced with dcheck (!BT::IsMLValid(io))

Done.

[jam, 2012-07-31 18:21:36]

lgtm