SystemMonitor: Pull device type into the device id

SystemMonitor: Pull device type into the device id

Add a utility class to create an split device ids as well as to get some information about devices.

BUG=NONE


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

[kmadhusu, 2012-08-17 18:53:34]

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/intents...
File chrome/browser/intents/device_attached_intent_source.cc (right):

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/intents...
chrome/browser/intents/device_attached_intent_source.cc:111:
chrome::MediaStorageUtil::Type type;
nit: using chrome::MediaStorageUtil;

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/intents...
chrome/browser/intents/device_attached_intent_source.cc:112:
chrome::MediaStorageUtil::CrackDeviceId(id, &type, NULL);
How about adding a DCHECK(type == MediaStorageUtil::USB_MASS_STORAGE); here

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
File
chrome/browser/media_gallery/media_device_notifications_chromeos_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_device_notifications_chromeos_unittest.cc:158:
const std::string kDeviceId0 = "FFFF-FFFF";
nit: Fix the device id formats. It is no longer "FFFF-FFFF".

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
File chrome/browser/media_gallery/media_device_notifications_linux.cc (right):

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_device_notifications_linux.cc:367:
MediaStorageUtil::USB_MASS_STORAGE_WITH_DCIM, unique_id);
Can we construct the DeviceId in get_device_info_func_?

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
File chrome/browser/media_gallery/media_device_notifications_window_win.cc
(right):

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_device_notifications_window_win.cc:103: //
TODO(vandebo) We need to look up a real device id as well as
You can assign this todo to me.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_device_notifications_window_win.cc:122:
std::string device_id = MediaStorageUtil::MakeDeviceId(
nit: Add a helper function to make device id.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
File
chrome/browser/media_gallery/media_device_notifications_window_win_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_device_notifications_window_win_unittest.cc:107:
EXPECT_CALL(observer_, OnMediaDeviceAttached(device_id name, drive))
(device_id, name, drive)

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
File chrome/browser/media_gallery/media_storage_util.cc (right):

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:5: //
chrome::MediaStorageUtil implementation.
nit: Comment is not required.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:13: #include
"base/lazy_instance.h"
lazy_instance is included both in .h and .cc file.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:24: const char
kUsbMassStorageWithDCIMPrefix[] = "dcim:";
nit: // Media device unique id prefix constants.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:29: void
EmptyPathIsFalseCallback(const base::Callback<void(bool)>& callback,
Document this helper function.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:30: FilePath path) {
nit: "const FilePath&"

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:36:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
nit: Add using content::BrowserThread;

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:44: FilePath
MakePathRelative(FilePath path) {
nit: GetRelativePath
nit: const FilePath&
nit: Document the function stating that the source path is the relative path
when the source path is not absolute, etc.,
nit: If possible, add an E.g.,

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:66: std::string
MediaStorageUtil::MakeDeviceId(Type type,
nit: const Type&
nit: DCHECK(!unique_id.empty());

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:79: return std::string();
Can we  return a bool to notify failure?

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:99: NOTREACHED();
Can we  return a bool to notify failure?

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:133:
base::SystemMonitor::Get()->GetAttachedMediaDevices();
Add a helper function in SystemMonitor to check whether the device is attached
or not.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:147:
base::Bind(&EmptyPathIsFalseCallback, callback));
nit: Can we rename "EmptyPathIsFalseCallback" ?

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:205:
base::SystemMonitor::Get()->GetAttachedMediaDevices();
nit: using base::SystemMonitor;

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
File chrome/browser/media_gallery/media_storage_util.h (right):

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.h:19: class MediaStorageUtil {
nit: Either move the comments from line 5 to here or Add "See comment at top of
file for a complete description"

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.h:25: USB_MASS_STORAGE_NO_DCIM,
You might want to add TODO's
(1) To handle mass storage with NO DCIM directory 
(2) To identify whether the mass storage is USB or not.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.h:32: // Makes a device id given
properties of the device. A prefix dependant on
nit: dependent

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.h:32: // Makes a device id given
properties of the device. A prefix dependant on
nit: Returns device id ....

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.h:36: // Extracts the device
type from |device_id|.
nit: Extracts device |type| and |unique_id| given |device_id|.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.h:41: // (type is
USB_MASS_STORAGE_WITH_DCIM or USB_MTP).
A device of type USB_MASS_STORAGE_WITH_NO_DCIM can also be a media device right?

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.h:46: static bool
IsRemovableDevice(const std::string& device_id);
If IsMediaDevice() returns true of USB_MASS_STORAGE_WITH_DCIM,
USB_MASS_STORAGE_WITH_NO_DCIM and USB_MTP, we can remove this function.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.h:50: const
base::Callback<void(bool)>& callback);
nit: Can we create a typedef for the Query callbacks? (here and at other places)

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.h:52: // Given a path |input|,
get the device_id, relative path from the root of
nit: Can we rename |input| to |path|? Since it is a reference param, it is
implicit that it is a input param.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.h:56: const
base::Callback<void(std::string /*device id*/,
nit: /* ABC */ (Space after /*)

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.h:67: MediaStorageUtil();
nit: Document this function // Do not instantiate this class

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.h:69: static void
FindUSBDeviceById(const std::string unique_id,
nit: Document this function (To specify that this is a platform specific code).

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.h:70: const
base::Callback<void(FilePath)>& callback);
DISALLOW_COPY_AND_ASSIGN(MediaStorageUtil);

[groby-ooo-7-16, 2012-08-17 21:01:30]

drive-by for the intents part...

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/intents...
File chrome/browser/intents/device_attached_intent_source.cc (right):

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/intents...
chrome/browser/intents/device_attached_intent_source.cc:112:
chrome::MediaStorageUtil::CrackDeviceId(id, &type, NULL);
If you DCHECK, you should _not_ handle that via if/return.

That would be probably not the best choice, unless there's no chance we ever get
an MTP device attached?

On 2012/08/17 18:53:35, kmadhusu wrote:
> How about adding a DCHECK(type == MediaStorageUtil::USB_MASS_STORAGE); here

[vandebo (ex-Chrome), 2012-08-17 22:55:51]

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/intents...
File chrome/browser/intents/device_attached_intent_source.cc (right):

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/intents...
chrome/browser/intents/device_attached_intent_source.cc:111:
chrome::MediaStorageUtil::Type type;
On 2012/08/17 18:53:35, kmadhusu wrote:
> nit: using chrome::MediaStorageUtil;

We prefer to not use "using" unless it significantly improves code readability. 
I don't think it does in this case.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/intents...
chrome/browser/intents/device_attached_intent_source.cc:112:
chrome::MediaStorageUtil::CrackDeviceId(id, &type, NULL);
On 2012/08/17 18:53:35, kmadhusu wrote:
> How about adding a DCHECK(type == MediaStorageUtil::USB_MASS_STORAGE); here

Added DCHECK(MediaStorageUtil::IsRemovableDevice(id));

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/intents...
chrome/browser/intents/device_attached_intent_source.cc:112:
chrome::MediaStorageUtil::CrackDeviceId(id, &type, NULL);
On 2012/08/17 21:01:30, groby wrote:
> If you DCHECK, you should _not_ handle that via if/return.
> 
> That would be probably not the best choice, unless there's no chance we ever
get
> an MTP device attached?

We don't currently handle MTP, but will do so shortly, so this check is
temporary.

> On 2012/08/17 18:53:35, kmadhusu wrote:
> > How about adding a DCHECK(type == MediaStorageUtil::USB_MASS_STORAGE); here
>

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
File
chrome/browser/media_gallery/media_device_notifications_chromeos_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_device_notifications_chromeos_unittest.cc:158:
const std::string kDeviceId0 = "FFFF-FFFF";
On 2012/08/17 18:53:35, kmadhusu wrote:
> nit: Fix the device id formats. It is no longer "FFFF-FFFF". 

Done.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
File chrome/browser/media_gallery/media_device_notifications_linux.cc (right):

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_device_notifications_linux.cc:367:
MediaStorageUtil::USB_MASS_STORAGE_WITH_DCIM, unique_id);
On 2012/08/17 18:53:35, kmadhusu wrote:
> Can we construct the DeviceId in get_device_info_func_?

SGTM. Done.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
File chrome/browser/media_gallery/media_device_notifications_window_win.cc
(right):

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_device_notifications_window_win.cc:103: //
TODO(vandebo) We need to look up a real device id as well as
On 2012/08/17 18:53:35, kmadhusu wrote:
> You can assign this todo to me.

Done.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_device_notifications_window_win.cc:122:
std::string device_id = MediaStorageUtil::MakeDeviceId(
On 2012/08/17 18:53:35, kmadhusu wrote:
> nit: Add a helper function to make device id.

I'll leave that for you.  When we look up a real device id I think we'll
probably keep a map so that we only have to look it up once.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
File
chrome/browser/media_gallery/media_device_notifications_window_win_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_device_notifications_window_win_unittest.cc:107:
EXPECT_CALL(observer_, OnMediaDeviceAttached(device_id name, drive))
On 2012/08/17 18:53:35, kmadhusu wrote:
> (device_id, name, drive)

Done.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
File chrome/browser/media_gallery/media_storage_util.cc (right):

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:5: //
chrome::MediaStorageUtil implementation.
On 2012/08/17 18:53:35, kmadhusu wrote:
> nit: Comment is not required.

In Lei's readability review the reviewer suggested a one line comment at the top
of the file.  This mimics what's in media_file_system_registry.cc

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:13: #include
"base/lazy_instance.h"
On 2012/08/17 18:53:35, kmadhusu wrote:
> lazy_instance is included both in .h and .cc file.

And used in neither.  Fixed.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:24: const char
kUsbMassStorageWithDCIMPrefix[] = "dcim:";
On 2012/08/17 18:53:35, kmadhusu wrote:
> nit: // Media device unique id prefix constants.

Done.  (Added a more "why" comment than you suggested.)

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:29: void
EmptyPathIsFalseCallback(const base::Callback<void(bool)>& callback,
On 2012/08/17 18:53:35, kmadhusu wrote:
> Document this helper function.

I think the name and body are pretty self descriptive.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:30: FilePath path) {
On 2012/08/17 18:53:35, kmadhusu wrote:
> nit: "const FilePath&"

This can't be const because it has to match the FilePathCallback type.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:44: FilePath
MakePathRelative(FilePath path) {
On 2012/08/17 18:53:35, kmadhusu wrote:
> nit: GetRelativePath
> nit: const FilePath&
> nit: Document the function stating that the source path is the relative path
> when the source path is not absolute, etc.,
> nit: If possible, add an E.g.,

Done.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:66: std::string
MediaStorageUtil::MakeDeviceId(Type type,
On 2012/08/17 18:53:35, kmadhusu wrote:
> nit: const Type&

I don't think we usually use const ref's for enums.

> nit: DCHECK(!unique_id.empty());

Done.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:79: return std::string();
On 2012/08/17 18:53:35, kmadhusu wrote:
> Can we  return a bool to notify failure?

Failure shouldn't happen, hence the NOTREACHED.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:99: NOTREACHED();
On 2012/08/17 18:53:35, kmadhusu wrote:
> Can we  return a bool to notify failure?

Failure shouldn't happen, hence the NOTREACHED.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:133:
base::SystemMonitor::Get()->GetAttachedMediaDevices();
On 2012/08/17 18:53:35, kmadhusu wrote:
> Add a helper function in SystemMonitor to check whether the device is attached
> or not.

What advantage would that have?

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:147:
base::Bind(&EmptyPathIsFalseCallback, callback));
On 2012/08/17 18:53:35, kmadhusu wrote:
> nit: Can we rename "EmptyPathIsFalseCallback" ?

Do you have a suggestion?  I think it's a descriptive name.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
File chrome/browser/media_gallery/media_storage_util.h (right):

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.h:19: class MediaStorageUtil {
On 2012/08/17 18:53:35, kmadhusu wrote:
> nit: Either move the comments from line 5 to here or Add "See comment at top
of
> file for a complete description"

Seems unnecessary to me.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.h:25: USB_MASS_STORAGE_NO_DCIM,
On 2012/08/17 18:53:35, kmadhusu wrote:
> You might want to add TODO's
> (1) To handle mass storage with NO DCIM directory 
> (2) To identify whether the mass storage is USB or not.

Where would you like me to add those TODOs?

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.h:32: // Makes a device id given
properties of the device. A prefix dependant on
On 2012/08/17 18:53:35, kmadhusu wrote:
> nit: dependent

Done.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.h:32: // Makes a device id given
properties of the device. A prefix dependant on
On 2012/08/17 18:53:35, kmadhusu wrote:
> nit: Returns device id ....

Done.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.h:36: // Extracts the device
type from |device_id|.
On 2012/08/17 18:53:35, kmadhusu wrote:
> nit: Extracts device |type| and |unique_id| given |device_id|.

Done.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.h:41: // (type is
USB_MASS_STORAGE_WITH_DCIM or USB_MTP).
On 2012/08/17 18:53:35, kmadhusu wrote:
> A device of type USB_MASS_STORAGE_WITH_NO_DCIM can also be a media device
right?

No, it would be type USB_MASS_STORAGE_WITH_DCIM in that case.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.h:50: const
base::Callback<void(bool)>& callback);
On 2012/08/17 18:53:35, kmadhusu wrote:
> nit: Can we create a typedef for the Query callbacks? (here and at other
places)

Done.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.h:52: // Given a path |input|,
get the device_id, relative path from the root of
On 2012/08/17 18:53:35, kmadhusu wrote:
> nit: Can we rename |input| to |path|? Since it is a reference param, it is
> implicit that it is a input param.

Done.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.h:56: const
base::Callback<void(std::string /*device id*/,
On 2012/08/17 18:53:35, kmadhusu wrote:
> nit: /* ABC */ (Space after /*)

Actually there's no clear guidance on this in the style guide.  The only
occurrence of an inline comment uses the no-space style.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.h:67: MediaStorageUtil();
On 2012/08/17 18:53:35, kmadhusu wrote:
> nit: Document this function // Do not instantiate this class

Done.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.h:69: static void
FindUSBDeviceById(const std::string unique_id,
On 2012/08/17 18:53:35, kmadhusu wrote:
> nit: Document this function (To specify that this is a platform specific
code).

Done.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.h:70: const
base::Callback<void(FilePath)>& callback);
On 2012/08/17 18:53:35, kmadhusu wrote:
> DISALLOW_COPY_AND_ASSIGN(MediaStorageUtil);

Done.

[kmadhusu, 2012-08-20 17:13:49]

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/intents...
File chrome/browser/intents/device_attached_intent_source.cc (right):

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/intents...
chrome/browser/intents/device_attached_intent_source.cc:111:
chrome::MediaStorageUtil::Type type;
On 2012/08/17 22:55:51, vandebo wrote:
> On 2012/08/17 18:53:35, kmadhusu wrote:
> > nit: using chrome::MediaStorageUtil;
> 
> We prefer to not use "using" unless it significantly improves code
readability. 
> I don't think it does in this case.

Google C++ style guide
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...),
also encourages using declaration in cc file. In this file,
chrome::MediaStorageUtil is used at 6 places. Therefore, "using
chrome::MediaStorageUtil;" improves readability.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
File chrome/browser/media_gallery/media_storage_util.cc (right):

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:79: return std::string();
On 2012/08/17 22:55:51, vandebo wrote:
> On 2012/08/17 18:53:35, kmadhusu wrote:
> > Can we  return a bool to notify failure?
> 
> Failure shouldn't happen, hence the NOTREACHED.

(repeating our offline discussion): There is a chance that this function is
called with invalid type which may cause this function to return an empty
string. Therefore, it is better to return a bool value to notify the failure.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:99: NOTREACHED();
On 2012/08/17 22:55:51, vandebo wrote:
> On 2012/08/17 18:53:35, kmadhusu wrote:
> > Can we  return a bool to notify failure?
> 
> Failure shouldn't happen, hence the NOTREACHED.

same here.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:133:
base::SystemMonitor::Get()->GetAttachedMediaDevices();
On 2012/08/17 22:55:51, vandebo wrote:
> On 2012/08/17 18:53:35, kmadhusu wrote:
> > Add a helper function in SystemMonitor to check whether the device is
attached
> > or not.
> 
> What advantage would that have?

As we discussed, you can leave this as it is.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
File chrome/browser/media_gallery/media_storage_util.h (right):

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.h:25: USB_MASS_STORAGE_NO_DCIM,
On 2012/08/17 22:55:51, vandebo wrote:
> On 2012/08/17 18:53:35, kmadhusu wrote:
> > You might want to add TODO's
> > (1) To handle mass storage with NO DCIM directory 
> > (2) To identify whether the mass storage is USB or not.
> 
> Where would you like me to add those TODOs?

You can add these TODO's in IsMediaDevice function

https://chromiumcodereview.appspot.com/10829384/diff/12002/chrome/browser/med...
File
chrome/browser/media_gallery/media_device_notifications_chromeos_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10829384/diff/12002/chrome/browser/med...
chrome/browser/media_gallery/media_device_notifications_chromeos_unittest.cc:37:
std::string GetDCIMDeviceId(std::string unique_id) {
nit: const std::string&

https://chromiumcodereview.appspot.com/10829384/diff/12002/chrome/browser/med...
File chrome/browser/media_gallery/media_device_notifications_linux.cc (right):

https://chromiumcodereview.appspot.com/10829384/diff/12002/chrome/browser/med...
chrome/browser/media_gallery/media_device_notifications_linux.cc:114: if (name)
{
Are you planning to call this function with NULL output params? 

From my understanding, this function is always called with valid output params.
So, it is sufficient to validate the params before assigning some value to it
"DCHECK(name) and DCHECK(id)".

https://chromiumcodereview.appspot.com/10829384/diff/12002/chrome/browser/med...
File chrome/browser/media_gallery/media_device_notifications_linux.h (right):

https://chromiumcodereview.appspot.com/10829384/diff/12002/chrome/browser/med...
chrome/browser/media_gallery/media_device_notifications_linux.h:29: // returns
true and fills in |device_name| and |unique_id|.
Rename |id| => |unique_id| to match the comments.

https://chromiumcodereview.appspot.com/10829384/diff/12002/chrome/browser/med...
File chrome/browser/media_gallery/media_device_notifications_linux_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10829384/diff/12002/chrome/browser/med...
chrome/browser/media_gallery/media_device_notifications_linux_unittest.cc:52:
std::string GetDCIMDeviceId(std::string unique_id) {
"const std::string&"

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

https://chromiumcodereview.appspot.com/10829384/diff/12002/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:97: if (type !=
MediaStorageUtil::USB_MTP &&
How about adding a TODO to handle MTP devices?

https://chromiumcodereview.appspot.com/10829384/diff/12002/chrome/browser/med...
File chrome/browser/media_gallery/media_storage_util.cc (right):

https://chromiumcodereview.appspot.com/10829384/diff/12002/chrome/browser/med...
chrome/browser/media_gallery/media_storage_util.cc:44: // TODO(vandebo) use
FilePath::AppendRelativePath instead
nit: TODO(vandebo):

https://chromiumcodereview.appspot.com/10829384/diff/12002/chrome/browser/med...
chrome/browser/media_gallery/media_storage_util.cc:168: // TODO(vandebo) This
needs to be implemented per platform.  Below is no
TODO(vandebo):

https://chromiumcodereview.appspot.com/10829384/diff/12002/chrome/browser/med...
chrome/browser/media_gallery/media_storage_util.cc:226: void
MediaStorageUtil::FindUSBDeviceById(const std::string unique_id,
nit: const std::string&

https://chromiumcodereview.appspot.com/10829384/diff/12002/chrome/browser/med...
File chrome/browser/media_gallery/media_storage_util.h (right):

https://chromiumcodereview.appspot.com/10829384/diff/12002/chrome/browser/med...
chrome/browser/media_gallery/media_storage_util.h:33: typedef
base::Callback<void(FilePath)> FilePathCallback;
const FilePath&

https://chromiumcodereview.appspot.com/10829384/diff/12002/chrome/browser/med...
chrome/browser/media_gallery/media_storage_util.h:34: typedef
base::Callback<void(std::string /*device id*/,
const std::string&
const FilePath&

https://chromiumcodereview.appspot.com/10829384/diff/12002/chrome/browser/med...
chrome/browser/media_gallery/media_storage_util.h:79: }  // namespace chromeos
chromeos => chrome

[vandebo (ex-Chrome), 2012-08-20 18:35:14]

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/intents...
File chrome/browser/intents/device_attached_intent_source.cc (right):

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/intents...
chrome/browser/intents/device_attached_intent_source.cc:111:
chrome::MediaStorageUtil::Type type;
On 2012/08/20 17:13:49, kmadhusu wrote:
> On 2012/08/17 22:55:51, vandebo wrote:
> > On 2012/08/17 18:53:35, kmadhusu wrote:
> > > nit: using chrome::MediaStorageUtil;
> > 
> > We prefer to not use "using" unless it significantly improves code
> readability. 
> > I don't think it does in this case.
> 
> Google C++ style guide
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...),
> also encourages using declaration in cc file. In this file,
> chrome::MediaStorageUtil is used at 6 places. Therefore, "using
> chrome::MediaStorageUtil;" improves readability. 

Done.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
File chrome/browser/media_gallery/media_storage_util.cc (right):

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:36:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
On 2012/08/17 18:53:35, kmadhusu wrote:
> nit: Add using content::BrowserThread;

Done.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:79: return std::string();
On 2012/08/20 17:13:49, kmadhusu wrote:
> On 2012/08/17 22:55:51, vandebo wrote:
> > On 2012/08/17 18:53:35, kmadhusu wrote:
> > > Can we  return a bool to notify failure?
> > 
> > Failure shouldn't happen, hence the NOTREACHED.
> 
> (repeating our offline discussion): There is a chance that this function is
> called with invalid type which may cause this function to return an empty
> string. Therefore, it is better to return a bool value to notify the failure.

Our in person conversation was about corrupt preferences, i.e. CrackDeviceId
being able to fail.  MakeDeviceId still should never fail.  I added a comment in
the header file saying that if an invalid type is passed in the result is an
empty string.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:99: NOTREACHED();
On 2012/08/17 18:53:35, kmadhusu wrote:
> Can we  return a bool to notify failure?

Done.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:133:
base::SystemMonitor::Get()->GetAttachedMediaDevices();
On 2012/08/20 17:13:49, kmadhusu wrote:
> On 2012/08/17 22:55:51, vandebo wrote:
> > On 2012/08/17 18:53:35, kmadhusu wrote:
> > > Add a helper function in SystemMonitor to check whether the device is
> attached
> > > or not.
> > 
> > What advantage would that have?
> 
> As we discussed, you can leave this as it is.

As you suggested, I pulled the duplicated code into a helper function.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.cc:205:
base::SystemMonitor::Get()->GetAttachedMediaDevices();
On 2012/08/17 18:53:35, kmadhusu wrote:
> nit: using base::SystemMonitor;

Done.

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
File chrome/browser/media_gallery/media_storage_util.h (right):

https://chromiumcodereview.appspot.com/10829384/diff/1/chrome/browser/media_g...
chrome/browser/media_gallery/media_storage_util.h:25: USB_MASS_STORAGE_NO_DCIM,
On 2012/08/20 17:13:49, kmadhusu wrote:
> On 2012/08/17 22:55:51, vandebo wrote:
> > On 2012/08/17 18:53:35, kmadhusu wrote:
> > > You might want to add TODO's
> > > (1) To handle mass storage with NO DCIM directory 
> > > (2) To identify whether the mass storage is USB or not.
> > 
> > Where would you like me to add those TODOs?
> 
> You can add these TODO's in IsMediaDevice function

Actually there's already TODO's in the implementation of GetDeviceInfoFromPath.

https://chromiumcodereview.appspot.com/10829384/diff/12002/chrome/browser/med...
File
chrome/browser/media_gallery/media_device_notifications_chromeos_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10829384/diff/12002/chrome/browser/med...
chrome/browser/media_gallery/media_device_notifications_chromeos_unittest.cc:37:
std::string GetDCIMDeviceId(std::string unique_id) {
On 2012/08/20 17:13:50, kmadhusu wrote:
> nit: const std::string&

Done.

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

https://chromiumcodereview.appspot.com/10829384/diff/12002/chrome/browser/med...
chrome/browser/media_gallery/media_file_system_registry.cc:97: if (type !=
MediaStorageUtil::USB_MTP &&
On 2012/08/20 17:13:50, kmadhusu wrote:
> How about adding a TODO to handle MTP devices?

Done.

https://chromiumcodereview.appspot.com/10829384/diff/12002/chrome/browser/med...
File chrome/browser/media_gallery/media_storage_util.cc (right):

https://chromiumcodereview.appspot.com/10829384/diff/12002/chrome/browser/med...
chrome/browser/media_gallery/media_storage_util.cc:44: // TODO(vandebo) use
FilePath::AppendRelativePath instead
On 2012/08/20 17:13:50, kmadhusu wrote:
> nit: TODO(vandebo):

colon is optional

https://chromiumcodereview.appspot.com/10829384/diff/12002/chrome/browser/med...
chrome/browser/media_gallery/media_storage_util.cc:226: void
MediaStorageUtil::FindUSBDeviceById(const std::string unique_id,
On 2012/08/20 17:13:50, kmadhusu wrote:
> nit: const std::string&

Done.

https://chromiumcodereview.appspot.com/10829384/diff/12002/chrome/browser/med...
File chrome/browser/media_gallery/media_storage_util.h (right):

https://chromiumcodereview.appspot.com/10829384/diff/12002/chrome/browser/med...
chrome/browser/media_gallery/media_storage_util.h:33: typedef
base::Callback<void(FilePath)> FilePathCallback;
On 2012/08/20 17:13:50, kmadhusu wrote:
> const FilePath&

Both in this case the the one below, the purpose of the callback is so that work
can happen on a different thread.  This means that the caller (e.g. the stack
frame where the arguments live) will have gone out of scope by the time the
posted task is run.  Thus a const reference would be referencing an already
deleted object.  base/callback.h notes that you have to be careful of this
situation in the "PASSING PARAMETERS BY REFERENCE" section.

https://chromiumcodereview.appspot.com/10829384/diff/12002/chrome/browser/med...
chrome/browser/media_gallery/media_storage_util.h:79: }  // namespace chromeos
On 2012/08/20 17:13:50, kmadhusu wrote:
> chromeos => chrome

Done.

[vandebo (ex-Chrome), 2012-08-20 20:59:48]

+groby: Owners signoff for intents
+willchan: Owners signoff for base
+sky: Owners signoff for chrome gypi changes.

[kmadhusu, 2012-08-20 21:02:20]

lgtm

https://chromiumcodereview.appspot.com/10829384/diff/12006/chrome/browser/med...
File chrome/browser/media_gallery/media_device_notifications_linux.h (right):

https://chromiumcodereview.appspot.com/10829384/diff/12006/chrome/browser/med...
chrome/browser/media_gallery/media_device_notifications_linux.h:29: // returns
true and fills in |device_name| and |unique_id|.
nit: id => unique_id

[sky, 2012-08-20 21:09:25]

LGTM

[vandebo (ex-Chrome), 2012-08-20 21:24:47]

https://chromiumcodereview.appspot.com/10829384/diff/12006/chrome/browser/med...
File chrome/browser/media_gallery/media_device_notifications_linux.h (right):

https://chromiumcodereview.appspot.com/10829384/diff/12006/chrome/browser/med...
chrome/browser/media_gallery/media_device_notifications_linux.h:29: // returns
true and fills in |device_name| and |unique_id|.
On 2012/08/20 21:02:20, kmadhusu wrote:
> nit: id => unique_id

Done.

[willchan no longer on Chromium, 2012-08-21 00:15:27]

lgtm

[groby-ooo-7-16, 2012-08-21 00:27:16]

lgtm

[commit-bot: I haz the power, 2012-08-21 00:29:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vandebo@chromium.org/10829384/4027

[commit-bot: I haz the power, 2012-08-21 02:53:52]

Change committed as 152496