Combine boolean MediaDeviceNotifications histograms to an enumerated histogram.

Combine boolean MediaDeviceNotifications histograms to an enumerated histogram.

For better correlation, combined
MediaDeviceNotifications.{DeviceUUIDAvailable, DeviceNameAvailable,
MTPDeviceUUIDAvailable, MTPDeviceNameAvailable} histograms to
MediaDeviceNotifications.DeviceInfo histogram.

BUG=none
TEST=none


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

[kmadhusu, 2012-09-16 22:07:06]

One of you please review. 

Thanks.

[vandebo (ex-Chrome), 2012-09-16 22:27:46]

https://codereview.chromium.org/10933114/diff/1/chrome/browser/system_monitor...
File chrome/browser/system_monitor/removable_device_notifications_chromeos.cc
(right):

https://codereview.chromium.org/10933114/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/removable_device_notifications_chromeos.cc:155: //
Keep track of device uuid, to see how often we receive empty uuid values.
update comment.

https://codereview.chromium.org/10933114/diff/1/chrome/browser/system_monitor...
File chrome/browser/system_monitor/removable_device_notifications_histogram.cc
(right):

https://codereview.chromium.org/10933114/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/removable_device_notifications_histogram.cc:17:
MASS_STORAGE_DEVICE_NAME_AVAILABLE_AND_UUID_NOT_AVAILABLE,
could call this FOO_UUID_MISSING

https://codereview.chromium.org/10933114/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/removable_device_notifications_histogram.cc:20:
MTP_STORAGE_DEVICE_NAME_AND_UUID_AVAILABLE,
the removable device notifiers only handle mass storage.  Maybe put this in
MediaStorageUtil?

