[Chrome OS]Implement MediaStorageUtil::GetDeviceInfoForPath function to support media gallery permi…

Implement MediaStorageUtil::GetDeviceInfoForPath function to support media gallery permissions.

BUG=145849
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=157287

[vandebo (ex-Chrome), 2012-09-15 23:51:13]

I'll take a look

[Lei Zhang, 2012-09-16 00:17:05]

https://chromiumcodereview.appspot.com/10918259/diff/2001/chrome/browser/syst...
File chrome/browser/system_monitor/media_storage_util_chromeos.cc (right):

https://chromiumcodereview.appspot.com/10918259/diff/2001/chrome/browser/syst...
chrome/browser/system_monitor/media_storage_util_chromeos.cc:38: return;  // No
matching device found.
This makes me think GetDeviceInfoFromPath() should return a bool. Otherwise how
do you know the returned info is valid?

https://chromiumcodereview.appspot.com/10918259/diff/2001/chrome/browser/syst...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc
(right):

https://chromiumcodereview.appspot.com/10918259/diff/2001/chrome/browser/syst...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc:143:
DCHECK(!g_mtp_device_observer);
this is weird, why not just set |g_mtp_device_observer| so the test ctor's
behavior is consistent with the real one?

https://chromiumcodereview.appspot.com/10918259/diff/2001/chrome/browser/syst...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.h
(right):

https://chromiumcodereview.appspot.com/10918259/diff/2001/chrome/browser/syst...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.h:34:
static MediaTransferProtocolDeviceObserverCros* GetInstance();
nit: don't put this between the ctor and dtor.

