Change base::SystemMonitor's media device functions to take a type and string16 instead of a FilePa…

Change base::SystemMonitor's media device functions to take a type and FilePath::StringType instead of a FilePath.

BUG=none
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=147758

[vandebo (ex-Chrome), 2012-07-19 00:13:30]

As discussed, System Monitor should track four things:

string16 name;
std::string unique_id;
enum DevType type;
string16 dev_location;

Lets have a nice comment describing what these all mean and do.

I won't get a chance to review more until late tomorrow.

[vandebo (ex-Chrome), 2012-07-19 19:05:20]

https://chromiumcodereview.appspot.com/10780023/diff/27002/base/system_monito...
File base/system_monitor/system_monitor.cc (right):

https://chromiumcodereview.appspot.com/10780023/diff/27002/base/system_monito...
base/system_monitor/system_monitor.cc:103: media_device_map_.insert(
Call the other ProcessMediaDeviceAttach with the converted data instead so that
we don't have to update both of them when changes are made.

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

https://chromiumcodereview.appspot.com/10780023/diff/27002/base/system_monito...
base/system_monitor/system_monitor.h:59: typedef unsigned int DeviceIdType;
With unique_id being a string, I think you can get rid of DevieIdType.

https://chromiumcodereview.appspot.com/10780023/diff/27002/base/system_monito...
base/system_monitor/system_monitor.h:61: typedef Tuple4<DeviceIdType,
std::string, MediaDeviceType, string16>
Why a Tuple4 and not just a struct?

https://chromiumcodereview.appspot.com/10780023/diff/27002/base/system_monito...
base/system_monitor/system_monitor.h:173: std::vector<MediaDeviceInfo>
GetAttachedMediaDevices() const;
This being down here implies that it's intended not for consumers of
SystemMonitor but for providers.  Should we move above all the Process* methods?

https://chromiumcodereview.appspot.com/10780023/diff/27002/chrome/browser/med...
File chrome/browser/media_gallery/media_file_system_registry.cc (right):

https://chromiumcodereview.appspot.com/10780023/diff/27002/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:67: #if
defined(OS_WIN)
ugg... Anyway to kill the ifdef here?

[Lei Zhang, 2012-07-19 21:17:10]

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

https://chromiumcodereview.appspot.com/10780023/diff/27002/base/system_monito...
base/system_monitor/system_monitor.h:61: typedef Tuple4<DeviceIdType,
std::string, MediaDeviceType, string16>
On 2012/07/19 19:05:20, vandebo wrote:
> Why a Tuple4 and not just a struct?

Because you were ok with a Tuple3 in the previous review? ;) A struct will be
more readable. I'll make the change on the next upload.

https://chromiumcodereview.appspot.com/10780023/diff/27002/chrome/browser/med...
File chrome/browser/media_gallery/media_file_system_registry.cc (right):

https://chromiumcodereview.appspot.com/10780023/diff/27002/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:67: #if
defined(OS_WIN)
On 2012/07/19 19:05:20, vandebo wrote:
> ugg... Anyway to kill the ifdef here?

If we make location FilePath::StringType instead of always string16. On POSIX,
we probably don't care and will just end up making lots of UTF8ToUTF16
conversions. Let's do that instead? WDYT?

[Lei Zhang, 2012-07-19 23:07:25]

Onwards to patch set 3...

https://chromiumcodereview.appspot.com/10780023/diff/27002/base/system_monito...
File base/system_monitor/system_monitor.cc (right):

