add device type as an argument in OnDevicesChanged.

add device type as an argument in OnDevicesChanged.
This allows DevicesChangedObserver to listen to desired events, instead of being woken up by change of uninterested devices.

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

[tommi (sloooow) - chröme, 2012-07-30 10:02:28]

looks good in general but I would like to just use the existing enum.  See
below.

http://codereview.chromium.org/10836004/diff/12002/base/system_monitor/system...
File base/system_monitor/system_monitor.h (right):

http://codereview.chromium.org/10836004/diff/12002/base/system_monitor/system...
base/system_monitor/system_monitor.h:55: enum DeviceType {
MediaDeviceType and DeviceType sound very similar.  Can't we add these values to
the existing MediaDeviceType enumeration?

I realize that MediaDeviceType maps to the MediaDeviceInfo class, but we could
use that as well for audio/video devices.  The only member variable that doesn't
fit is perhaps the 'location' member.

I can see that class growing in the future to support pluggable devices in
general so I'd rather not start forking things now and have two types for
essentially the same thing.

[tommi (sloooow) - chröme, 2012-07-30 12:54:54]

Actually, I've looked closer into the media gallery code and have now changed my
mind.

Just to summarize my current thinking:

The media gallery notification code should eventually be moved to
content/browser because it is written for base::SystemMonitor, which is not
specific to the media gallery.  When that is done, I think we need to revisit
the "MediaDevice" design a bit since there are some assumptions in there that do
not apply to all media devices (where media means more than just usb drives).

As is, we cannot rely on the MG (media gallery) provided notifications since if
'media gallery' isn't available (such as is the case for content_shell), then
that part of  SystemMonitor is simply not there.

So, in short, I now think you've done the right thing here and avoided mixing
the content code with the code that, although is in *base::*SystemMonitor is
specific to the media gallery.

LGTM!

[wjia(left Chromium), 2012-07-30 16:52:15]

add owners for stamp:
sky@ for content/browser
brettw@ for base/

[Lei Zhang, 2012-07-30 18:57:51]

On 2012/07/30 12:54:54, tommi wrote:
> Actually, I've looked closer into the media gallery code and have now changed
my
> mind.
> 
> Just to summarize my current thinking:
> 
> The media gallery notification code should eventually be moved to
> content/browser because it is written for base::SystemMonitor, which is not
> specific to the media gallery.  When that is done, I think we need to revisit
> the "MediaDevice" design a bit since there are some assumptions in there that
do
> not apply to all media devices (where media means more than just usb drives).
> 
> As is, we cannot rely on the MG (media gallery) provided notifications since
if
> 'media gallery' isn't available (such as is the case for content_shell), then
> that part of  SystemMonitor is simply not there.
> 
> So, in short, I now think you've done the right thing here and avoided mixing
> the content code with the code that, although is in *base::*SystemMonitor is
> specific to the media gallery.
> 
> LGTM!

tommi: We had the media gallery code in content/browser, but we moved it out
into chrome/browser since it is a chrome specific feature. We are also in the
process of modifying it to take more than just usb drives / mass storage
devices.

The "MediaDevice" name is a bit vague, but it refers to storage devices with
media on them like usb drives and ptp/mtp cameras. The A/V media device code for
webcams should not have anything to do with the media gallery devices.

[Lei Zhang, 2012-07-30 18:59:49]

+scottmg FYI

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

On 2012/07/30 18:57:51, Lei Zhang wrote:
> 
> tommi: We had the media gallery code in content/browser, but we moved it out
> into chrome/browser since it is a chrome specific feature. We are also in the
> process of modifying it to take more than just usb drives / mass storage
> devices.

Thanks for the info Lei. I'm curious, what else will you be adding support for?
The detection code for other types of devices that can be unplugged/plugged
is very similar or even the same, as is the case on Windows, which is why
I thought this would be a good fit initially.

> The "MediaDevice" name is a bit vague, but it refers to storage devices with
> media on them like usb drives and ptp/mtp cameras. The A/V media device code
for
> webcams should not have anything to do with the media gallery devices.

I was thinking about this more from a SystemMonitor perspective.  If we think of
only the feature of "detecting removal/arrival of devices", then that sounds
like
a generic feature that more clients than media gallery could use.  Media gallery
would be one client, MediaStream another.  I think there are about 3 places in
chrome
that have the basic same detection code right now so I think it makes sense to
think
about consolidating.

For now though, I think what Wei has is good and agree with you that we
shouldn't
mix this with media device.

[Lei Zhang, 2012-07-30 22:13:39]

On 2012/07/30 19:29:13, tommi wrote:
> On 2012/07/30 18:57:51, Lei Zhang wrote:
> > 
> > tommi: We had the media gallery code in content/browser, but we moved it out
> > into chrome/browser since it is a chrome specific feature. We are also in
the
> > process of modifying it to take more than just usb drives / mass storage
> > devices.
> 
> Thanks for the info Lei. I'm curious, what else will you be adding support
for?
> The detection code for other types of devices that can be unplugged/plugged
> is very similar or even the same, as is the case on Windows, which is why
> I thought this would be a good fit initially.
> 
> > The "MediaDevice" name is a bit vague, but it refers to storage devices with
> > media on them like usb drives and ptp/mtp cameras. The A/V media device code
> for
> > webcams should not have anything to do with the media gallery devices.
> 
> I was thinking about this more from a SystemMonitor perspective.  If we think
of
> only the feature of "detecting removal/arrival of devices", then that sounds
> like
> a generic feature that more clients than media gallery could use.  Media
gallery
> would be one client, MediaStream another.  I think there are about 3 places in
> chrome
> that have the basic same detection code right now so I think it makes sense to
> think
> about consolidating.
> 
> For now though, I think what Wei has is good and agree with you that we
> shouldn't
> mix this with media device.

Perhaps we can unify the DevicesChangedObserver interface to just
OnDevicesChanged() in the future, if OnDevicesChanged() can also pass along a
dictionary that contains the relevant, platform-specific info for the device
that changed. But for now, this is fine.

[jar (doing other things), 2012-07-30 23:18:12]

I was asked for a rubber stamp... but it appears that this is still under
discussion.

nit listed below in the mean time.

http://codereview.chromium.org/10836004/diff/12002/base/system_monitor/system...
File base/system_monitor/system_monitor.cc (right):

http://codereview.chromium.org/10836004/diff/12002/base/system_monitor/system...
base/system_monitor/system_monitor.cc:142:
&DevicesChangedObserver::OnDevicesChanged, device_type);
nit: 4 character indent

