Add SystemMonitor::GetMediaDevices()

Add SystemMonitor::GetAttachedMediaDevices().

BUG=none
TEST=included
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=138148

[vandebo (ex-Chrome), 2012-05-17 21:17:08]

https://chromiumcodereview.appspot.com/10332190/diff/5006/chrome/browser/chro...
File chrome/browser/chrome_browser_main_linux.h (left):

https://chromiumcodereview.appspot.com/10332190/diff/5006/chrome/browser/chro...
chrome/browser/chrome_browser_main_linux.h:29:
scoped_refptr<chrome::MediaDeviceNotificationsLinux>
Why move the notifiers into the consumer class?  They complete the
implementation of functionality defined in base and only need to be constructed
once, not once per consumer.  It seems like MediaDeviceManager should just be a
consumer of the base API.

https://chromiumcodereview.appspot.com/10332190/diff/5006/chrome/browser/medi...
File chrome/browser/media_gallery/media_device_manager.cc (right):

https://chromiumcodereview.appspot.com/10332190/diff/5006/chrome/browser/medi...
chrome/browser/media_gallery/media_device_manager.cc:46: #if defined(OS_WIN)
Can we have a single Init method that is defined differently on different
platforms?

https://chromiumcodereview.appspot.com/10332190/diff/5006/chrome/browser/medi...
chrome/browser/media_gallery/media_device_manager.cc:54:
media_device_notifications_window_;
If we keep this, lets use the same name across platforms:
media_device_notifications_