https://chromiumcodereview.appspot.com/10780023/diff/27002/base/system_monito...
base/system_monitor/system_monitor.cc:103: media_device_map_.insert(
On 2012/07/19 19:05:20, vandebo wrote:
> Call the other ProcessMediaDeviceAttach with the converted data instead so
that
> we don't have to update both of them when changes are made.

I removed this since we switched to FilePath::StringType.

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

https://chromiumcodereview.appspot.com/10780023/diff/27002/base/system_monito...
base/system_monitor/system_monitor.h:59: typedef unsigned int DeviceIdType;
On 2012/07/19 19:05:20, vandebo wrote:
> With unique_id being a string, I think you can get rid of DevieIdType.

Done.

https://chromiumcodereview.appspot.com/10780023/diff/27002/base/system_monito...
base/system_monitor/system_monitor.h:173: std::vector<MediaDeviceInfo>
GetAttachedMediaDevices() const;
On 2012/07/19 19:05:20, vandebo wrote:
> This being down here implies that it's intended not for consumers of
> SystemMonitor but for providers.  Should we move above all the Process*
methods?

Done.

[vandebo (ex-Chrome), 2012-07-20 00:27:41]

LGTM

https://chromiumcodereview.appspot.com/10780023/diff/26005/base/system_monito...
File base/system_monitor/system_monitor.cc (right):

https://chromiumcodereview.appspot.com/10780023/diff/26005/base/system_monito...
base/system_monitor/system_monitor.cc:95:
media_device_map_.insert(std::make_pair(id, info));
As estade raised yesterday, we should probably lookup id first and not insert &
warn if it already exists.

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

https://chromiumcodereview.appspot.com/10780023/diff/26005/base/system_monito...
base/system_monitor/system_monitor.h:72: std::string uuid;
nit: uuid means a 16 byte number, use a generic name instead, i.e.
persistent_id, unique_id ?

https://chromiumcodereview.appspot.com/10780023/diff/26005/chrome/browser/med...
File chrome/browser/media_gallery/media_device_notifications_window_win.cc
(right):

https://chromiumcodereview.appspot.com/10780023/diff/26005/chrome/browser/med...
chrome/browser/media_gallery/media_device_notifications_window_win.cc:105:
CheckDeviceTypeOnFileThread, this, base::IntToString(i),
nit: Pass drive instead of IntToString(i) ?

[Lei Zhang, 2012-07-20 00:46:24]

https://chromiumcodereview.appspot.com/10780023/diff/26005/base/system_monito...
File base/system_monitor/system_monitor.cc (right):

https://chromiumcodereview.appspot.com/10780023/diff/26005/base/system_monito...
base/system_monitor/system_monitor.cc:95:
media_device_map_.insert(std::make_pair(id, info));
On 2012/07/20 00:27:41, vandebo wrote:
> As estade raised yesterday, we should probably lookup id first and not insert
&
> warn if it already exists.

Done.

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

https://chromiumcodereview.appspot.com/10780023/diff/26005/base/system_monito...
base/system_monitor/system_monitor.h:72: std::string uuid;
On 2012/07/20 00:27:41, vandebo wrote:
> nit: uuid means a 16 byte number, use a generic name instead, i.e.
> persistent_id, unique_id ?

Done.

https://chromiumcodereview.appspot.com/10780023/diff/26005/chrome/browser/med...
File chrome/browser/media_gallery/media_device_notifications_window_win.cc
(right):

https://chromiumcodereview.appspot.com/10780023/diff/26005/chrome/browser/med...
chrome/browser/media_gallery/media_device_notifications_window_win.cc:105:
CheckDeviceTypeOnFileThread, this, base::IntToString(i),
On 2012/07/20 00:27:41, vandebo wrote:
> nit: Pass drive instead of IntToString(i) ?

Meh. If we do then we need to reconstruct |drive| in the detach code below too.
We need to rewrite this soon anyway so the IDs are unique.

[Lei Zhang, 2012-07-20 03:36:27]

+willchan for base/ OWNERS

[willchan no longer on Chromium, 2012-07-20 16:02:21]

lgtm

[commit-bot: I haz the power, 2012-07-20 18:31:09]

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

[Evan Stade, 2012-07-20 18:39:00]

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

https://chromiumcodereview.appspot.com/10780023/diff/23058/base/system_monito...
base/system_monitor/system_monitor.h:72: std::string unique_id;
we also need a way to get the unique_id and location (mount point) for any
arbitrary directory, even on drives that are not media devices. Where would that
go?

[Evan Stade, 2012-07-20 18:40:08]

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

https://chromiumcodereview.appspot.com/10780023/diff/23058/base/system_monito...
base/system_monitor/system_monitor.h:72: std::string unique_id;
On 2012/07/20 18:39:00, Evan Stade wrote:
> we also need a way to get the unique_id and location (mount point) for any
> arbitrary directory, even on drives that are not media devices. Where would
that
> go?

and also, the mount point for any unique_id (assuming it's currently attached)

[vandebo (ex-Chrome), 2012-07-20 18:41:59]

On 2012/07/20 18:40:08, Evan Stade wrote:
>
https://chromiumcodereview.appspot.com/10780023/diff/23058/base/system_monito...
> File base/system_monitor/system_monitor.h (right):
> 
>
https://chromiumcodereview.appspot.com/10780023/diff/23058/base/system_monito...
> base/system_monitor/system_monitor.h:72: std::string unique_id;
> On 2012/07/20 18:39:00, Evan Stade wrote:
> > we also need a way to get the unique_id and location (mount point) for any
> > arbitrary directory, even on drives that are not media devices. Where would
> that
> > go?
> 
> and also, the mount point for any unique_id (assuming it's currently attached)

These are not SystemMonitor type methods.  Maybe they should go on
MediaGalleryRegistry

[commit-bot: I haz the power, 2012-07-20 20:13:59]

List of reviewers changed. estade@chromium.org did a drive-by without LGTM'ing!

[commit-bot: I haz the power, 2012-07-20 23:13:16]

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

[commit-bot: I haz the power, 2012-07-21 01:52:28]

Change committed as 147758