https://chromiumcodereview.appspot.com/10918259/diff/2001/chrome/browser/syst...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.h:57:
struct StorageInfo {
hey, this looks familiar.

https://chromiumcodereview.appspot.com/10918259/diff/2001/chrome/browser/syst...
File chrome/browser/system_monitor/removable_device_notifications_chromeos.h
(right):

https://chromiumcodereview.appspot.com/10918259/diff/2001/chrome/browser/syst...
chrome/browser/system_monitor/removable_device_notifications_chromeos.h:56:
struct DeviceInfo {
Is this already defined elsewhere?

[vandebo (ex-Chrome), 2012-09-16 00:31:44]

http://codereview.chromium.org/10918259/diff/2001/chrome/browser/system_monit...
File chrome/browser/system_monitor/media_storage_util_chromeos.cc (right):

http://codereview.chromium.org/10918259/diff/2001/chrome/browser/system_monit...
chrome/browser/system_monitor/media_storage_util_chromeos.cc:38: return;  // No
matching device found.
The function can fail, so we should propagate that out.

http://codereview.chromium.org/10918259/diff/2001/chrome/browser/system_monit...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc
(right):

http://codereview.chromium.org/10918259/diff/2001/chrome/browser/system_monit...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc:176:
while (!ContainsKey(storage_map_, current.value()) &&
Why do this?  Isn't the path for a mtp device a fixed format?

http://codereview.chromium.org/10918259/diff/2001/chrome/browser/system_monit...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.h
(right):

http://codereview.chromium.org/10918259/diff/2001/chrome/browser/system_monit...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.h:58:
StorageInfo();
You can omit the default constructor when you have no other constructors.

[kmadhusu, 2012-09-16 22:35:42]

Addressed review comments. PTAL.

Thanks.

https://codereview.chromium.org/10918259/diff/2001/chrome/browser/system_moni...
File chrome/browser/system_monitor/media_storage_util_chromeos.cc (right):

https://codereview.chromium.org/10918259/diff/2001/chrome/browser/system_moni...
chrome/browser/system_monitor/media_storage_util_chromeos.cc:38: return;  // No
matching device found.
On 2012/09/16 00:31:44, vandebo wrote:
> The function can fail, so we should propagate that out.

Done.

https://codereview.chromium.org/10918259/diff/2001/chrome/browser/system_moni...
chrome/browser/system_monitor/media_storage_util_chromeos.cc:38: return;  // No
matching device found.
On 2012/09/16 00:17:05, Lei Zhang wrote:
> This makes me think GetDeviceInfoFromPath() should return a bool. Otherwise
how
> do you know the returned info is valid?

Modified the function signature to return bool.

https://codereview.chromium.org/10918259/diff/2001/chrome/browser/system_moni...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc
(right):

https://codereview.chromium.org/10918259/diff/2001/chrome/browser/system_moni...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc:143:
DCHECK(!g_mtp_device_observer);
On 2012/09/16 00:17:05, Lei Zhang wrote:
> this is weird, why not just set |g_mtp_device_observer| so the test ctor's
> behavior is consistent with the real one?

Done.

https://codereview.chromium.org/10918259/diff/2001/chrome/browser/system_moni...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc:176:
while (!ContainsKey(storage_map_, current.value()) &&
On 2012/09/16 00:31:44, vandebo wrote:
> Why do this?  Isn't the path for a mtp device a fixed format?

Fixed. The first component of the path is always the device location.

https://codereview.chromium.org/10918259/diff/2001/chrome/browser/system_moni...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.h
(right):

https://codereview.chromium.org/10918259/diff/2001/chrome/browser/system_moni...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.h:34:
static MediaTransferProtocolDeviceObserverCros* GetInstance();
On 2012/09/16 00:17:05, Lei Zhang wrote:
> nit: don't put this between the ctor and dtor.

Done.

https://codereview.chromium.org/10918259/diff/2001/chrome/browser/system_moni...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.h:57:
struct StorageInfo {
On 2012/09/16 00:17:05, Lei Zhang wrote:
> hey, this looks familiar.

:)

https://codereview.chromium.org/10918259/diff/2001/chrome/browser/system_moni...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.h:58:
StorageInfo();
On 2012/09/16 00:31:44, vandebo wrote:
> You can omit the default constructor when you have no other constructors.

Done.

https://codereview.chromium.org/10918259/diff/2001/chrome/browser/system_moni...
File chrome/browser/system_monitor/removable_device_notifications_chromeos.h
(right):

https://codereview.chromium.org/10918259/diff/2001/chrome/browser/system_moni...
chrome/browser/system_monitor/removable_device_notifications_chromeos.h:56:
struct DeviceInfo {
On 2012/09/16 00:17:05, Lei Zhang wrote:
> Is this already defined elsewhere?

DeviceInfo and StorageInfo structs has the same member fields as
RemovableStorageInfo. So, removed the new structs and created instances of
SystemMonitor::RemovableStorageInfo.

https://codereview.chromium.org/10918259/diff/13001/chrome/browser/system_mon...
File chrome/browser/system_monitor/media_storage_util_chromeos.cc (right):

https://codereview.chromium.org/10918259/diff/13001/chrome/browser/system_mon...
chrome/browser/system_monitor/media_storage_util_chromeos.cc:5: // Chrome OS
specific implementation of chrome::MediaStorageUtil.
Do not review this file. Just uploaded for reference. After landing,
https://codereview.chromium.org/10909259/, we will not require this file.

[vandebo (ex-Chrome), 2012-09-16 23:03:19]

http://codereview.chromium.org/10918259/diff/13001/chrome/browser/system_moni...
File chrome/browser/system_monitor/media_storage_util.cc (right):

http://codereview.chromium.org/10918259/diff/13001/chrome/browser/system_moni...
chrome/browser/system_monitor/media_storage_util.cc:189: #if !defined(OS_LINUX)
Remove this change or rebase on to the mentioned CL.

http://codereview.chromium.org/10918259/diff/13001/chrome/browser/system_moni...
File chrome/browser/system_monitor/media_storage_util_chromeos.cc (right):

http://codereview.chromium.org/10918259/diff/13001/chrome/browser/system_moni...
chrome/browser/system_monitor/media_storage_util_chromeos.cc:5: // Chrome OS
specific implementation of chrome::MediaStorageUtil.
On 2012/09/16 22:35:42, kmadhusu wrote:
> Do not review this file. Just uploaded for reference. After landing,
> https://codereview.chromium.org/10909259/, we will not require this file.

You can remove it now, since it's still in the previous patch sets.

http://codereview.chromium.org/10918259/diff/13001/chrome/browser/system_moni...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc
(right):

http://codereview.chromium.org/10918259/diff/13001/chrome/browser/system_moni...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc:140:

nit: extra line

http://codereview.chromium.org/10918259/diff/13001/chrome/browser/system_moni...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc:147:
if (g_mtp_device_observer) {
No if - this is set in the constructor.

http://codereview.chromium.org/10918259/diff/13001/chrome/browser/system_moni...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.h
(right):

http://codereview.chromium.org/10918259/diff/13001/chrome/browser/system_moni...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.h:55:
// Mapping of storage location and mtp storage info object.
Looks like you could continue using storage name as the key... though there's
not much difference.

http://codereview.chromium.org/10918259/diff/13001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

http://codereview.chromium.org/10918259/diff/13001/chrome/chrome_browser.gypi...
chrome/chrome_browser.gypi:1920:
'browser/system_monitor/media_storage_util_chromeos.cc',
Also not needed

[kmadhusu, 2012-09-17 21:47:55]

Addressed comments. PTAL.

Thanks.

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

https://codereview.chromium.org/10918259/diff/13001/chrome/browser/system_mon...
chrome/browser/system_monitor/media_storage_util.cc:189: #if !defined(OS_LINUX)
On 2012/09/16 23:03:19, vandebo wrote:
> Remove this change or rebase on to the mentioned CL.

Done.

https://codereview.chromium.org/10918259/diff/13001/chrome/browser/system_mon...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc
(right):

https://codereview.chromium.org/10918259/diff/13001/chrome/browser/system_mon...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc:140:

On 2012/09/16 23:03:19, vandebo wrote:
> nit: extra line

Done.

https://codereview.chromium.org/10918259/diff/13001/chrome/browser/system_mon...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc:147:
if (g_mtp_device_observer) {
On 2012/09/16 23:03:19, vandebo wrote:
> No if - this is set in the constructor.

Done.

https://codereview.chromium.org/10918259/diff/13001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/10918259/diff/13001/chrome/chrome_browser.gyp...
chrome/chrome_browser.gypi:1920:
'browser/system_monitor/media_storage_util_chromeos.cc',
On 2012/09/16 23:03:19, vandebo wrote:
> Also not needed

Done.

https://codereview.chromium.org/10918259/diff/17/chrome/browser/system_monito...
File chrome/browser/system_monitor/removable_device_notifications_chromeos.cc
(left):

https://codereview.chromium.org/10918259/diff/17/chrome/browser/system_monito...
chrome/browser/system_monitor/removable_device_notifications_chromeos.cc:179:
NOTREACHED();
CheckExistingMountPointsOnUIThread() added the mount point information in the
map before the device attached handler is called. Therefore, an entry for the
device already exists in the map. It is not a bug. So, just return.

https://codereview.chromium.org/10918259/diff/9012/chrome/browser/system_moni...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc
(right):

https://codereview.chromium.org/10918259/diff/9012/chrome/browser/system_moni...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc:89:
// Add the data store id to the device label.
The following code appends the data store id to the device label. E.g: "Sony
Corporation Sony Digital Still Camera:131073". Do you think, we need to add some
text such as data store "Id" right before the # (Sony Corporation Sony Digital
Still Camera:ID:131073)?

[vandebo (ex-Chrome), 2012-09-17 22:29:44]

LGTM

https://codereview.chromium.org/10918259/diff/9012/chrome/browser/system_moni...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc
(right):

https://codereview.chromium.org/10918259/diff/9012/chrome/browser/system_moni...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc:89:
// Add the data store id to the device label.
On 2012/09/17 21:47:56, kmadhusu wrote:
> The following code appends the data store id to the device label. E.g: "Sony
> Corporation Sony Digital Still Camera:131073". Do you think, we need to add
some
> text such as data store "Id" right before the # (Sony Corporation Sony Digital
> Still Camera:ID:131073)? 

The user won't understand the value anyway, so it's just an opaque part of the
name.  Ideally we wouldn't show empty storage's, but (id) is a reasonable
solution for now.

https://codereview.chromium.org/10918259/diff/9012/chrome/browser/system_moni...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc:92:
if (volume_id.empty()) {
nit: negate this:

if (!volume_id.empty()) {
  device_label += ... volume_id;
} else {
  data_stire_id =
  ...
}

https://codereview.chromium.org/10918259/diff/9012/chrome/browser/system_moni...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.h
(right):

https://codereview.chromium.org/10918259/diff/9012/chrome/browser/system_moni...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.h:17:
namespace chromeos {
Remove this stuff for now.  Since the MTP section in media_storage_util is CrOS
specific right just use the full name there.  We'll figure out how to do this
better once we need it for another mtp impl.

[Lei Zhang, 2012-09-17 22:52:52]

https://codereview.chromium.org/10918259/diff/9012/chrome/browser/system_moni...
File chrome/browser/system_monitor/removable_device_notifications_chromeos.h
(right):

https://codereview.chromium.org/10918259/diff/9012/chrome/browser/system_moni...
chrome/browser/system_monitor/removable_device_notifications_chromeos.h:26:
class RemovableDeviceNotificationsCros;
just move this down to line 37

[kmadhusu, 2012-09-17 23:32:23]

https://codereview.chromium.org/10918259/diff/9012/chrome/browser/system_moni...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc
(right):

https://codereview.chromium.org/10918259/diff/9012/chrome/browser/system_moni...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc:89:
// Add the data store id to the device label.
On 2012/09/17 22:29:44, vandebo wrote:
> On 2012/09/17 21:47:56, kmadhusu wrote:
> > The following code appends the data store id to the device label. E.g: "Sony
> > Corporation Sony Digital Still Camera:131073". Do you think, we need to add
> some
> > text such as data store "Id" right before the # (Sony Corporation Sony
Digital
> > Still Camera:ID:131073)? 
> 
> The user won't understand the value anyway, so it's just an opaque part of the
> name.  Ideally we wouldn't show empty storage's, but (id) is a reasonable
> solution for now.

Done.

https://codereview.chromium.org/10918259/diff/9012/chrome/browser/system_moni...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc:92:
if (volume_id.empty()) {
On 2012/09/17 22:29:44, vandebo wrote:
> nit: negate this:
> 
> if (!volume_id.empty()) {
>   device_label += ... volume_id;
> } else {
>   data_stire_id =
>   ...
> }

Done.

https://codereview.chromium.org/10918259/diff/9012/chrome/browser/system_moni...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.h
(right):

https://codereview.chromium.org/10918259/diff/9012/chrome/browser/system_moni...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.h:17:
namespace chromeos {
On 2012/09/17 22:29:44, vandebo wrote:
> Remove this stuff for now.  Since the MTP section in media_storage_util is
CrOS
> specific right just use the full name there.  We'll figure out how to do this
> better once we need it for another mtp impl.

Done.

https://codereview.chromium.org/10918259/diff/9012/chrome/browser/system_moni...
File chrome/browser/system_monitor/removable_device_notifications_chromeos.h
(right):

https://codereview.chromium.org/10918259/diff/9012/chrome/browser/system_moni...
chrome/browser/system_monitor/removable_device_notifications_chromeos.h:26:
class RemovableDeviceNotificationsCros;
On 2012/09/17 22:52:52, Lei Zhang wrote:
> just move this down to line 37

I need this forward declaration right before the typedef. I will add a TODO to
clean up this mess later.

[Lei Zhang, 2012-09-18 00:28:02]

https://chromiumcodereview.appspot.com/10918259/diff/4006/chrome/browser/syst...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc
(right):

https://chromiumcodereview.appspot.com/10918259/diff/4006/chrome/browser/syst...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc:36:
// E.g.: /usb:2,2:12345
Is this the example input or output?

https://chromiumcodereview.appspot.com/10918259/diff/4006/chrome/browser/syst...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc:93:
device_label += chrome::kLeftParen + volume_id + chrome::kRightParen;
nit: StringPrintf here and on line 98?

https://chromiumcodereview.appspot.com/10918259/diff/4006/chrome/browser/syst...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc:97:
if (!data_store_id.empty())
nit: Add {}s

https://chromiumcodereview.appspot.com/10918259/diff/4006/chrome/browser/syst...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.h
(right):

https://chromiumcodereview.appspot.com/10918259/diff/4006/chrome/browser/syst...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.h:57:
base::SystemMonitor::RemovableStorageInfo>
nit: move up to the previous line

https://chromiumcodereview.appspot.com/10918259/diff/4006/chrome/browser/syst...
File chrome/browser/system_monitor/removable_device_notifications_chromeos.cc
(right):

https://chromiumcodereview.appspot.com/10918259/diff/4006/chrome/browser/syst...
chrome/browser/system_monitor/removable_device_notifications_chromeos.cc:215: if
(ContainsKey(mount_map_, mount_info.mount_path))
You mentioned the condition under which this can happen. Can you put that info
as a comment here?

[Lei Zhang, 2012-09-18 00:33:20]

lgtm

https://chromiumcodereview.appspot.com/10918259/diff/4006/chrome/browser/syst...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc
(right):

https://chromiumcodereview.appspot.com/10918259/diff/4006/chrome/browser/syst...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc:191:
storage_map_.find(path_components[0] + path_components[1]);
Use GetDeviceLocationFromStorageName(path_components[1]) to be consistent with
code elsewhere.

[kmadhusu, 2012-09-18 00:50:05]

thestig: Addressed review comments. I am going to start the commit queue
process. I'm happy to address your review comments(if any) in the separate CL.

Thanks.

https://codereview.chromium.org/10918259/diff/4006/chrome/browser/system_moni...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc
(right):

https://codereview.chromium.org/10918259/diff/4006/chrome/browser/system_moni...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc:36:
// E.g.: /usb:2,2:12345
On 2012/09/18 00:28:02, Lei Zhang wrote:
> Is this the example input or output?

Fixed comment.

https://codereview.chromium.org/10918259/diff/4006/chrome/browser/system_moni...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc:93:
device_label += chrome::kLeftParen + volume_id + chrome::kRightParen;
On 2012/09/18 00:28:02, Lei Zhang wrote:
> nit: StringPrintf here and on line 98?

I will do this in a separate clean up CL.

https://codereview.chromium.org/10918259/diff/4006/chrome/browser/system_moni...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc:97:
if (!data_store_id.empty())
On 2012/09/18 00:28:02, Lei Zhang wrote:
> nit: Add {}s

Done.

https://codereview.chromium.org/10918259/diff/4006/chrome/browser/system_moni...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.cc:191:
storage_map_.find(path_components[0] + path_components[1]);
On 2012/09/18 00:33:20, Lei Zhang wrote:
> Use GetDeviceLocationFromStorageName(path_components[1]) to be consistent with
> code elsewhere.

Done.

https://codereview.chromium.org/10918259/diff/4006/chrome/browser/system_moni...
File
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.h
(right):

https://codereview.chromium.org/10918259/diff/4006/chrome/browser/system_moni...
chrome/browser/system_monitor/media_transfer_protocol_device_observer_chromeos.h:57:
base::SystemMonitor::RemovableStorageInfo>
On 2012/09/18 00:28:02, Lei Zhang wrote:
> nit: move up to the previous line

Done.

https://codereview.chromium.org/10918259/diff/4006/chrome/browser/system_moni...
File chrome/browser/system_monitor/removable_device_notifications_chromeos.cc
(right):

https://codereview.chromium.org/10918259/diff/4006/chrome/browser/system_moni...
chrome/browser/system_monitor/removable_device_notifications_chromeos.cc:215: if
(ContainsKey(mount_map_, mount_info.mount_path))
On 2012/09/18 00:28:02, Lei Zhang wrote:
> You mentioned the condition under which this can happen. Can you put that info
> as a comment here?

Done.

[commit-bot: I haz the power, 2012-09-18 00:50:52]

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

[commit-bot: I haz the power, 2012-09-18 02:14:09]

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

[commit-bot: I haz the power, 2012-09-18 03:21:09]

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