Extract and dispatch device uuid in media device attached notification message.

In order to manage device media gallery permissions, use a device id that is unique and persistent across device attachments. Therefore, extract device uuid from DiskInfo::InitializeFromResponse and use that identifier while dispatching media device attached notification event.

BUG=none
TEST=none

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

[Lei Zhang, 2012-07-26 02:17:31]

http://codereview.chromium.org/10830003/diff/8002/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_device_notifications_chromeos.cc
(right):

http://codereview.chromium.org/10830003/diff/8002/chrome/browser/media_galler...
chrome/browser/media_gallery/media_device_notifications_chromeos.cc:113: if
(iter != disks.end()) {
nit: would you mind using |it| instead of |iter| to be consistent with the rest
of the file?

http://codereview.chromium.org/10830003/diff/8002/chrome/browser/media_galler...
chrome/browser/media_gallery/media_device_notifications_chromeos.cc:113: if
(iter != disks.end()) {
If I understand the disk manager code correctly, I think you should be able to
CHECK_NE(it, disks.end());

http://codereview.chromium.org/10830003/diff/8002/chrome/browser/media_galler...
chrome/browser/media_gallery/media_device_notifications_chromeos.cc:119:
device_id_str =  base::IntToString(current_device_id_++);
You should get rid of |current_device_id_|. Trying to handle the failure by
getting a device id this way does not fall in line with the notion that the
device id should be unique and persistent across device attachments.

I would rather we just fail and have a UMA to keep track of how often this
occurs.

[kmadhusu, 2012-07-26 17:01:23]

thestig: Addressed review comments. PTAL.

Thanks.

http://codereview.chromium.org/10830003/diff/8002/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_device_notifications_chromeos.cc
(right):

http://codereview.chromium.org/10830003/diff/8002/chrome/browser/media_galler...
chrome/browser/media_gallery/media_device_notifications_chromeos.cc:113: if
(iter != disks.end()) {
On 2012/07/26 02:17:32, Lei Zhang wrote:
> nit: would you mind using |it| instead of |iter| to be consistent with the
rest
> of the file?

Done.

http://codereview.chromium.org/10830003/diff/8002/chrome/browser/media_galler...
chrome/browser/media_gallery/media_device_notifications_chromeos.cc:113: if
(iter != disks.end()) {
On 2012/07/26 02:17:32, Lei Zhang wrote:
> If I understand the disk manager code correctly, I think you should be able to
> CHECK_NE(it, disks.end());

I was not able to use CHECK_NE. Why do you think it should work?

http://codereview.chromium.org/10830003/diff/8002/chrome/browser/media_galler...
chrome/browser/media_gallery/media_device_notifications_chromeos.cc:119:
device_id_str =  base::IntToString(current_device_id_++);
On 2012/07/26 02:17:32, Lei Zhang wrote:
> You should get rid of |current_device_id_|. Trying to handle the failure by
> getting a device id this way does not fall in line with the notion that the
> device id should be unique and persistent across device attachments.
> 
> I would rather we just fail and have a UMA to keep track of how often this
> occurs.

Done.

[kmadhusu, 2012-07-26 17:02:01]

benchan@: Please review chromeos code.

Thanks.

[Ben Chan, 2012-07-26 17:11:18]

This is just my preference: try to avoid using "we" / "I" in commit message.

http://codereview.chromium.org/10830003/diff/11008/chrome/browser/media_galle...
File chrome/browser/media_gallery/media_device_notifications_chromeos.cc
(right):

http://codereview.chromium.org/10830003/diff/11008/chrome/browser/media_galle...
chrome/browser/media_gallery/media_device_notifications_chromeos.cc:105: // Get
the media device uuid if exists.
nit: perhaps turning 105 - 114 into method

[kmadhusu, 2012-07-26 17:23:05]

benchan@: Addressed review comments. PTAL. Thanks.

http://codereview.chromium.org/10830003/diff/11008/chrome/browser/media_galle...
File chrome/browser/media_gallery/media_device_notifications_chromeos.cc
(right):

http://codereview.chromium.org/10830003/diff/11008/chrome/browser/media_galle...
chrome/browser/media_gallery/media_device_notifications_chromeos.cc:105: // Get
the media device uuid if exists.
On 2012/07/26 17:11:18, Ben Chan wrote:
> nit: perhaps turning 105 - 114 into method

Done.

[Ben Chan, 2012-07-26 17:56:03]

lgtm

http://codereview.chromium.org/10830003/diff/4003/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_device_notifications_chromeos.cc
(right):

http://codereview.chromium.org/10830003/diff/4003/chrome/browser/media_galler...
chrome/browser/media_gallery/media_device_notifications_chromeos.cc:30:
device_id = disk.fs_uuid();
this may be simpler:

if (it != disks.end())
  device_id = it->second->fs_uuid();


On another note, having seen several places doing similar lookup, it feels like
DiskMountManager should provide a FindDiskBySourcePath method instead of
exposing disks(), which violates the law of diameters. But that can be cleaned
up in a subsequent CL.

[Lei Zhang, 2012-07-26 18:34:30]

http://codereview.chromium.org/10830003/diff/4003/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_device_notifications_chromeos.cc
(right):

http://codereview.chromium.org/10830003/diff/4003/chrome/browser/media_galler...
chrome/browser/media_gallery/media_device_notifications_chromeos.cc:27:
std::string device_id;
you can further simplify lines 27-32 to:

  return it == disks.end() ? std::string() : it->second->fs_uuid();

http://codereview.chromium.org/10830003/diff/4003/chrome/browser/media_galler...
chrome/browser/media_gallery/media_device_notifications_chromeos.cc:126: static
base::Histogram* media_device_uuid_available(NULL);
I see some Histogram code that uses a static Histogram pointer, but more code
that do not.

+jar: What's preferred? This code only gets called when someone connects a USB
drive to their CrOS device. Which is a relatively rare event.

[kmadhusu, 2012-07-26 18:44:49]

benchan@, thestig@: Addressed review comments.

http://codereview.chromium.org/10830003/diff/4003/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_device_notifications_chromeos.cc
(right):

http://codereview.chromium.org/10830003/diff/4003/chrome/browser/media_galler...
chrome/browser/media_gallery/media_device_notifications_chromeos.cc:27:
std::string device_id;
On 2012/07/26 18:34:31, Lei Zhang wrote:
> you can further simplify lines 27-32 to:
> 
>   return it == disks.end() ? std::string() : it->second->fs_uuid();

Done.

http://codereview.chromium.org/10830003/diff/4003/chrome/browser/media_galler...
chrome/browser/media_gallery/media_device_notifications_chromeos.cc:30:
device_id = disk.fs_uuid();
On 2012/07/26 17:56:03, Ben Chan wrote:
> this may be simpler:
> 
> if (it != disks.end())
>   device_id = it->second->fs_uuid();
> 

done.
> 
> On another note, having seen several places doing similar lookup, it feels
like
> DiskMountManager should provide a FindDiskBySourcePath method instead of
> exposing disks(), which violates the law of diameters. But that can be cleaned
> up in a subsequent CL.

I will submit a different CL to address this.

[Lei Zhang, 2012-07-26 18:47:12]

lgtm

[jar (doing other things), 2012-07-27 17:48:48]

(Invited/requested) drive by review of histogram usage below.

I think the histogram will probably contain reasonable and interesting values. 
It is good that you are counting both cases, so you can normalize your answers.

I would strongly recommend you use the macro instead of the personal code.  It
is more maintainable, and definitively (I hope) more correct.

http://codereview.chromium.org/10830003/diff/4005/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_device_notifications_chromeos.cc
(right):

http://codereview.chromium.org/10830003/diff/4005/chrome/browser/media_galler...
chrome/browser/media_gallery/media_device_notifications_chromeos.cc:121: static
base::Histogram* media_device_uuid_available(NULL);
Why didn't you use the UMA_HISTOGRAM_BOOLEAN() macro?

Your code looks reasonable, but can break if this is called (in a race) on
multiple threads.

[kmadhusu, 2012-07-27 18:30:31]

jar@: Addressed review comments. Thanks.

http://codereview.chromium.org/10830003/diff/4005/chrome/browser/media_galler...
File chrome/browser/media_gallery/media_device_notifications_chromeos.cc
(right):

http://codereview.chromium.org/10830003/diff/4005/chrome/browser/media_galler...
chrome/browser/media_gallery/media_device_notifications_chromeos.cc:121: static
base::Histogram* media_device_uuid_available(NULL);
On 2012/07/27 17:48:48, jar wrote:
> Why didn't you use the UMA_HISTOGRAM_BOOLEAN() macro?

I saw a couple of sample code doing this. So I thought it should be okay. 
> 
> Your code looks reasonable, but can break if this is called (in a race) on
> multiple threads.

Fixed.

[jar (doing other things), 2012-07-27 22:07:43]

histogram call and usage LGTM (Though I did not read the surrounding code).

[kmadhusu, 2012-07-28 23:22:52]

benchan@, thestig@: Modified mock_disk_mount_manager fix
MediaDeviceNotificationsTest. PTAL. 

Thanks.

[Ben Chan, 2012-07-30 22:03:59]

http://codereview.chromium.org/10830003/diff/2006/chrome/browser/chromeos/dis...
File chrome/browser/chromeos/disks/mock_disk_mount_manager.cc (right):

http://codereview.chromium.org/10830003/diff/2006/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/mock_disk_mount_manager.cc:161: Disk* disk =  new
DiskMountManager::Disk(std::string(mount_info.source_path),
nit: remove the extra space after =

http://codereview.chromium.org/10830003/diff/2006/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/mock_disk_mount_manager.cc:163: std::string(),
nit: comment what these empty strings are

http://codereview.chromium.org/10830003/diff/2006/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/mock_disk_mount_manager.cc:167:
std::string(device_id),
std::string(device_id) -> device_id

[Lei Zhang, 2012-07-30 22:11:32]

http://codereview.chromium.org/10830003/diff/2006/chrome/browser/chromeos/dis...
File chrome/browser/chromeos/disks/mock_disk_mount_manager.cc (right):

http://codereview.chromium.org/10830003/diff/2006/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/mock_disk_mount_manager.cc:98:
std::string(kTestUuid),
either this is correct and the change on line 121 is wrong, or vice versa.

Here we have: drive_label, uuid, system_path_prefix
below we have: uuid, drive_label, system_path_prefix

http://codereview.chromium.org/10830003/diff/2006/chrome/browser/chromeos/dis...
File chrome/browser/chromeos/disks/mock_disk_mount_manager.h (right):

http://codereview.chromium.org/10830003/diff/2006/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/mock_disk_mount_manager.h:51: void
CreateDiskEntryForMountDevice(
Briefly explain what this is used for.

[kmadhusu, 2012-07-30 22:51:14]

Addressed review comments. PTAL. Thanks.

http://codereview.chromium.org/10830003/diff/2006/chrome/browser/chromeos/dis...
File chrome/browser/chromeos/disks/mock_disk_mount_manager.cc (right):

http://codereview.chromium.org/10830003/diff/2006/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/mock_disk_mount_manager.cc:98:
std::string(kTestUuid),
On 2012/07/30 22:11:32, Lei Zhang wrote:
> either this is correct and the change on line 121 is wrong, or vice versa.
> 
> Here we have: drive_label, uuid, system_path_prefix
> below we have: uuid, drive_label, system_path_prefix

oops. Good catch. Line 98 is correct. Fixed line 121.

http://codereview.chromium.org/10830003/diff/2006/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/mock_disk_mount_manager.cc:161: Disk* disk =  new
DiskMountManager::Disk(std::string(mount_info.source_path),
On 2012/07/30 22:03:59, Ben Chan wrote:
> nit: remove the extra space after =

Done.

http://codereview.chromium.org/10830003/diff/2006/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/mock_disk_mount_manager.cc:163: std::string(),
On 2012/07/30 22:03:59, Ben Chan wrote:
> nit: comment what these empty strings are

Done.

http://codereview.chromium.org/10830003/diff/2006/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/mock_disk_mount_manager.cc:167:
std::string(device_id),
On 2012/07/30 22:03:59, Ben Chan wrote:
> std::string(device_id) -> device_id

Done.

http://codereview.chromium.org/10830003/diff/2006/chrome/browser/chromeos/dis...
File chrome/browser/chromeos/disks/mock_disk_mount_manager.h (right):

http://codereview.chromium.org/10830003/diff/2006/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/mock_disk_mount_manager.h:51: void
CreateDiskEntryForMountDevice(
On 2012/07/30 22:11:32, Lei Zhang wrote:
> Briefly explain what this is used for.

Done.

[kmadhusu, 2012-07-30 22:51:49]

tbarzic: Please review chrome side disk manager code.

Thanks.

[Lei Zhang, 2012-07-30 23:00:49]

lgtm

http://codereview.chromium.org/10830003/diff/15014/chrome/browser/chromeos/di...
File chrome/browser/chromeos/disks/mock_disk_mount_manager.h (right):

http://codereview.chromium.org/10830003/diff/15014/chrome/browser/chromeos/di...
chrome/browser/chromeos/disks/mock_disk_mount_manager.h:52: // primarily for
MediaDeviceNotificationsTest.
You may want to mention this is needed so MediaDeviceNotifications can look up
the disk's device id.

[kmadhusu, 2012-07-30 23:08:29]

http://codereview.chromium.org/10830003/diff/15014/chrome/browser/chromeos/di...
File chrome/browser/chromeos/disks/mock_disk_mount_manager.h (right):

http://codereview.chromium.org/10830003/diff/15014/chrome/browser/chromeos/di...
chrome/browser/chromeos/disks/mock_disk_mount_manager.h:52: // primarily for
MediaDeviceNotificationsTest.
On 2012/07/30 23:00:49, Lei Zhang wrote:
> You may want to mention this is needed so MediaDeviceNotifications can look up
> the disk's device id.

I thought "This function is primarily for MediaDeviceNotificationsTest" says it
all. I don't want to specify the implementation details in the comments.

[tbarzic, 2012-07-30 23:11:16]

lgtm

[Ben Chan, 2012-07-30 23:19:31]

On 2012/07/30 23:11:16, tbarzic wrote:
> lgtm

lgtm

[commit-bot: I haz the power, 2012-07-30 23:21:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/10830003/15014

[commit-bot: I haz the power, 2012-07-31 01:00:40]

Change committed as 149099