Chromium Code Reviews
Help | Chromium Project | Sign in
(239)

Issue 11366144: [Media Gallery][ChromeOS] Improve device media gallery names. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 9 months ago by kmadhusu
Modified:
2 years, 9 months ago
CC:
chromium-reviews, stevenjb+watch_chromium.org, oshima+watch_chromium.org
Visibility:
Public.

Description

[Media Gallery][ChromeOS] Improve device media gallery names. (1) When a SD card is attached, report a more generic string like "SD card" on the media permissions dialog. (2) When the attached device is not a SD card or when a sub folder of the device is selected as a media gallery, report the gallery name in the following format: <Device_storage_size> <Selected_sub_folder_gallery_name> <Volume_Name> <(Vendor_Name, Model_Name)> Sample gallery names: (1) 1GB TEST_USB (2) 4GB DCIM - RED_USB (3) 8KB PHOTOS - (TESTCOMPANY, A101) (4) 4GB SD Card (5) 16MB SD Card BUG=159671, 158600 TEST=none Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=167606

Patch Set 1 : '' #

Total comments: 13

Patch Set 2 : Addressed review comments #

Total comments: 12

Patch Set 3 : Addressed review comments + Added util functions #

Total comments: 12

Patch Set 4 : Addressed review comments #

Total comments: 8

Patch Set 5 : Addressed review comments. #

Total comments: 2

Patch Set 6 : Addressed comment #

Patch Set 7 : '' #

Total comments: 2

Messages