https://codereview.chromium.org/10933114/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/removable_device_notifications_histogram.cc:29:
void RecordDeviceInfoHistogram(MediaStorageUtil::Type device_type,
bool mass_storage, so you don't have to figure out the complete type before
calling.

https://codereview.chromium.org/10933114/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/removable_device_notifications_histogram.cc:33:
event_number += device_name.empty() ? 2 : 0;
if (device_name.empty()) event_number += 2;

https://codereview.chromium.org/10933114/diff/1/chrome/browser/system_monitor...
File chrome/browser/system_monitor/removable_device_notifications_linux.cc
(right):

https://codereview.chromium.org/10933114/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/removable_device_notifications_linux.cc:487: if
(unique_id.empty() || name.empty())
No point in figuring out the precise type if we're going to return and ignore
it... See other comment about type

[kmadhusu, 2012-09-17 18:29:07]

vandebo@: Addressed review comments. PTAL.

Thanks.

https://codereview.chromium.org/10933114/diff/1/chrome/browser/system_monitor...
File chrome/browser/system_monitor/removable_device_notifications_chromeos.cc
(right):

https://codereview.chromium.org/10933114/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/removable_device_notifications_chromeos.cc:155: //
Keep track of device uuid, to see how often we receive empty uuid values.
On 2012/09/16 22:27:46, vandebo wrote:
> update comment.

Done.

https://codereview.chromium.org/10933114/diff/1/chrome/browser/system_monitor...
File chrome/browser/system_monitor/removable_device_notifications_histogram.cc
(right):

https://codereview.chromium.org/10933114/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/removable_device_notifications_histogram.cc:17:
MASS_STORAGE_DEVICE_NAME_AVAILABLE_AND_UUID_NOT_AVAILABLE,
On 2012/09/16 22:27:46, vandebo wrote:
> could call this FOO_UUID_MISSING

Done.

https://codereview.chromium.org/10933114/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/removable_device_notifications_histogram.cc:20:
MTP_STORAGE_DEVICE_NAME_AND_UUID_AVAILABLE,
On 2012/09/16 22:27:46, vandebo wrote:
> the removable device notifiers only handle mass storage.  Maybe put this in
> MediaStorageUtil?

Moved to MediaStorageUtil.

https://codereview.chromium.org/10933114/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/removable_device_notifications_histogram.cc:29:
void RecordDeviceInfoHistogram(MediaStorageUtil::Type device_type,
On 2012/09/16 22:27:46, vandebo wrote:
> bool mass_storage, so you don't have to figure out the complete type before
> calling.

Done.

https://codereview.chromium.org/10933114/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/removable_device_notifications_histogram.cc:33:
event_number += device_name.empty() ? 2 : 0;
On 2012/09/16 22:27:46, vandebo wrote:
> if (device_name.empty()) event_number += 2;

Done.

https://codereview.chromium.org/10933114/diff/1/chrome/browser/system_monitor...
File chrome/browser/system_monitor/removable_device_notifications_linux.cc
(right):

https://codereview.chromium.org/10933114/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/removable_device_notifications_linux.cc:487: if
(unique_id.empty() || name.empty())
On 2012/09/16 22:27:46, vandebo wrote:
> No point in figuring out the precise type if we're going to return and ignore
> it... See other comment about type

Fixed.

[vandebo (ex-Chrome), 2012-09-17 18:40:07]

LGTM with comments

https://codereview.chromium.org/10933114/diff/10001/chrome/browser/system_mon...
File chrome/browser/system_monitor/media_storage_util.cc (right):

https://codereview.chromium.org/10933114/diff/10001/chrome/browser/system_mon...
chrome/browser/system_monitor/media_storage_util.cc:296: int event_number =
mass_storage ? 0 : 4;
nit: Remove the trinary operator.

https://codereview.chromium.org/10933114/diff/10001/chrome/browser/system_mon...
File chrome/browser/system_monitor/removable_device_notifications_mac.mm
(right):

https://codereview.chromium.org/10933114/diff/10001/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_mac.mm:115:
MediaStorageUtil::RecordDeviceInfoHistogram(false, info.device_id(),
true, right?

[Lei Zhang, 2012-09-17 18:46:54]

https://codereview.chromium.org/10933114/diff/10001/chrome/browser/system_mon...
File chrome/browser/system_monitor/media_storage_util.cc (right):

https://codereview.chromium.org/10933114/diff/10001/chrome/browser/system_mon...
chrome/browser/system_monitor/media_storage_util.cc:296: int event_number =
mass_storage ? 0 : 4;
Why even use integers? Why not just use the enums?

[kmadhusu, 2012-09-17 19:32:49]

https://codereview.chromium.org/10933114/diff/10001/chrome/browser/system_mon...
File chrome/browser/system_monitor/media_storage_util.cc (right):

https://codereview.chromium.org/10933114/diff/10001/chrome/browser/system_mon...
chrome/browser/system_monitor/media_storage_util.cc:296: int event_number =
mass_storage ? 0 : 4;
On 2012/09/17 18:40:07, vandebo wrote:
> nit: Remove the trinary operator.

Done.

https://codereview.chromium.org/10933114/diff/10001/chrome/browser/system_mon...
chrome/browser/system_monitor/media_storage_util.cc:296: int event_number =
mass_storage ? 0 : 4;
On 2012/09/17 18:46:54, Lei Zhang wrote:
> Why even use integers? Why not just use the enums

I think this function is simple and easier to understand. So I preferred
integers vs using enum values.

https://codereview.chromium.org/10933114/diff/10001/chrome/browser/system_mon...
File chrome/browser/system_monitor/removable_device_notifications_mac.mm
(right):

https://codereview.chromium.org/10933114/diff/10001/chrome/browser/system_mon...
chrome/browser/system_monitor/removable_device_notifications_mac.mm:115:
MediaStorageUtil::RecordDeviceInfoHistogram(false, info.device_id(),
On 2012/09/17 18:40:07, vandebo wrote:
> true, right?

Fixed.

[Lei Zhang, 2012-09-17 19:48:27]

https://codereview.chromium.org/10933114/diff/10001/chrome/browser/system_mon...
File chrome/browser/system_monitor/media_storage_util.cc (right):

https://codereview.chromium.org/10933114/diff/10001/chrome/browser/system_mon...
chrome/browser/system_monitor/media_storage_util.cc:296: int event_number =
mass_storage ? 0 : 4;
On 2012/09/17 19:32:49, kmadhusu wrote:
> On 2012/09/17 18:46:54, Lei Zhang wrote:
> > Why even use integers? Why not just use the enums
> 
> I think this function is simple and easier to understand. So I preferred
> integers vs using enum values.

I'm not sure it is. Every time |event_number| changes, I have to go up to the
enum to look up what the new value means.

[Lei Zhang, 2012-09-17 20:46:01]

lgtm

https://codereview.chromium.org/10933114/diff/7002/chrome/browser/system_moni...
File chrome/browser/system_monitor/media_storage_util.cc (right):

https://codereview.chromium.org/10933114/diff/7002/chrome/browser/system_moni...
chrome/browser/system_monitor/media_storage_util.cc:296: int event_number = 0;
unsigned int? otherwise you'd also want to check for event < 0 on line 307.

https://codereview.chromium.org/10933114/diff/7002/chrome/browser/system_moni...
chrome/browser/system_monitor/media_storage_util.cc:296: int event_number = 0;
I think the numbers are a bit hard to read, but I can't think of anything
significantly better, so meh.

https://codereview.chromium.org/10933114/diff/7002/chrome/browser/system_moni...
File chrome/browser/system_monitor/media_storage_util.h (right):

https://codereview.chromium.org/10933114/diff/7002/chrome/browser/system_moni...
chrome/browser/system_monitor/media_storage_util.h:79: // |device_name|. Set
|mass_storage| to true, if the current device is a
nit: Just say |mass_storage| indicates whether the current device is a mass
storage device, as defined by IsMassStorageDevice().

[kmadhusu, 2012-09-17 21:52:20]

https://codereview.chromium.org/10933114/diff/7002/chrome/browser/system_moni...
File chrome/browser/system_monitor/media_storage_util.cc (right):

https://codereview.chromium.org/10933114/diff/7002/chrome/browser/system_moni...
chrome/browser/system_monitor/media_storage_util.cc:296: int event_number = 0;
On 2012/09/17 20:46:01, Lei Zhang wrote:
> unsigned int? otherwise you'd also want to check for event < 0 on line 307.

Done.

https://codereview.chromium.org/10933114/diff/7002/chrome/browser/system_moni...
File chrome/browser/system_monitor/media_storage_util.h (right):

https://codereview.chromium.org/10933114/diff/7002/chrome/browser/system_moni...
chrome/browser/system_monitor/media_storage_util.h:79: // |device_name|. Set
|mass_storage| to true, if the current device is a
On 2012/09/17 20:46:01, Lei Zhang wrote:
> nit: Just say |mass_storage| indicates whether the current device is a mass
> storage device, as defined by IsMassStorageDevice().

Done.

[commit-bot: I haz the power, 2012-09-17 21:53:15]

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

[commit-bot: I haz the power, 2012-09-17 21:55:15]

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

[commit-bot: I haz the power, 2012-09-17 22:04:09]

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

[commit-bot: I haz the power, 2012-09-18 01:08:57]

Change committed as 157266