[StorageMonitor] Move gallery name generation to StorageInfo.

[StorageMonitor] Move name generation to StorageInfo.

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

[Haojian Wu, 2013-12-23 16:07:58]

Please review it. Thanks.

[vandebo (ex-Chrome), 2014-01-08 19:46:24]

https://codereview.chromium.org/120303003/diff/30001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/media_storage_util.cc (right):

https://codereview.chromium.org/120303003/diff/30001/chrome/browser/storage_m...
chrome/browser/storage_monitor/media_storage_util.cc:187: if
(!GetDeviceInfoFromPath(absolute_path, &storage_info, &relative_path))
I think this ends up jumping through more hoops than we need to. - This change
generally calls MediaGalleryPrefInfo::AbsolutePath() to pass into this function.

Instead, I suggest adding a method to MediaGalleryPrefInfo that will give you
back an StorageInfo.  Then if you have a MediaGalleryPrefInfo and want the
display name, you can just do pref_info.MakeStorageInfo().GetDisplayName()

https://codereview.chromium.org/120303003/diff/30001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_info.cc (right):

https://codereview.chromium.org/120303003/diff/30001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_info.cc:25: string16
GetDisplayNameForDevice(uint64 storage_size_in_bytes,
Please inline this function at it's single call site.

https://codereview.chromium.org/120303003/diff/30001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_info.cc:34: string16
GetDisplayNameForSubFolder(const string16& device_name,
Same with this one.

https://codereview.chromium.org/120303003/diff/30001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_info.h (right):

https://codereview.chromium.org/120303003/diff/30001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_info.h:83: void set_storage_label(const
base::string16& storage_label) {
Remove these new setters. The other setters should go away too, but that can be
done in a different CL.

[Lei Zhang, 2014-01-08 19:55:37]

FYI, you may be interested in https://codereview.chromium.org/19489006/

[Haojian Wu, 2014-01-08 23:56:40]

On 2014/01/08 19:55:37, Lei Zhang wrote:
> FYI, you may be interested in https://codereview.chromium.org/19489006/

Oh, so should we redirect this CL to CL:
https://codereview.chromium.org/19489006/?

I can follow this work, but I will be available on next week since I have school
examinations recently.

[Haojian Wu, 2014-01-20 09:43:00]

This patch has been updated. Sorry for the delay. Please review it again.
Thanks.

https://codereview.chromium.org/120303003/diff/30001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/media_storage_util.cc (right):

https://codereview.chromium.org/120303003/diff/30001/chrome/browser/storage_m...
chrome/browser/storage_monitor/media_storage_util.cc:187: if
(!GetDeviceInfoFromPath(absolute_path, &storage_info, &relative_path))
On 2014/01/08 19:46:25, vandebo wrote:
> I think this ends up jumping through more hoops than we need to. - This change
> generally calls MediaGalleryPrefInfo::AbsolutePath() to pass into this
function.
> 
> Instead, I suggest adding a method to MediaGalleryPrefInfo that will give you
> back an StorageInfo.  Then if you have a MediaGalleryPrefInfo and want the
> display name, you can just do pref_info.MakeStorageInfo().GetDisplayName()

Done. I keep this function(just invoke StorageInfo::GetDisplayName) in
MediaGalleryPrefInfo.

https://codereview.chromium.org/120303003/diff/30001/chrome/browser/storage_m...
File chrome/browser/storage_monitor/storage_info.cc (right):

https://codereview.chromium.org/120303003/diff/30001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_info.cc:25: string16
GetDisplayNameForDevice(uint64 storage_size_in_bytes,
On 2014/01/08 19:46:25, vandebo wrote:
> Please inline this function at it's single call site.

Done.

https://codereview.chromium.org/120303003/diff/30001/chrome/browser/storage_m...
chrome/browser/storage_monitor/storage_info.cc:34: string16
GetDisplayNameForSubFolder(const string16& device_name,
On 2014/01/08 19:46:25, vandebo wrote:
> Same with this one.

Done.

https://codereview.chromium.org/120303003/diff/210001/chrome/browser/storage_...
File chrome/browser/storage_monitor/storage_info.h (right):

https://codereview.chromium.org/120303003/diff/210001/chrome/browser/storage_...
chrome/browser/storage_monitor/storage_info.h:107: base::string16 name_;
Should we also remove this field? since we add GetDisplayName method.

[vandebo (ex-Chrome), 2014-02-26 00:19:16]

Sorry for the very long delay on this review.  It dropped off my radar.  Feel
free to ping if I take too long.

I've looked at this CL as well as the one Lei mentioned.  I think his makes more
sense to do.  At this point, they are both out of date, as storage monitor has
moved into a component.  Would you be interested in cleaning up the other CL?

[Haojian Wu, 2014-02-27 15:29:37]

On 2014/02/26 00:19:16, vandebo wrote:
> Sorry for the very long delay on this review.  It dropped off my radar.  Feel
> free to ping if I take too long.
> 
> I've looked at this CL as well as the one Lei mentioned.  I think his makes
more
> sense to do.  At this point, they are both out of date, as storage monitor has
> moved into a component.  Would you be interested in cleaning up the other CL?

Since Lei@ is OOO, I will take his CL.

[Haojian Wu, 2014-03-03 16:45:01]

On 2014/02/27 15:29:37, Haojian Wu wrote:
> On 2014/02/26 00:19:16, vandebo wrote:
> > Sorry for the very long delay on this review.  It dropped off my radar. 
Feel
> > free to ping if I take too long.
> > 
> > I've looked at this CL as well as the one Lei mentioned.  I think his makes
> more
> > sense to do.  At this point, they are both out of date, as storage monitor
has
> > moved into a component.  Would you be interested in cleaning up the other
CL?
> 
> Since Lei@ is OOO, I will take his CL.

ping vandebo@. I have updated this CL based on Lei@'s CL. Please review it.
Thanks.

BTW, could you help to click the try-bot button to see whether this CL will
break compilation
on other platform(I only test it on windows)?

[vandebo (ex-Chrome), 2014-03-03 19:58:30]

You may have missed updates to these two files:
https://codereview.chromium.org/19489006/diff/140001/chrome/browser/storage_m...
https://codereview.chromium.org/19489006/diff/140001/chrome/browser/extension...

The TEST= line is to specify manual tests to the QA team, please remove it since
you have all automated tests there.

No need to include 261350 on the BUG= line, it has been fixed and verified.

Issue description should probable be about StorageMonitor, not media galleries.

https://codereview.chromium.org/120303003/diff/330001/chrome/browser/extensio...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_apitest.cc
(right):

https://codereview.chromium.org/120303003/diff/330001/chrome/browser/extensio...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_apitest.cc:96:
base::ASCIIToUTF16(kDeviceName), base::string16(),
rename constant

https://codereview.chromium.org/120303003/diff/330001/chrome/browser/extensio...
File chrome/browser/extensions/api/system_storage/storage_api_test_util.cc
(right):

https://codereview.chromium.org/120303003/diff/330001/chrome/browser/extensio...
chrome/browser/extensions/api/system_storage/storage_api_test_util.cc:21:
base::UTF8ToUTF16(unit.name), /* storage label */
nit: unit.name -> unit.label

https://codereview.chromium.org/120303003/diff/330001/chrome/browser/extensio...
File chrome/browser/extensions/api/system_storage/storage_info_provider.cc
(right):

https://codereview.chromium.org/120303003/diff/330001/chrome/browser/extensio...
chrome/browser/extensions/api/system_storage/storage_info_provider.cc:36:
unit->name = base::UTF16ToUTF8(info.storage_label());
This should probably get handled at the lower level as noted elsewhere.

https://codereview.chromium.org/120303003/diff/330001/chrome/browser/media_ga...
File chrome/browser/media_galleries/media_galleries_preferences.cc (left):

https://codereview.chromium.org/120303003/diff/330001/chrome/browser/media_ga...
chrome/browser/media_galleries/media_galleries_preferences.cc:41: #include
"ui/base/text/bytes_formatting.h"
Is this include still used?

https://codereview.chromium.org/120303003/diff/330001/components/storage_moni...
File components/storage_monitor/media_transfer_protocol_device_observer_linux.cc
(right):

https://codereview.chromium.org/120303003/diff/330001/components/storage_moni...
components/storage_monitor/media_transfer_protocol_device_observer_linux.cc:193:
base::string16 device_name;
nit: this is already called label in GetStorageInfo(), please rename it here so
it matches the StorageInfo constructor args.

https://codereview.chromium.org/120303003/diff/330001/components/storage_moni...
components/storage_monitor/media_transfer_protocol_device_observer_linux.cc:207:
0);
Not enough args.

https://codereview.chromium.org/120303003/diff/330001/components/storage_moni...
File components/storage_monitor/portable_device_watcher_win.cc (right):

https://codereview.chromium.org/120303003/diff/330001/components/storage_moni...
components/storage_monitor/portable_device_watcher_win.cc:636: base::string16
storage_name(name + L" (" +
Should storage_name be passed as the model?  It's not really the make volume
label or vendor...

https://codereview.chromium.org/120303003/diff/330001/components/storage_moni...
File components/storage_monitor/storage_info.cc (right):

https://codereview.chromium.org/120303003/diff/330001/components/storage_moni...
components/storage_monitor/storage_info.cc:57: //const base::string16&
device_name,
Remove

https://codereview.chromium.org/120303003/diff/330001/components/storage_moni...
components/storage_monitor/storage_info.cc:192: if
(!IsRemovableDevice(device_id_)) {
Storage Monitor should support names for non-removable devices. If the volume
label is empty, the value that MediaStorageUtil::FindDevicePathById() returns is
probably fine for this case.

https://codereview.chromium.org/120303003/diff/330001/components/storage_moni...
File components/storage_monitor/test_volume_mount_watcher_win.cc (right):

https://codereview.chromium.org/120303003/diff/330001/components/storage_moni...
components/storage_monitor/test_volume_mount_watcher_win.cc:98: const
base::string16& device_name,
nit: device_label ?

[Haojian Wu, 2014-03-04 04:58:18]

On 2014/03/03 19:58:30, vandebo wrote:
> You may have missed updates to these two files:
>
https://codereview.chromium.org/19489006/diff/140001/chrome/browser/storage_m...
>
https://codereview.chromium.org/19489006/diff/140001/chrome/browser/extension...

Done. The media_galleries_eject_apitest.cc has been removed.
 
> The TEST= line is to specify manual tests to the QA team, please remove it
since
> you have all automated tests there.

Done.

> No need to include 261350 on the BUG= line, it has been fixed and verified.
> Issue description should probable be about StorageMonitor, not media
galleries.

Done.

[Haojian Wu, 2014-03-04 05:00:11]

https://codereview.chromium.org/120303003/diff/330001/chrome/browser/extensio...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_apitest.cc
(right):

https://codereview.chromium.org/120303003/diff/330001/chrome/browser/extensio...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_apitest.cc:96:
base::ASCIIToUTF16(kDeviceName), base::string16(),
On 2014/03/03 19:58:31, vandebo wrote:
> rename constant

Done.

https://codereview.chromium.org/120303003/diff/330001/chrome/browser/extensio...
File chrome/browser/extensions/api/system_storage/storage_api_test_util.cc
(right):

https://codereview.chromium.org/120303003/diff/330001/chrome/browser/extensio...
chrome/browser/extensions/api/system_storage/storage_api_test_util.cc:21:
base::UTF8ToUTF16(unit.name), /* storage label */
On 2014/03/03 19:58:31, vandebo wrote:
> nit: unit.name -> unit.label

Since TestStorageUnitInfo should be consistent with StorageUnitInfo defined in
idl. So we should keep the |name| field.

https://codereview.chromium.org/120303003/diff/330001/chrome/browser/extensio...
File chrome/browser/extensions/api/system_storage/storage_info_provider.cc
(right):

https://codereview.chromium.org/120303003/diff/330001/chrome/browser/extensio...
chrome/browser/extensions/api/system_storage/storage_info_provider.cc:36:
unit->name = base::UTF16ToUTF8(info.storage_label());
On 2014/03/03 19:58:31, vandebo wrote:
> This should probably get handled at the lower level as noted elsewhere.

you mean move this out of the BuildStorageUnitInfo function or out of the
if-else case?

https://codereview.chromium.org/120303003/diff/330001/chrome/browser/media_ga...
File chrome/browser/media_galleries/media_galleries_preferences.cc (left):

https://codereview.chromium.org/120303003/diff/330001/chrome/browser/media_ga...
chrome/browser/media_galleries/media_galleries_preferences.cc:41: #include
"ui/base/text/bytes_formatting.h"
On 2014/03/03 19:58:31, vandebo wrote:
> Is this include still used?

Done.

https://codereview.chromium.org/120303003/diff/330001/components/storage_moni...
File components/storage_monitor/media_transfer_protocol_device_observer_linux.cc
(right):

https://codereview.chromium.org/120303003/diff/330001/components/storage_moni...
components/storage_monitor/media_transfer_protocol_device_observer_linux.cc:193:
base::string16 device_name;
On 2014/03/03 19:58:31, vandebo wrote:
> nit: this is already called label in GetStorageInfo(), please rename it here
so
> it matches the StorageInfo constructor args.

Done.

https://codereview.chromium.org/120303003/diff/330001/components/storage_moni...
components/storage_monitor/media_transfer_protocol_device_observer_linux.cc:207:
0);
On 2014/03/03 19:58:31, vandebo wrote:
> Not enough args.

Done.

https://codereview.chromium.org/120303003/diff/330001/components/storage_moni...
File components/storage_monitor/portable_device_watcher_win.cc (right):

https://codereview.chromium.org/120303003/diff/330001/components/storage_moni...
components/storage_monitor/portable_device_watcher_win.cc:636: base::string16
storage_name(name + L" (" +
On 2014/03/03 19:58:31, vandebo wrote:
> Should storage_name be passed as the model?  It's not really the make volume
> label or vendor...

Sounds fine. Done.

https://codereview.chromium.org/120303003/diff/330001/components/storage_moni...
File components/storage_monitor/storage_info.cc (right):

https://codereview.chromium.org/120303003/diff/330001/components/storage_moni...
components/storage_monitor/storage_info.cc:57: //const base::string16&
device_name,
On 2014/03/03 19:58:31, vandebo wrote:
> Remove

Done.

https://codereview.chromium.org/120303003/diff/330001/components/storage_moni...
components/storage_monitor/storage_info.cc:192: if
(!IsRemovableDevice(device_id_)) {
On 2014/03/03 19:58:31, vandebo wrote:
> Storage Monitor should support names for non-removable devices. If the volume
> label is empty, the value that MediaStorageUtil::FindDevicePathById() returns
is
> probably fine for this case.

Done.

https://codereview.chromium.org/120303003/diff/330001/components/storage_moni...
File components/storage_monitor/test_volume_mount_watcher_win.cc (right):

https://codereview.chromium.org/120303003/diff/330001/components/storage_moni...
components/storage_monitor/test_volume_mount_watcher_win.cc:98: const
base::string16& device_name,
On 2014/03/03 19:58:31, vandebo wrote:
> nit: device_label ?

Done.

[vandebo (ex-Chrome), 2014-03-04 19:24:47]

https://codereview.chromium.org/120303003/diff/330001/chrome/browser/extensio...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_apitest.cc
(right):

https://codereview.chromium.org/120303003/diff/330001/chrome/browser/extensio...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_apitest.cc:96:
base::ASCIIToUTF16(kDeviceName), base::string16(),
On 2014/03/04 05:00:12, Haojian Wu wrote:
> On 2014/03/03 19:58:31, vandebo wrote:
> > rename constant
> 
> Done.

Sorry, I meant rename kDeviceName to something like kVolumeLabel.

https://codereview.chromium.org/120303003/diff/330001/chrome/browser/extensio...
File chrome/browser/extensions/api/system_storage/storage_info_provider.cc
(right):

https://codereview.chromium.org/120303003/diff/330001/chrome/browser/extensio...
chrome/browser/extensions/api/system_storage/storage_info_provider.cc:36:
unit->name = base::UTF16ToUTF8(info.storage_label());
On 2014/03/04 05:00:12, Haojian Wu wrote:
> On 2014/03/03 19:58:31, vandebo wrote:
> > This should probably get handled at the lower level as noted elsewhere.
> 
> you mean move this out of the BuildStorageUnitInfo function or out of the
> if-else case?

Now that GetDisplayName works for removable devices, you only need line 33 and
don't need line 36 and line 33 should be moved out of the if.

https://codereview.chromium.org/120303003/diff/350001/components/storage_moni...
File components/storage_monitor/storage_info.cc (right):

https://codereview.chromium.org/120303003/diff/350001/components/storage_moni...
components/storage_monitor/storage_info.cc:189: 
nit: remove blank line

https://codereview.chromium.org/120303003/diff/350001/components/storage_moni...
components/storage_monitor/storage_info.cc:191: if (storage_label_.empty())
if ! empty

[Haojian Wu, 2014-03-05 07:28:08]

https://codereview.chromium.org/120303003/diff/330001/chrome/browser/extensio...
File
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_apitest.cc
(right):

https://codereview.chromium.org/120303003/diff/330001/chrome/browser/extensio...
chrome/browser/extensions/api/media_galleries_private/media_galleries_private_apitest.cc:96:
base::ASCIIToUTF16(kDeviceName), base::string16(),
On 2014/03/04 19:24:47, vandebo wrote:
> On 2014/03/04 05:00:12, Haojian Wu wrote:
> > On 2014/03/03 19:58:31, vandebo wrote:
> > > rename constant
> > 
> > Done.
> 
> Sorry, I meant rename kDeviceName to something like kVolumeLabel.

Done.

https://codereview.chromium.org/120303003/diff/330001/chrome/browser/extensio...
File chrome/browser/extensions/api/system_storage/storage_info_provider.cc
(right):

https://codereview.chromium.org/120303003/diff/330001/chrome/browser/extensio...
chrome/browser/extensions/api/system_storage/storage_info_provider.cc:36:
unit->name = base::UTF16ToUTF8(info.storage_label());
On 2014/03/04 19:24:47, vandebo wrote:
> On 2014/03/04 05:00:12, Haojian Wu wrote:
> > On 2014/03/03 19:58:31, vandebo wrote:
> > > This should probably get handled at the lower level as noted elsewhere.
> > 
> > you mean move this out of the BuildStorageUnitInfo function or out of the
> > if-else case?
> 
> Now that GetDisplayName works for removable devices, you only need line 33 and
> don't need line 36 and line 33 should be moved out of the if.

I see. Done.

https://codereview.chromium.org/120303003/diff/350001/components/storage_moni...
File components/storage_monitor/storage_info.cc (right):

https://codereview.chromium.org/120303003/diff/350001/components/storage_moni...
components/storage_monitor/storage_info.cc:189: 
On 2014/03/04 19:24:48, vandebo wrote:
> nit: remove blank line

Done.

https://codereview.chromium.org/120303003/diff/350001/components/storage_moni...
components/storage_monitor/storage_info.cc:191: if (storage_label_.empty())
On 2014/03/04 19:24:48, vandebo wrote:
> if ! empty 

Done.

https://codereview.chromium.org/120303003/diff/370001/components/storage_moni...
File components/storage_monitor/storage_info.cc (right):

https://codereview.chromium.org/120303003/diff/370001/components/storage_moni...
components/storage_monitor/storage_info.cc:190: return
base::FilePath(location_).LossyDisplayName();
It would be better to return the storage's |location| rather than volume label
as display name for fixed device here.

On windows, it will return the drive letter which is expected.

[vandebo (ex-Chrome), 2014-03-05 17:39:57]

https://codereview.chromium.org/120303003/diff/370001/components/storage_moni...
File components/storage_monitor/storage_info.cc (right):

https://codereview.chromium.org/120303003/diff/370001/components/storage_moni...
components/storage_monitor/storage_info.cc:190: return
base::FilePath(location_).LossyDisplayName();
On 2014/03/05 07:28:09, Haojian Wu wrote:
> It would be better to return the storage's |location| rather than volume label
> as display name for fixed device here.
> 
> On windows, it will return the drive letter which is expected.

Volume label is often empty, in which case we should fall back to location.  If
a windows user has set a volume label, they probably remember it.  On Mac, we
only have volume labels.

[Haojian Wu, 2014-03-07 11:28:21]

https://codereview.chromium.org/120303003/diff/370001/components/storage_moni...
File components/storage_monitor/storage_info.cc (right):

https://codereview.chromium.org/120303003/diff/370001/components/storage_moni...
components/storage_monitor/storage_info.cc:190: return
base::FilePath(location_).LossyDisplayName();
On 2014/03/05 17:39:57, vandebo wrote:
> On 2014/03/05 07:28:09, Haojian Wu wrote:
> > It would be better to return the storage's |location| rather than volume
label
> > as display name for fixed device here.
> > 
> > On windows, it will return the drive letter which is expected.
> 
> Volume label is often empty, in which case we should fall back to location. 
If
> a windows user has set a volume label, they probably remember it.  On Mac, we
> only have volume labels.

Done. Thanks for the explaination.

[vandebo (ex-Chrome), 2014-03-07 16:43:07]

LGTM

[vandebo (ex-Chrome), 2014-03-07 16:43:15]

The CQ bit was checked by vandebo@chromium.org

[commit-bot: I haz the power, 2014-03-07 16:43:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/120303003/390001

[commit-bot: I haz the power, 2014-03-07 17:09:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-07 17:09:46]

Try jobs failed on following builders: linux_chromium_clang_dbg

[Haojian Wu, 2014-03-08 01:41:30]

+ben for ui/base in DEPS
+hongbo for systemInfo part.

[tfarina, 2014-03-08 02:33:09]

On 2014/03/08 01:41:30, Haojian Wu wrote:
> +ben for ui/base in DEPS
Are you sure? There isn't an ui/base/DEPS change in this CL. Rather there is a
components/storage_monitor/DEPS. Which Ben isn't the right owner.

> +hongbo for systemInfo part.

[vandebo (ex-Chrome), 2014-03-08 02:51:07]

On Mar 7, 2014 6:33 PM, <tfarina@chromium.org> wrote:
>
> On 2014/03/08 01:41:30, Haojian Wu wrote:
>>
>> +ben for ui/base in DEPS
>
> Are you sure? There isn't an ui/base/DEPS change in this CL. Rather there
is a
> components/storage_monitor/DEPS. Which Ben isn't the right owner.

Yes, for additions to deps you need  approval from the new dep's owner.

>> +hongbo for systemInfo part.
>
>
>
> https://codereview.chromium.org/120303003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Hongbo Min, 2014-03-10 02:09:42]

On 2014/03/08 01:41:30, Haojian Wu wrote:
> +hongbo for systemInfo part.

lgtm for system info part. Thanks

[Ben Goodger (Google), 2014-03-10 20:11:46]

lgtm for ui/base in DEPS

[Haojian Wu, 2014-03-11 01:19:04]

The CQ bit was checked by hokein.wu@gmail.com

[commit-bot: I haz the power, 2014-03-11 01:22:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/Hokein.Wu@gmail.com/120303003/410001

[commit-bot: I haz the power, 2014-03-11 10:57:08]

Change committed as 256185

[Lei Zhang, 2014-03-11 20:05:01]

On 2014/03/11 10:57:08, I haz the power (commit-bot) wrote:
> Change committed as 256185

Woohoo! Thanks for taking over this CL and landing it.