[Lei Zhang, 2012-07-30 23:24:37]

On 2012/07/30 23:18:12, jar wrote:
> I was asked for a rubber stamp... but it appears that this is still under
> discussion.
> 
> nit listed below in the mean time.
> 
>
http://codereview.chromium.org/10836004/diff/12002/base/system_monitor/system...
> File base/system_monitor/system_monitor.cc (right):
> 
>
http://codereview.chromium.org/10836004/diff/12002/base/system_monitor/system...
> base/system_monitor/system_monitor.cc:142:
> &DevicesChangedObserver::OnDevicesChanged, device_type);
> nit: 4 character indent

We are mostly discussing where we would like to see this code go in the future.

Patch set 3 LGTM with jar's nit fixed.

[wjia(left Chromium), 2012-07-30 23:32:36]

nit fixed.

http://codereview.chromium.org/10836004/diff/12002/base/system_monitor/system...
File base/system_monitor/system_monitor.cc (right):

http://codereview.chromium.org/10836004/diff/12002/base/system_monitor/system...
base/system_monitor/system_monitor.cc:142:
&DevicesChangedObserver::OnDevicesChanged, device_type);
On 2012/07/30 23:18:13, jar wrote:
> nit: 4 character indent

Done.

[jar (doing other things), 2012-07-30 23:34:59]

Rubber stamp LGTM for base.

[jam, 2012-07-31 01:15:40]

lgtm