Total messages: 21 (0 generated)
kmadhusu
https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc File chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc (left): https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc#oldcode48 chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:48: file_thread_(BrowserThread::FILE) { Moved the function definitions out of the ...
2 years, 9 months ago (2012-11-08 03:57:25 UTC) #1
Lei Zhang
https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_monitor/media_storage_util.cc File chrome/browser/system_monitor/media_storage_util.cc (right): https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_monitor/media_storage_util.cc#newcode266 chrome/browser/system_monitor/media_storage_util.cc:266: *device_name = notifier->GetStorageSizeInfo(device_info.location) + Consider localizing this as well? ...
2 years, 9 months ago (2012-11-08 07:42:43 UTC) #2
kmadhusu
Addressed review comments. PTAL. Thanks. https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_monitor/media_storage_util.cc File chrome/browser/system_monitor/media_storage_util.cc (right): https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_monitor/media_storage_util.cc#newcode266 chrome/browser/system_monitor/media_storage_util.cc:266: *device_name = notifier->GetStorageSizeInfo(device_info.location) + ...
2 years, 9 months ago (2012-11-08 18:50:35 UTC) #3
Lei Zhang
https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_monitor/removable_device_notifications_chromeos.cc File chrome/browser/system_monitor/removable_device_notifications_chromeos.cc (right): https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_monitor/removable_device_notifications_chromeos.cc#newcode65 chrome/browser/system_monitor/removable_device_notifications_chromeos.cc:65: "SD Card" : disk.device_label(); On 2012/11/08 18:50:35, kmadhusu wrote: ...
2 years, 9 months ago (2012-11-09 01:12:17 UTC) #4
Lei Zhang
https://chromiumcodereview.appspot.com/11366144/diff/5002/chrome/browser/system_monitor/removable_device_notifications_chromeos.h File chrome/browser/system_monitor/removable_device_notifications_chromeos.h (right): https://chromiumcodereview.appspot.com/11366144/diff/5002/chrome/browser/system_monitor/removable_device_notifications_chromeos.h#newcode73 chrome/browser/system_monitor/removable_device_notifications_chromeos.h:73: string16 storage_size_info; BTW, have you considered just storing this ...
2 years, 9 months ago (2012-11-09 04:47:05 UTC) #5
Lei Zhang
https://chromiumcodereview.appspot.com/11366144/diff/5002/chrome/browser/system_monitor/removable_device_notifications_chromeos.cc File chrome/browser/system_monitor/removable_device_notifications_chromeos.cc (right): https://chromiumcodereview.appspot.com/11366144/diff/5002/chrome/browser/system_monitor/removable_device_notifications_chromeos.cc#newcode32 chrome/browser/system_monitor/removable_device_notifications_chromeos.cc:32: device_name = "{" + disk.vendor_name() + ", " + ...
2 years, 9 months ago (2012-11-09 04:51:14 UTC) #6
kmadhusu
Addressed review comments. PTAL. https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_monitor/removable_device_notifications_chromeos.cc File chrome/browser/system_monitor/removable_device_notifications_chromeos.cc (right): https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_monitor/removable_device_notifications_chromeos.cc#newcode65 chrome/browser/system_monitor/removable_device_notifications_chromeos.cc:65: "SD Card" : disk.device_label(); On ...
2 years, 9 months ago (2012-11-09 21:59:39 UTC) #7
kmadhusu
CL is ready for review. PTAL. Thanks.
2 years, 9 months ago (2012-11-10 03:23:52 UTC) #8
Lei Zhang
https://codereview.chromium.org/11366144/diff/5002/chrome/browser/system_monitor/removable_device_notifications_chromeos.h File chrome/browser/system_monitor/removable_device_notifications_chromeos.h (right): https://codereview.chromium.org/11366144/diff/5002/chrome/browser/system_monitor/removable_device_notifications_chromeos.h#newcode73 chrome/browser/system_monitor/removable_device_notifications_chromeos.h:73: string16 storage_size_info; On 2012/11/09 21:59:40, kmadhusu wrote: > On ...
2 years, 9 months ago (2012-11-12 07:46:56 UTC) #9
kmadhusu
Addressed review comments. PTAL. Thanks. https://codereview.chromium.org/11366144/diff/5002/chrome/browser/system_monitor/removable_device_notifications_chromeos.h File chrome/browser/system_monitor/removable_device_notifications_chromeos.h (right): https://codereview.chromium.org/11366144/diff/5002/chrome/browser/system_monitor/removable_device_notifications_chromeos.h#newcode73 chrome/browser/system_monitor/removable_device_notifications_chromeos.h:73: string16 storage_size_info; On 2012/11/12 ...
2 years, 9 months ago (2012-11-12 19:56:56 UTC) #10
Lei Zhang
lgtm with nits: https://codereview.chromium.org/11366144/diff/7013/chrome/browser/system_monitor/media_device_notifications_utils.h File chrome/browser/system_monitor/media_device_notifications_utils.h (right): https://codereview.chromium.org/11366144/diff/7013/chrome/browser/system_monitor/media_device_notifications_utils.h#newcode22 chrome/browser/system_monitor/media_device_notifications_utils.h:22: // Constructs and returns the device ...
2 years, 9 months ago (2012-11-12 23:20:52 UTC) #11
Lei Zhang
Also, the commit message does not need to say "modified code." Most CLs do that. ...
2 years, 9 months ago (2012-11-12 23:22:45 UTC) #12
kmadhusu
https://codereview.chromium.org/11366144/diff/7013/chrome/browser/system_monitor/media_device_notifications_utils.h File chrome/browser/system_monitor/media_device_notifications_utils.h (right): https://codereview.chromium.org/11366144/diff/7013/chrome/browser/system_monitor/media_device_notifications_utils.h#newcode22 chrome/browser/system_monitor/media_device_notifications_utils.h:22: // Constructs and returns the device product name from ...
2 years, 9 months ago (2012-11-13 01:03:58 UTC) #13
kmadhusu
oshima@: Please review chromeos/disks/ changes. Thanks.
2 years, 9 months ago (2012-11-13 01:07:15 UTC) #14
Lei Zhang
https://chromiumcodereview.appspot.com/11366144/diff/12030/chrome/browser/system_monitor/media_device_notifications_utils.h File chrome/browser/system_monitor/media_device_notifications_utils.h (right): https://chromiumcodereview.appspot.com/11366144/diff/12030/chrome/browser/system_monitor/media_device_notifications_utils.h#newcode23 chrome/browser/system_monitor/media_device_notifications_utils.h:23: std::string GetFullProductName(const std::string& vendor_name, Also, have you considered moving ...
2 years, 9 months ago (2012-11-13 01:13:07 UTC) #15
kmadhusu
https://codereview.chromium.org/11366144/diff/12030/chrome/browser/system_monitor/media_device_notifications_utils.h File chrome/browser/system_monitor/media_device_notifications_utils.h (right): https://codereview.chromium.org/11366144/diff/12030/chrome/browser/system_monitor/media_device_notifications_utils.h#newcode23 chrome/browser/system_monitor/media_device_notifications_utils.h:23: std::string GetFullProductName(const std::string& vendor_name, On 2012/11/13 01:13:07, Lei Zhang ...
2 years, 9 months ago (2012-11-13 01:37:32 UTC) #16
Lei Zhang
lgtm++
2 years, 9 months ago (2012-11-13 01:48:55 UTC) #17
oshima (OOO Aug 24 thru 28)
chromeos/ lgtm http://codereview.chromium.org/11366144/diff/4017/chromeos/disks/mock_disk_mount_manager.cc File chromeos/disks/mock_disk_mount_manager.cc (right): http://codereview.chromium.org/11366144/diff/4017/chromeos/disks/mock_disk_mount_manager.cc#newcode209 chromeos/disks/mock_disk_mount_manager.cc:209: false); // is_hidden Wow, this argument list ...
2 years, 9 months ago (2012-11-13 23:57:10 UTC) #18
kmadhusu
http://codereview.chromium.org/11366144/diff/4017/chromeos/disks/mock_disk_mount_manager.cc File chromeos/disks/mock_disk_mount_manager.cc (right): http://codereview.chromium.org/11366144/diff/4017/chromeos/disks/mock_disk_mount_manager.cc#newcode209 chromeos/disks/mock_disk_mount_manager.cc:209: false); // is_hidden On 2012/11/13 23:57:10, oshima wrote: > ...
2 years, 9 months ago (2012-11-14 00:33:37 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/11366144/4017
2 years, 9 months ago (2012-11-14 00:33:52 UTC) #20
commit-bot: I haz the power
2 years, 9 months ago (2012-11-14 05:27:45 UTC) #21
Change committed as 167606
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld c33a7a4