https://chromiumcodereview.appspot.com/10332190/diff/5006/chrome/browser/medi...
chrome/browser/media_gallery/media_device_manager.cc:127:
MediaDeviceManager::MediaDeviceManager() {
Use initialization list?

https://chromiumcodereview.appspot.com/10332190/diff/5006/chrome/browser/medi...
File chrome/browser/media_gallery/media_device_manager.h (right):

https://chromiumcodereview.appspot.com/10332190/diff/5006/chrome/browser/medi...
chrome/browser/media_gallery/media_device_manager.h:31: void
GetDevices(std::vector<MediaDeviceInfo>* results);
I was going to ask why this had an separate implementation, but then... why not
just add a GetDevices method to base/SystemMonitor?  It's already maintaining
that list.

[Lei Zhang, 2012-05-17 23:41:58]

Ok, let's just do it in SystemMonitor instead.

[vandebo (ex-Chrome), 2012-05-17 23:49:16]

https://chromiumcodereview.appspot.com/10332190/diff/16002/base/system_monito...
File base/system_monitor/system_monitor.h (right):

https://chromiumcodereview.appspot.com/10332190/diff/16002/base/system_monito...
base/system_monitor/system_monitor.h:54: typedef std::pair<std::string,
FilePath> MediaDeviceInfo;
Seems that this should now include id, so that we detaches come through, it's
easier to associate things.

https://chromiumcodereview.appspot.com/10332190/diff/16002/base/system_monito...
base/system_monitor/system_monitor.h:154: void
GetMediaDevices(std::vector<MediaDeviceInfo>* results);
GetAttachedMediaDevices ?

https://chromiumcodereview.appspot.com/10332190/diff/16002/base/system_monito...
base/system_monitor/system_monitor.h:157: typedef
std::map<base::SystemMonitor::DeviceIdType,
Is this typedef worthwhile?  It's only used three times.

[Lei Zhang, 2012-05-18 00:07:27]

https://chromiumcodereview.appspot.com/10332190/diff/16002/base/system_monito...
File base/system_monitor/system_monitor.h (right):

https://chromiumcodereview.appspot.com/10332190/diff/16002/base/system_monito...
base/system_monitor/system_monitor.h:54: typedef std::pair<std::string,
FilePath> MediaDeviceInfo;
On 2012/05/17 23:49:17, vandebo wrote:
> Seems that this should now include id, so that we detaches come through, it's
> easier to associate things.

Done.

I had it earlier, but I never used the id so I ended up removing it. Since
SystemMonitor is more general, some other caller may have a use for it.

https://chromiumcodereview.appspot.com/10332190/diff/16002/base/system_monito...
base/system_monitor/system_monitor.h:154: void
GetMediaDevices(std::vector<MediaDeviceInfo>* results);
On 2012/05/17 23:49:17, vandebo wrote:
> GetAttachedMediaDevices ?

Done.

https://chromiumcodereview.appspot.com/10332190/diff/16002/base/system_monito...
base/system_monitor/system_monitor.h:157: typedef
std::map<base::SystemMonitor::DeviceIdType,
On 2012/05/17 23:49:17, vandebo wrote:
> Is this typedef worthwhile?  It's only used three times.

Yes, because I'd prefer not to write
"std::map<base::SystemMonitor::DeviceIdType, MediaDeviceInfo>::iterator" more
than once.

[vandebo (ex-Chrome), 2012-05-18 00:09:29]

LGTM

[Lei Zhang, 2012-05-18 00:14:38]

willchan: base OWNERS review please.

[willchan no longer on Chromium, 2012-05-21 05:32:36]

I think we're almost to the level of code duplication in the test setup that we
may want to consider moving the MessageLoop and OSX specific initialization code
into a test fixture. I defer to you here, since it's not clear to me if we've
crossed the line yet.

https://chromiumcodereview.appspot.com/10332190/diff/18002/base/system_monito...
File base/system_monitor/system_monitor.h (right):

https://chromiumcodereview.appspot.com/10332190/diff/18002/base/system_monito...
base/system_monitor/system_monitor.h:153: // Appends info for attached media
devices to |results|.
Is there a reason you don't just return std::vector<MediaDeviceInfo>? Are we
worried about perf cost here? I'd prefer returning std::vector since it has a
clearer API. When you pass in an output param, it's unspecified how to treat any
existing data in that vector, so we have to rely on comments. Therefore, I'd
prefer not to use an output param unless it's necessary.

https://chromiumcodereview.appspot.com/10332190/diff/18002/base/system_monito...
File base/system_monitor/system_monitor_unittest.cc (right):

https://chromiumcodereview.appspot.com/10332190/diff/18002/base/system_monito...
base/system_monitor/system_monitor_unittest.cc:8: #include
"base/system_monitor/system_monitor.h"
While you're here, please move this first as per Google style guide.

[Lei Zhang, 2012-05-21 19:42:06]

willchan: Please see patch set 8.

I'd be happy to create a test fixture in a (near) future cleanup CL.

https://chromiumcodereview.appspot.com/10332190/diff/18002/base/system_monito...
File base/system_monitor/system_monitor.h (right):

https://chromiumcodereview.appspot.com/10332190/diff/18002/base/system_monito...
base/system_monitor/system_monitor.h:153: // Appends info for attached media
devices to |results|.
On 2012/05/21 05:32:36, willchan wrote:
> Is there a reason you don't just return std::vector<MediaDeviceInfo>? Are we
> worried about perf cost here? I'd prefer returning std::vector since it has a
> clearer API. When you pass in an output param, it's unspecified how to treat
any
> existing data in that vector, so we have to rely on comments. Therefore, I'd
> prefer not to use an output param unless it's necessary.

Done.

A lot of our code in our code base does it that way, e.g. SplitString(). Whereas
I have not seen as much code return a vector. I don't have a strong pref either
way.

https://chromiumcodereview.appspot.com/10332190/diff/18002/base/system_monito...
File base/system_monitor/system_monitor_unittest.cc (right):

https://chromiumcodereview.appspot.com/10332190/diff/18002/base/system_monito...
base/system_monitor/system_monitor_unittest.cc:8: #include
"base/system_monitor/system_monitor.h"
On 2012/05/21 05:32:36, willchan wrote:
> While you're here, please move this first as per Google style guide.

Done.

[willchan no longer on Chromium, 2012-05-21 20:24:34]

lgtm

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

http://codereview.chromium.org/10332190/diff/18002/base/system_monitor/system...
base/system_monitor/system_monitor.h:153: // Appends info for attached media
devices to |results|.
On 2012/05/21 19:42:06, Lei Zhang wrote:
> On 2012/05/21 05:32:36, willchan wrote:
> > Is there a reason you don't just return std::vector<MediaDeviceInfo>? Are we
> > worried about perf cost here? I'd prefer returning std::vector since it has
a
> > clearer API. When you pass in an output param, it's unspecified how to treat
> any
> > existing data in that vector, so we have to rely on comments. Therefore, I'd
> > prefer not to use an output param unless it's necessary.
> 
> Done.
> 
> A lot of our code in our code base does it that way, e.g. SplitString().
Whereas
> I have not seen as much code return a vector. I don't have a strong pref
either
> way.

You're right that our existing code generally uses output params. I don't agree
with that :) We've got a lot of suboptimal practices in our code base and I'm
trying to crack down on them.

Output params are better in some cases (such as wanting to reuse containers for
copy-avoidance reasons), but the interface is better defined with simply
returning the container if possible.

[commit-bot: I haz the power, 2012-05-21 20:28:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/10332190/24001

[commit-bot: I haz the power, 2012-05-21 21:11:21]

Try job failure for 10332190-24001 (retry) on win for step "update" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

Step "update" is always a major failure.
Look at the try server FAQ for more details.