Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11366144:
[Media Gallery][ChromeOS] Improve device media gallery names. (Closed)
|
Created:
8 years, 1 month ago by kmadhusu Modified:
8 years, 1 month ago CC:
chromium-reviews, stevenjb+watch_chromium.org, oshima+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src 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)
https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc (left): https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:48: file_thread_(BrowserThread::FILE) { Moved the function definitions out of the class declaration.
https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_moni... File chrome/browser/system_monitor/media_storage_util.cc (right): https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_moni... chrome/browser/system_monitor/media_storage_util.cc:266: *device_name = notifier->GetStorageSizeInfo(device_info.location) + Consider localizing this as well? Is this going to stay CrOS only? https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_chromeos.cc (right): https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_chromeos.cc:27: string16 GetDeviceSizeAsString(uint64 total_size_in_bytes) { I think you have reinvented the unlocalized version of FormatBytes(). https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_chromeos.cc:65: "SD Card" : disk.device_label(); Similarly, "SD Card" may not work well for other languages. https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_chromeos.cc:67: device_name = "{" + disk.vendor_name() + ", " + disk.product_name() + "}"; Did you and Steve decide to use braces?
Addressed review comments. PTAL. Thanks. https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_moni... File chrome/browser/system_monitor/media_storage_util.cc (right): https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_moni... chrome/browser/system_monitor/media_storage_util.cc:266: *device_name = notifier->GetStorageSizeInfo(device_info.location) + On 2012/11/08 07:42:44, Lei Zhang wrote: > Consider localizing this as well? > Modified code to use FormatBytes() and it localizes the byte block units. > Is this going to stay CrOS only? No. I will update windows, linux and mac code in a separate CL. https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_chromeos.cc (right): https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_chromeos.cc:27: string16 GetDeviceSizeAsString(uint64 total_size_in_bytes) { On 2012/11/08 07:42:44, Lei Zhang wrote: > I think you have reinvented the unlocalized version of FormatBytes(). oops.. I didn't know that. Fixed. https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_chromeos.cc:65: "SD Card" : disk.device_label(); On 2012/11/08 07:42:44, Lei Zhang wrote: > Similarly, "SD Card" may not work well for other languages. ChromeOS file manager is not localizing this text. So, we decided to use it as such. https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_chromeos.cc:67: device_name = "{" + disk.vendor_name() + ", " + disk.product_name() + "}"; On 2012/11/08 07:42:44, Lei Zhang wrote: > Did you and Steve decide to use braces? Yes (along with the PM's).
https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_chromeos.cc (right): https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_chromeos.cc:65: "SD Card" : disk.device_label(); On 2012/11/08 18:50:35, kmadhusu wrote: > On 2012/11/08 07:42:44, Lei Zhang wrote: > > Similarly, "SD Card" may not work well for other languages. > > ChromeOS file manager is not localizing this text. So, we decided to use it as > such. Ok, can you add a comment for that? https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc (left): https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:48: file_thread_(BrowserThread::FILE) { On 2012/11/08 03:57:25, kmadhusu wrote: > Moved the function definitions out of the class declaration. It would be nice if you can do this in a different CL. That would make the necessary changes from this CL much easier to review. https://codereview.chromium.org/11366144/diff/5002/chrome/browser/system_moni... File chrome/browser/system_monitor/media_storage_util.cc (right): https://codereview.chromium.org/11366144/diff/5002/chrome/browser/system_moni... chrome/browser/system_monitor/media_storage_util.cc:264: #if defined(OS_CHROMEOS) So after this CL, Linux no longer gets folder names? https://codereview.chromium.org/11366144/diff/5002/chrome/browser/system_moni... chrome/browser/system_monitor/media_storage_util.cc:265: if (device_name) { Can we do all the |device_name| appends in one block, rather than splitting it into two? https://codereview.chromium.org/11366144/diff/5002/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_chromeos.h (right): https://codereview.chromium.org/11366144/diff/5002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_chromeos.h:73: string16 storage_size_info; How about storage_size_str ?
https://chromiumcodereview.appspot.com/11366144/diff/5002/chrome/browser/syst... File chrome/browser/system_monitor/removable_device_notifications_chromeos.h (right): https://chromiumcodereview.appspot.com/11366144/diff/5002/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_chromeos.h:73: string16 storage_size_info; BTW, have you considered just storing this as a uint64, instead of a string? That way, only the common places that need to format it for human readability can call FormatBytes(), rather than every GetStorageInfo() function.
https://chromiumcodereview.appspot.com/11366144/diff/5002/chrome/browser/syst... File chrome/browser/system_monitor/removable_device_notifications_chromeos.cc (right): https://chromiumcodereview.appspot.com/11366144/diff/5002/chrome/browser/syst... chrome/browser/system_monitor/removable_device_notifications_chromeos.cc:32: device_name = "{" + disk.vendor_name() + ", " + disk.product_name() + "}"; What if either the vendor or product name is empty? Put this logic in a util function to share with the Linux implementation?
Addressed review comments. PTAL. https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_chromeos.cc (right): https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_chromeos.cc:65: "SD Card" : disk.device_label(); On 2012/11/09 01:12:18, Lei Zhang wrote: > On 2012/11/08 18:50:35, kmadhusu wrote: > > On 2012/11/08 07:42:44, Lei Zhang wrote: > > > Similarly, "SD Card" may not work well for other languages. > > > > ChromeOS file manager is not localizing this text. So, we decided to use it as > > such. > > Ok, can you add a comment for that? As we discussed, we will be showing the base name of the mount path. It can be either a volume label or a generic string "SD Card". https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc (left): https://codereview.chromium.org/11366144/diff/2001/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:48: file_thread_(BrowserThread::FILE) { On 2012/11/09 01:12:18, Lei Zhang wrote: > On 2012/11/08 03:57:25, kmadhusu wrote: > > Moved the function definitions out of the class declaration. > > It would be nice if you can do this in a different CL. That would make the > necessary changes from this CL much easier to review. Done. https://codereview.chromium.org/11366144/diff/5002/chrome/browser/system_moni... File chrome/browser/system_monitor/media_storage_util.cc (right): https://codereview.chromium.org/11366144/diff/5002/chrome/browser/system_moni... chrome/browser/system_monitor/media_storage_util.cc:264: #if defined(OS_CHROMEOS) On 2012/11/09 01:12:18, Lei Zhang wrote: > So after this CL, Linux no longer gets folder names? Yes. Linux changes are ready for review (https://codereview.chromium.org/11363153/). Mac and Win changes will be ready soon. https://codereview.chromium.org/11366144/diff/5002/chrome/browser/system_moni... chrome/browser/system_monitor/media_storage_util.cc:265: if (device_name) { On 2012/11/09 01:12:18, Lei Zhang wrote: > Can we do all the |device_name| appends in one block, rather than splitting it > into two? sure. https://codereview.chromium.org/11366144/diff/5002/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_chromeos.cc (right): https://codereview.chromium.org/11366144/diff/5002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_chromeos.cc:32: device_name = "{" + disk.vendor_name() + ", " + disk.product_name() + "}"; On 2012/11/09 04:51:14, Lei Zhang wrote: > What if either the vendor or product name is empty? Put this logic in a util > function to share with the Linux implementation? Fixed. Created a util function. https://codereview.chromium.org/11366144/diff/5002/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_chromeos.h (right): https://codereview.chromium.org/11366144/diff/5002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_chromeos.h:73: string16 storage_size_info; On 2012/11/09 01:12:18, Lei Zhang wrote: > How about storage_size_str ? storage_size_info -> storage_size. https://codereview.chromium.org/11366144/diff/5002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_chromeos.h:73: string16 storage_size_info; On 2012/11/09 04:47:05, Lei Zhang wrote: > BTW, have you considered just storing this as a uint64, instead of a string? > That way, only the common places that need to format it for human readability > can call FormatBytes(), rather than every GetStorageInfo() function. I don't see any reason to store this as uint64. Every caller who access this function will eventually call FormatBytes.
CL is ready for review. PTAL. Thanks.
https://codereview.chromium.org/11366144/diff/5002/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_chromeos.h (right): https://codereview.chromium.org/11366144/diff/5002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_chromeos.h:73: string16 storage_size_info; On 2012/11/09 21:59:40, kmadhusu wrote: > On 2012/11/09 04:47:05, Lei Zhang wrote: > > BTW, have you considered just storing this as a uint64, instead of a string? > > That way, only the common places that need to format it for human readability > > can call FormatBytes(), rather than every GetStorageInfo() function. > > I don't see any reason to store this as uint64. Every caller who access this > function will eventually call FormatBytes. So with this CL and the Linux equivalent, both removable_device_notifications_chromeos.cc and removable_device_notifications_linux.cc have to call FormatBytes() in their respective GetDeviceInfo functions. Alternatively, you can keep it as a uint64, then the function that calls FormatBytes() will be GetDisplayNameForDevice(). Wouldn't calling FormatBytes() in the function that format the names for display make more sense? https://codereview.chromium.org/11366144/diff/3010/chrome/browser/system_moni... File chrome/browser/system_monitor/media_device_notifications_utils.cc (right): https://codereview.chromium.org/11366144/diff/3010/chrome/browser/system_moni... chrome/browser/system_monitor/media_device_notifications_utils.cc:41: if (!vendor_name.empty() && !model_name.empty()) Do you think this will be a little better? if (vendor_name.empty() && model_name.empty()) return std::string(); std::string product_name; if (vendor_name.empty()) product_name = model_name; else if (model_name.empty()) product_name = vendor_name; else product_name = vendor_name + ", " + model_name; return "(" + product_name + ")"; https://codereview.chromium.org/11366144/diff/3010/chrome/browser/system_moni... File chrome/browser/system_monitor/media_device_notifications_utils.h (right): https://codereview.chromium.org/11366144/diff/3010/chrome/browser/system_moni... chrome/browser/system_monitor/media_device_notifications_utils.h:23: std::string GetDeviceManufacturerName(const std::string& vendor_name, This name is confusing. For computer products, "manufacturer name" means the same thing as "vendor name". How about GetFullProductName instead? https://codereview.chromium.org/11366144/diff/3010/chrome/browser/system_moni... chrome/browser/system_monitor/media_device_notifications_utils.h:28: string16 GetDisplayNameForDevice(const string16& device_partition_size, You need to #include string16.h in the header. https://codereview.chromium.org/11366144/diff/3010/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_chromeos.cc (right): https://codereview.chromium.org/11366144/diff/3010/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_chromeos.cc:246: chrome::MediaStorageUtil::RecordDeviceInfoHistogram(true, unique_id, You are already in the chrome namespace. https://codereview.chromium.org/11366144/diff/3010/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_chromeos.cc:265: device_size + ASCIIToUTF16(" ") + device_label, Use GetDisplayNameForDevice() here? https://codereview.chromium.org/11366144/diff/3010/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc (right): https://codereview.chromium.org/11366144/diff/3010/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:139: FilePath CreateMountPoint(const std::string& dir, unneeded change?
Addressed review comments. PTAL. Thanks. https://codereview.chromium.org/11366144/diff/5002/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_chromeos.h (right): https://codereview.chromium.org/11366144/diff/5002/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_chromeos.h:73: string16 storage_size_info; On 2012/11/12 07:46:56, Lei Zhang wrote: > On 2012/11/09 21:59:40, kmadhusu wrote: > > On 2012/11/09 04:47:05, Lei Zhang wrote: > > > BTW, have you considered just storing this as a uint64, instead of a string? > > > That way, only the common places that need to format it for human > readability > > > can call FormatBytes(), rather than every GetStorageInfo() function. > > > > I don't see any reason to store this as uint64. Every caller who access this > > function will eventually call FormatBytes. > > So with this CL and the Linux equivalent, both > removable_device_notifications_chromeos.cc and > removable_device_notifications_linux.cc have to call FormatBytes() in their > respective GetDeviceInfo functions. > > Alternatively, you can keep it as a uint64, then the function that calls > FormatBytes() will be GetDisplayNameForDevice(). Wouldn't calling FormatBytes() > in the function that format the names for display make more sense? Done. https://codereview.chromium.org/11366144/diff/3010/chrome/browser/system_moni... File chrome/browser/system_monitor/media_device_notifications_utils.cc (right): https://codereview.chromium.org/11366144/diff/3010/chrome/browser/system_moni... chrome/browser/system_monitor/media_device_notifications_utils.cc:41: if (!vendor_name.empty() && !model_name.empty()) On 2012/11/12 07:46:56, Lei Zhang wrote: > Do you think this will be a little better? > > if (vendor_name.empty() && model_name.empty()) > return std::string(); > std::string product_name; > if (vendor_name.empty()) > product_name = model_name; > else if (model_name.empty()) > product_name = vendor_name; > else > product_name = vendor_name + ", " + model_name; > return "(" + product_name + ")"; Sure. I tried to avoid the temp variable "product_name". https://codereview.chromium.org/11366144/diff/3010/chrome/browser/system_moni... File chrome/browser/system_monitor/media_device_notifications_utils.h (right): https://codereview.chromium.org/11366144/diff/3010/chrome/browser/system_moni... chrome/browser/system_monitor/media_device_notifications_utils.h:23: std::string GetDeviceManufacturerName(const std::string& vendor_name, On 2012/11/12 07:46:56, Lei Zhang wrote: > This name is confusing. For computer products, "manufacturer name" means the > same thing as "vendor name". How about GetFullProductName instead? Done. https://codereview.chromium.org/11366144/diff/3010/chrome/browser/system_moni... chrome/browser/system_monitor/media_device_notifications_utils.h:28: string16 GetDisplayNameForDevice(const string16& device_partition_size, On 2012/11/12 07:46:56, Lei Zhang wrote: > You need to #include string16.h in the header. Done. https://codereview.chromium.org/11366144/diff/3010/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_chromeos.cc (right): https://codereview.chromium.org/11366144/diff/3010/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_chromeos.cc:246: chrome::MediaStorageUtil::RecordDeviceInfoHistogram(true, unique_id, On 2012/11/12 07:46:56, Lei Zhang wrote: > You are already in the chrome namespace. This code is in chromeos namespace. Fixed the comment @line 269. https://codereview.chromium.org/11366144/diff/3010/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_chromeos.cc:265: device_size + ASCIIToUTF16(" ") + device_label, On 2012/11/12 07:46:56, Lei Zhang wrote: > Use GetDisplayNameForDevice() here? oops. Fixed. Missed while merging CL's. https://codereview.chromium.org/11366144/diff/3010/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc (right): https://codereview.chromium.org/11366144/diff/3010/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:139: FilePath CreateMountPoint(const std::string& dir, On 2012/11/12 07:46:56, Lei Zhang wrote: > unneeded change? Reverted.
lgtm with nits: https://codereview.chromium.org/11366144/diff/7013/chrome/browser/system_moni... File chrome/browser/system_monitor/media_device_notifications_utils.h (right): https://codereview.chromium.org/11366144/diff/7013/chrome/browser/system_moni... chrome/browser/system_monitor/media_device_notifications_utils.h:22: // Constructs and returns the device product name from |vendor_name| and nit: you don't need to say "and returns", it's implied. https://codereview.chromium.org/11366144/diff/7013/chrome/browser/system_moni... File chrome/browser/system_monitor/media_storage_util.cc (right): https://codereview.chromium.org/11366144/diff/7013/chrome/browser/system_moni... chrome/browser/system_monitor/media_storage_util.cc:258: FilePath sub_folder_path; put this bit of new code below the |device_id| code, so it's closer to the related |device_name| / |relative_path| code. https://codereview.chromium.org/11366144/diff/7013/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_chromeos.cc (right): https://codereview.chromium.org/11366144/diff/7013/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_chromeos.cc:35: if (!mount_point.BaseName().LossyDisplayName().empty()) Save this to a local variable to avoid calling it again on the next line. https://codereview.chromium.org/11366144/diff/7013/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc (right): https://codereview.chromium.org/11366144/diff/7013/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:284: const std::string kUniqueId1 = "FFFF-FFFF"; These can go to the top as const char []s and be shared among multiple tests.
Also, the commit message does not need to say "modified code." Most CLs do that. ;)
https://codereview.chromium.org/11366144/diff/7013/chrome/browser/system_moni... File chrome/browser/system_monitor/media_device_notifications_utils.h (right): https://codereview.chromium.org/11366144/diff/7013/chrome/browser/system_moni... chrome/browser/system_monitor/media_device_notifications_utils.h:22: // Constructs and returns the device product name from |vendor_name| and On 2012/11/12 23:20:52, Lei Zhang wrote: > nit: you don't need to say "and returns", it's implied. Done. https://codereview.chromium.org/11366144/diff/7013/chrome/browser/system_moni... File chrome/browser/system_monitor/media_storage_util.cc (right): https://codereview.chromium.org/11366144/diff/7013/chrome/browser/system_moni... chrome/browser/system_monitor/media_storage_util.cc:258: FilePath sub_folder_path; On 2012/11/12 23:20:52, Lei Zhang wrote: > put this bit of new code below the |device_id| code, so it's closer to the > related |device_name| / |relative_path| code. Done. https://codereview.chromium.org/11366144/diff/7013/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_chromeos.cc (right): https://codereview.chromium.org/11366144/diff/7013/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_chromeos.cc:35: if (!mount_point.BaseName().LossyDisplayName().empty()) On 2012/11/12 23:20:52, Lei Zhang wrote: > Save this to a local variable to avoid calling it again on the next line. Done. https://codereview.chromium.org/11366144/diff/7013/chrome/browser/system_moni... File chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc (right): https://codereview.chromium.org/11366144/diff/7013/chrome/browser/system_moni... chrome/browser/system_monitor/removable_device_notifications_chromeos_unittest.cc:284: const std::string kUniqueId1 = "FFFF-FFFF"; On 2012/11/12 23:20:52, Lei Zhang wrote: > These can go to the top as const char []s and be shared among multiple tests. Done.
oshima@: Please review chromeos/disks/ changes. Thanks.
https://chromiumcodereview.appspot.com/11366144/diff/12030/chrome/browser/sys... File chrome/browser/system_monitor/media_device_notifications_utils.h (right): https://chromiumcodereview.appspot.com/11366144/diff/12030/chrome/browser/sys... chrome/browser/system_monitor/media_device_notifications_utils.h:23: std::string GetFullProductName(const std::string& vendor_name, Also, have you considered moving the IsStringUTF8() call into this function as well? That way callers do no have to worry if the returned string is UTF-8 or not.
https://codereview.chromium.org/11366144/diff/12030/chrome/browser/system_mon... File chrome/browser/system_monitor/media_device_notifications_utils.h (right): https://codereview.chromium.org/11366144/diff/12030/chrome/browser/system_mon... 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 wrote: > Also, have you considered moving the IsStringUTF8() call into this function as > well? That way callers do no have to worry if the returned string is UTF-8 or > not. As we discussed, all the callers of this function will eventually convert the result of this function to string16. Therefore, modified this function to return a string16.
lgtm++
chromeos/ lgtm http://codereview.chromium.org/11366144/diff/4017/chromeos/disks/mock_disk_mo... File chromeos/disks/mock_disk_mount_manager.cc (right): http://codereview.chromium.org/11366144/diff/4017/chromeos/disks/mock_disk_mo... chromeos/disks/mock_disk_mount_manager.cc:209: false); // is_hidden Wow, this argument list is way too long. Probably not your fault, but please consider re-organize them as more structured arguments when you have a chance.
http://codereview.chromium.org/11366144/diff/4017/chromeos/disks/mock_disk_mo... File chromeos/disks/mock_disk_mount_manager.cc (right): http://codereview.chromium.org/11366144/diff/4017/chromeos/disks/mock_disk_mo... chromeos/disks/mock_disk_mount_manager.cc:209: false); // is_hidden On 2012/11/13 23:57:10, oshima wrote: > Wow, this argument list is way too long. Probably not your fault, but please > consider re-organize them as more structured arguments when you have a chance. Sure. Will do.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/11366144/4017
Change committed as 167606 |
