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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 5 months ago by kmadhusu
Modified:
1 year, 5 months ago
Reviewers:
Lei Zhang, oshima
CC:
chromium-reviews_chromium.org, 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
Unified diffs Side-by-side diffs Delta from patch set Stats (+274 lines, -74 lines) Lint Patch
M chrome/browser/system_monitor/media_device_notifications_utils.h View 1 2 3 4 5 2 chunks +10 lines, -0 lines 0 comments ? errors Download
M chrome/browser/system_monitor/media_device_notifications_utils.cc View 1 2 3 4 5 2 chunks +25 lines, -0 lines 0 comments ? errors Download
M chrome/browser/system_monitor/media_storage_util.cc View 1 2 3 4 2 chunks +21 lines, -11 lines 0 comments ? errors Download
M chrome/browser/system_monitor/removable_device_notifications_chromeos.h View 1 2 3 4 5 6 1 chunk +13 lines, -2 lines 0 comments ? errors Download
M chrome/browser/system_monitor/removable_device_notifications_chromeos.cc View 1 2 3 4 5 6 7 chunks +51 lines, -30 lines 0 comments ? errors Download
M chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc View 1 2 3 4 11 chunks +139 lines, -25 lines 0 comments ? errors Download
M chromeos/disks/mock_disk_mount_manager.h View 2 chunks +6 lines, -1 line 0 comments ? errors Download
M chromeos/disks/mock_disk_mount_manager.cc View 2 chunks +9 lines, -5 lines 2 comments ? errors Download
Commit:

Messages

Total messages: 21
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 ...
1 year, 5 months ago #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? ...
1 year, 5 months ago #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) + ...
1 year, 5 months ago #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: ...
1 year, 5 months ago #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 ...
1 year, 5 months ago #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() + ", " + ...
1 year, 5 months ago #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 ...
1 year, 5 months ago #7
kmadhusu
CL is ready for review. PTAL. Thanks.
1 year, 5 months ago #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 ...
1 year, 5 months ago #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 ...
1 year, 5 months ago #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 ...
1 year, 5 months ago #11
Lei Zhang
Also, the commit message does not need to say "modified code." Most CLs do that. ...
1 year, 5 months ago #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 ...
1 year, 5 months ago #13
kmadhusu
oshima@: Please review chromeos/disks/ changes. Thanks.
1 year, 5 months ago #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 ...
1 year, 5 months ago #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 ...
1 year, 5 months ago #16
Lei Zhang
lgtm++
1 year, 5 months ago #17
oshima
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 ...
1 year, 5 months ago #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: > ...
1 year, 5 months ago #19
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/11366144/4017
1 year, 5 months ago #20
I haz the power (commit-bot)
1 year, 5 months ago #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 1280:2d3e6564b7b6