Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10829228:
[LINUX] Extract the name and id of the device and send it along the device attach message. (Closed)
|
Created:
8 years, 4 months ago by kmadhusu Modified:
8 years, 4 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[LINUX] Extract the name and id of the device and send it along the device attach notification message.
Using udev library, extract the device name and id property. If ID_FS_UUID
and ID_FS_LABEL does not exists, construct a unique id and label using
device model and vendor information.
BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151811
Patch Set 1 #Patch Set 2 : Fix unit tests #Patch Set 3 : '' #Patch Set 4 : '' #
Total comments: 26
Patch Set 5 : Added dependency to udev library to fix componenet build compile error. #
Total comments: 14
Patch Set 6 : Address review comments. #
Total comments: 30
Patch Set 7 : Fixed nits #
Total comments: 52
Patch Set 8 : Addressed review comments #
Total comments: 35
Patch Set 9 : Addressed review comments #Patch Set 10 : Fix nits #
Total comments: 2
Patch Set 11 : '' #Patch Set 12 : '' #
Total comments: 36
Patch Set 13 : fix nits #
Total comments: 19
Patch Set 14 : Added more documentation #Patch Set 15 : '' #
Total comments: 8
Patch Set 16 : Addressed nits #
Messages
Total messages: 34 (0 generated)
thestig: PTAL. Thanks.
Cc'ed vandebo@
http://codereview.chromium.org/10829228/diff/5001/chrome/browser/media_galler... File chrome/browser/media_gallery/media_device_notifications_linux.cc (right): http://codereview.chromium.org/10829228/diff/5001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:12: #include <libudev.h> not: C header should be grouped with the other C headers. http://codereview.chromium.org/10829228/diff/5001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:19: #include "base/string_util.h" nit: alphabetical order. http://codereview.chromium.org/10829228/diff/5001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:59: if (!enumerate) Just CHECK(enumerate). It should never fail, and if it does, we would like to know about it. http://codereview.chromium.org/10829228/diff/5001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:76: udev_list_entry *cur = NULL; nit: foo_struct* varname; Same elsewhere. http://codereview.chromium.org/10829228/diff/5001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:78: udev_list_entry_foreach(cur, devs) { Why not just call udev_list_entry_get_next()? Can this list ever contain more than 1 entry? http://codereview.chromium.org/10829228/diff/5001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:80: sys_path.assign(udev_list_entry_get_name(cur)); Is there a reason you use std_string_var.assign() instead of just '=' ? http://codereview.chromium.org/10829228/diff/5001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:98: if (!udev_ptr) Just CHECK() instead. http://codereview.chromium.org/10829228/diff/5001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:109: udev_device* dev = udev_device_new_from_syspath(udev_ptr, sys_path.c_str()); I think you may be able to just stat(dev_path) and use udev_device_new_from_devnum(). Then you don't even need GetSysPathFromDevPath(). http://codereview.chromium.org/10829228/diff/5001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:115: // Construct a device name using label or vendor and model information. From the comment, I can't tell if this means (label | vendor) & model or label | (vendor & model) stupid natural languages. http://codereview.chromium.org/10829228/diff/5001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:208: bool MediaDeviceNotificationsLinux::GetDeviceInfo(const std::string& dev_path, Is it possible to make this static, get rid of GetDeviceInfoHelper() ? Not sure if you can override this as a static. http://codereview.chromium.org/10829228/diff/5001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:253: (base::strcasecmp(newiter->second.c_str(), Can't you just compare |newiter->second| to |mount_device| with == ? http://codereview.chromium.org/10829228/diff/5001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:301: typedef int EntryPos; just use int. http://codereview.chromium.org/10829228/diff/5001/chrome/browser/media_galler... File chrome/browser/media_gallery/media_device_notifications_linux_unittest.cc (right): http://codereview.chromium.org/10829228/diff/5001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux_unittest.cc:286: UTF8ToUTF16(kDeviceLabel2), No need to change this?
http://codereview.chromium.org/10829228/diff/9001/chrome/browser/media_galler... File chrome/browser/media_gallery/media_device_notifications_linux.cc (left): http://codereview.chromium.org/10829228/diff/9001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:151: // New device mounted. Please retain this comment. http://codereview.chromium.org/10829228/diff/9001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:197: // No device at |mount_point|, save |device| to it. Retain comment please. http://codereview.chromium.org/10829228/diff/9001/chrome/browser/media_galler... File chrome/browser/media_gallery/media_device_notifications_linux.cc (right): http://codereview.chromium.org/10829228/diff/9001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:249: MountPointDeviceMap::iterator newiter = new_mtab.find(mount_point); If you are calling this, then don't call ContainsKey() below, which will have to call find() internally again. http://codereview.chromium.org/10829228/diff/9001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:290: mtab_[mount_point] = std::make_pair(mount_device, device_id); can't this just be olditer->second = ... ? http://codereview.chromium.org/10829228/diff/9001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:336: device_map[mount_device] = std::make_pair(mount_point, entry_pos++); it->second = ... http://codereview.chromium.org/10829228/diff/9001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:344: const std::string mount_device = device_it->first; nit: const std::string& to avoid a copy. http://codereview.chromium.org/10829228/diff/9001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:359: new_mtab[mount_point] = mount_device; new_it->second = mount_device;
thestig: Addressed review comments. PTAL. Thanks. http://codereview.chromium.org/10829228/diff/5001/chrome/browser/media_galler... File chrome/browser/media_gallery/media_device_notifications_linux.cc (right): http://codereview.chromium.org/10829228/diff/5001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:12: #include <libudev.h> On 2012/08/09 00:32:41, Lei Zhang wrote: > not: C header should be grouped with the other C headers. Done. http://codereview.chromium.org/10829228/diff/5001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:19: #include "base/string_util.h" On 2012/08/09 00:32:41, Lei Zhang wrote: > nit: alphabetical order. Done. http://codereview.chromium.org/10829228/diff/5001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:59: if (!enumerate) On 2012/08/09 00:32:41, Lei Zhang wrote: > Just CHECK(enumerate). It should never fail, and if it does, we would like to > know about it. Done. http://codereview.chromium.org/10829228/diff/5001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:76: udev_list_entry *cur = NULL; On 2012/08/09 00:32:41, Lei Zhang wrote: > nit: foo_struct* varname; Same elsewhere. Done. http://codereview.chromium.org/10829228/diff/5001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:78: udev_list_entry_foreach(cur, devs) { On 2012/08/09 00:32:41, Lei Zhang wrote: > Why not just call udev_list_entry_get_next()? Can this list ever contain more > than 1 entry? I don't expect more than one entry. Just to be on the safer side, I am breaking the loop control. http://codereview.chromium.org/10829228/diff/5001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:80: sys_path.assign(udev_list_entry_get_name(cur)); On 2012/08/09 00:32:41, Lei Zhang wrote: > Is there a reason you use std_string_var.assign() instead of just '=' ? Done. http://codereview.chromium.org/10829228/diff/5001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:98: if (!udev_ptr) On 2012/08/09 00:32:41, Lei Zhang wrote: > Just CHECK() instead. Done. http://codereview.chromium.org/10829228/diff/5001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:109: udev_device* dev = udev_device_new_from_syspath(udev_ptr, sys_path.c_str()); On 2012/08/09 00:32:41, Lei Zhang wrote: > I think you may be able to just stat(dev_path) and use > udev_device_new_from_devnum(). Then you don't even need GetSysPathFromDevPath(). Thanks for the pointer. That worked. http://codereview.chromium.org/10829228/diff/5001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:115: // Construct a device name using label or vendor and model information. On 2012/08/09 00:32:41, Lei Zhang wrote: > From the comment, I can't tell if this means > > (label | vendor) & model > or > label | (vendor & model) > > stupid natural languages. Rephrased the comment to "Construct a device name using label or manufacturer(vendor and model) details." http://codereview.chromium.org/10829228/diff/5001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:208: bool MediaDeviceNotificationsLinux::GetDeviceInfo(const std::string& dev_path, On 2012/08/09 00:32:41, Lei Zhang wrote: > Is it possible to make this static, get rid of GetDeviceInfoHelper() ? I don't want to add udev function calls in this class. That is the reason, I added a helper function. > Not sure > if you can override this as a static. I cannot override a static function. http://codereview.chromium.org/10829228/diff/5001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:253: (base::strcasecmp(newiter->second.c_str(), On 2012/08/09 00:32:41, Lei Zhang wrote: > Can't you just compare |newiter->second| to |mount_device| with == ? Done. http://codereview.chromium.org/10829228/diff/5001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:301: typedef int EntryPos; On 2012/08/09 00:32:41, Lei Zhang wrote: > just use int. Done. http://codereview.chromium.org/10829228/diff/5001/chrome/browser/media_galler... File chrome/browser/media_gallery/media_device_notifications_linux_unittest.cc (right): http://codereview.chromium.org/10829228/diff/5001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux_unittest.cc:286: UTF8ToUTF16(kDeviceLabel2), On 2012/08/09 00:32:41, Lei Zhang wrote: > No need to change this? I need to change them. These params should match the values returned by GetDeviceInfo(). http://codereview.chromium.org/10829228/diff/9001/chrome/browser/media_galler... File chrome/browser/media_gallery/media_device_notifications_linux.cc (left): http://codereview.chromium.org/10829228/diff/9001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:151: // New device mounted. On 2012/08/09 01:12:00, Lei Zhang wrote: > Please retain this comment. Done. http://codereview.chromium.org/10829228/diff/9001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:197: // No device at |mount_point|, save |device| to it. On 2012/08/09 01:12:00, Lei Zhang wrote: > Retain comment please. Done. http://codereview.chromium.org/10829228/diff/9001/chrome/browser/media_galler... File chrome/browser/media_gallery/media_device_notifications_linux.cc (right): http://codereview.chromium.org/10829228/diff/9001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:249: MountPointDeviceMap::iterator newiter = new_mtab.find(mount_point); On 2012/08/09 01:12:00, Lei Zhang wrote: > If you are calling this, then don't call ContainsKey() below, which will have to > call find() internally again. Done. http://codereview.chromium.org/10829228/diff/9001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:290: mtab_[mount_point] = std::make_pair(mount_device, device_id); On 2012/08/09 01:12:00, Lei Zhang wrote: > can't this just be olditer->second = ... ? Done. http://codereview.chromium.org/10829228/diff/9001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:336: device_map[mount_device] = std::make_pair(mount_point, entry_pos++); On 2012/08/09 01:12:00, Lei Zhang wrote: > it->second = ... Done. http://codereview.chromium.org/10829228/diff/9001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:344: const std::string mount_device = device_it->first; On 2012/08/09 01:12:00, Lei Zhang wrote: > nit: const std::string& to avoid a copy. Done. http://codereview.chromium.org/10829228/diff/9001/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:359: new_mtab[mount_point] = mount_device; On 2012/08/09 01:12:00, Lei Zhang wrote: > new_it->second = mount_device; Done.
https://chromiumcodereview.appspot.com/10829228/diff/1007/chrome/browser/medi... File chrome/browser/media_gallery/media_device_notifications_linux.cc (right): https://chromiumcodereview.appspot.com/10829228/diff/1007/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.cc:61: udev_ptr = udev_new(); nit: combine with previous line. https://chromiumcodereview.appspot.com/10829228/diff/1007/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.cc:66: if (stat(dev_path.c_str(), &st) == -1) { usually people do < 0 https://chromiumcodereview.appspot.com/10829228/diff/1007/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.cc:81: udev_device* dev = udev_device_new_from_devnum(udev_ptr, dev_type, Can you name this "device"? It's impossible to search for "dev" in this file. :) https://chromiumcodereview.appspot.com/10829228/diff/1007/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.cc:98: const char *vendor_name = NULL, *model_name = NULL; nit: char* https://chromiumcodereview.appspot.com/10829228/diff/1007/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.cc:99: if ((vendor_name = udev_device_get_property_value(dev, kVendor))) nit: extra set of () here and below? https://chromiumcodereview.appspot.com/10829228/diff/1007/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.cc:103: device_name.append(kSeperator); still got a bunch of appends. https://chromiumcodereview.appspot.com/10829228/diff/1007/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.cc:189: return GetDeviceInfoHelper(dev_path, id, name); I don't understand why you need to keep udev out of MediaDeviceNotificationsLinux, but if you do, then just reduce this method to a 1-liner. https://chromiumcodereview.appspot.com/10829228/diff/1007/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.cc:225: // |mount_point| not in |new_mtab| or |mount_device| is no longer mounted in nit: mounted in -> mounted at https://chromiumcodereview.appspot.com/10829228/diff/1007/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.cc:275: typedef std::pair<std::string, int> MountEntryInfo; You can probably fold this into the next typedef, since you don't use MountEntryInfo anywhere else. https://chromiumcodereview.appspot.com/10829228/diff/1007/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.cc:286: MountPointsInfoMap mount_points_info_map; you can declare this and its typedef further below, closer to where it's actually used. https://chromiumcodereview.appspot.com/10829228/diff/1007/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.cc:318: MountPointDeviceMap::iterator new_it = new_mtab.find(mount_point); you may want to rename |new_it| and |it| to |new_mtab_it| and |mount_position_it|, respectively. https://chromiumcodereview.appspot.com/10829228/diff/1007/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.cc:342: if (!GetDeviceInfo(mount_device, device_id, &device_name)) You may also want to keep track of how often this fails. https://chromiumcodereview.appspot.com/10829228/diff/1007/chrome/browser/medi... File chrome/browser/media_gallery/media_device_notifications_linux.h (right): https://chromiumcodereview.appspot.com/10829228/diff/1007/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.h:49: virtual bool GetDeviceInfo(const std::string& dev_path, I suspect you can make this private. https://chromiumcodereview.appspot.com/10829228/diff/1007/chrome/browser/medi... File chrome/browser/media_gallery/media_device_notifications_linux_unittest.cc (right): https://chromiumcodereview.appspot.com/10829228/diff/1007/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux_unittest.cc:286: UTF8ToUTF16(kDeviceLabel2), What I mean is, you can still use ASCIIToUTF16(). https://chromiumcodereview.appspot.com/10829228/diff/1007/chrome/chrome_brows... File chrome/chrome_browser.gypi (right): https://chromiumcodereview.appspot.com/10829228/diff/1007/chrome/chrome_brows... chrome/chrome_browser.gypi:4690: ['OS=="linux"', { It might be slightly better to put this up around line 4524.
thestig: Fixed nits. PTAL. Thanks. http://codereview.chromium.org/10829228/diff/1007/chrome/browser/media_galler... File chrome/browser/media_gallery/media_device_notifications_linux.cc (right): http://codereview.chromium.org/10829228/diff/1007/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:61: udev_ptr = udev_new(); On 2012/08/09 03:47:16, Lei Zhang wrote: > nit: combine with previous line. Done. http://codereview.chromium.org/10829228/diff/1007/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:66: if (stat(dev_path.c_str(), &st) == -1) { On 2012/08/09 03:47:16, Lei Zhang wrote: > usually people do < 0 Done. http://codereview.chromium.org/10829228/diff/1007/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:81: udev_device* dev = udev_device_new_from_devnum(udev_ptr, dev_type, On 2012/08/09 03:47:16, Lei Zhang wrote: > Can you name this "device"? It's impossible to search for "dev" in this file. :) Done. http://codereview.chromium.org/10829228/diff/1007/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:98: const char *vendor_name = NULL, *model_name = NULL; On 2012/08/09 03:47:16, Lei Zhang wrote: > nit: char* Since I am doing multiple declarations on a single statement, I prefer this style. http://codereview.chromium.org/10829228/diff/1007/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:99: if ((vendor_name = udev_device_get_property_value(dev, kVendor))) On 2012/08/09 03:47:16, Lei Zhang wrote: > nit: extra set of () here and below? Since I am using the result value of the expression as a truth value, compiler complains if I dont add extra (). "error: suggest parentheses around assignment used as truth value" http://codereview.chromium.org/10829228/diff/1007/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:103: device_name.append(kSeperator); On 2012/08/09 03:47:16, Lei Zhang wrote: > still got a bunch of appends. Done. I didn't find enough reference to state that operator"+=" or "+" is better than appends. If you think we should not use assigns and appends in the code, may be you should add a note in the c++ coding style guide with a valid reason. http://codereview.chromium.org/10829228/diff/1007/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:189: return GetDeviceInfoHelper(dev_path, id, name); On 2012/08/09 03:47:16, Lei Zhang wrote: > I don't understand why you need to keep udev out of > MediaDeviceNotificationsLinux, but if you do, then just reduce this method to a > 1-liner. Done. http://codereview.chromium.org/10829228/diff/1007/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:225: // |mount_point| not in |new_mtab| or |mount_device| is no longer mounted in On 2012/08/09 03:47:16, Lei Zhang wrote: > nit: mounted in -> mounted at Done. http://codereview.chromium.org/10829228/diff/1007/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:275: typedef std::pair<std::string, int> MountEntryInfo; On 2012/08/09 03:47:16, Lei Zhang wrote: > You can probably fold this into the next typedef, since you don't use > MountEntryInfo anywhere else. Done. http://codereview.chromium.org/10829228/diff/1007/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:286: MountPointsInfoMap mount_points_info_map; On 2012/08/09 03:47:16, Lei Zhang wrote: > you can declare this and its typedef further below, closer to where it's > actually used. Done. http://codereview.chromium.org/10829228/diff/1007/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:318: MountPointDeviceMap::iterator new_it = new_mtab.find(mount_point); On 2012/08/09 03:47:16, Lei Zhang wrote: > you may want to rename |new_it| and |it| to |new_mtab_it| and > |mount_position_it|, respectively. Done. http://codereview.chromium.org/10829228/diff/1007/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:342: if (!GetDeviceInfo(mount_device, device_id, &device_name)) On 2012/08/09 03:47:16, Lei Zhang wrote: > You may also want to keep track of how often this fails. Done. http://codereview.chromium.org/10829228/diff/1007/chrome/browser/media_galler... File chrome/browser/media_gallery/media_device_notifications_linux.h (right): http://codereview.chromium.org/10829228/diff/1007/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.h:49: virtual bool GetDeviceInfo(const std::string& dev_path, On 2012/08/09 03:47:16, Lei Zhang wrote: > I suspect you can make this private. Done. http://codereview.chromium.org/10829228/diff/1007/chrome/browser/media_galler... File chrome/browser/media_gallery/media_device_notifications_linux_unittest.cc (right): http://codereview.chromium.org/10829228/diff/1007/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux_unittest.cc:286: UTF8ToUTF16(kDeviceLabel2), On 2012/08/09 03:47:16, Lei Zhang wrote: > What I mean is, you can still use ASCIIToUTF16(). Done. http://codereview.chromium.org/10829228/diff/1007/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): http://codereview.chromium.org/10829228/diff/1007/chrome/chrome_browser.gypi#... chrome/chrome_browser.gypi:4690: ['OS=="linux"', { On 2012/08/09 03:47:16, Lei Zhang wrote: > It might be slightly better to put this up around line 4524. Done. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... File chrome/browser/media_gallery/media_device_notifications_linux.cc (right): http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:225: base::strcasecmp(newiter->second.c_str(), mount_device.c_str()) != 0) { When I use "==", the unit tests are failing. So, I added base::strcasecmp() check.
http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... File chrome/browser/media_gallery/media_device_notifications_linux.cc (left): http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:176: // Add entries, but overwrite entries for the same mount device. Keep track We probably want to keep the comment about overwriting earlier entries with later ones. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... File chrome/browser/media_gallery/media_device_notifications_linux.cc (right): http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:55: bool GetDeviceInfoHelper(const std::string& dev_path, nit: device_path http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:60: // libudev-related items. remove comment (what, not why) http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:61: udev* udev_ptr = udev_new(); This should be a scoped object. If there isn't already a helper in out udev code you can use ScopedGenericObj http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:64: // Get device file status. remove comment http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:71: // Identify the device type. remove comment http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:72: char dev_type; nit: consistent naming -> device_type http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:80: // Create a new udev_device object from device ID. remove comment http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:81: udev_device* device = udev_device_new_from_devnum(udev_ptr, dev_type, Same here http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:88: // Construct a device name using label or manufacturer(vendor and model) Name should be something that helps the user understand what device we're talking about. So we might want to pass type into this function and do different things for different types. i.e. for MTP devices or even removable devices, the device manufacturer and model is probably a good name. But for things that are usually mounted, then the path is probably the best "name" for the device. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:91: const char* dev_name = NULL; nit: device_name http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:93: (dev_name = udev_device_get_property_value(device, kSerial))) { what are the ownership semantics of udev_device_get_property_value? i.e. does the result need to be freed? http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:97: // Eg: KnCompany_Model2010 why an underscore? name is for presentation to humans, so a space would be more appropriate. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:110: // details. We'll eventually want to have a function that given this id will tell us if the device is attached to the system. So you might want to add a schema to the start of the id string. i.e. uuid: mtp: etc. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:225: base::strcasecmp(newiter->second.c_str(), mount_device.c_str()) != 0) { On 2012/08/09 08:10:03, kmadhusu wrote: > When I use "==", the unit tests are failing. So, I added base::strcasecmp() > check. case should be consistent, so lets understand what's going wrong instead of working around it. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:241: std::string device_id; Move this to just before line 248 and just before line 261. They don't share the value and we like to declare things just before we use them. Actually later comment makes this one moot. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:248: AddNewDevice(mount_device, mount_point, &device_id); Seems like AddNewDevice should be responsible for adding it into mtab_. That way you don't need to pop device_id back up to this scope. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:275: std::pair<std::string, int> > DeviceMap; don't use a pair, use a struct http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:276: DeviceMap device_map; Move declaration of device_map to just before while loop. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:278: MountPointDeviceMap& new_mtab = *mtab; Move new_mtab declaration to just before the for loop. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:304: typedef std::map<std::string, int> MountPointsInfoMap; To make things simpler, I'd suggest pulling this loop apart into two loops. The first one would resolve the mount point collisions and the second would just populate the new_mtab. At the least, it's worth adding a comment here saying what we're going to do.... We need to resolve mount entries that mount different devices to the same mount point where last mount wins. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... File chrome/browser/media_gallery/media_device_notifications_linux.h (right): http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.h:55: typedef std::pair<std::string, std::string> MountDeviceAndId; You don't have to fix it, but if you feel like this, we should change this to a struct instead of a pair. Doing so will make things much easier to read. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.h:97: MountMap mtab_; This is more than the mtab now, right? mtab_ -> mount_info_map_ ?
http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... File chrome/browser/media_gallery/media_device_notifications_linux.cc (right): http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:342: UMA_HISTOGRAM_BOOLEAN("MediaDeviceNotification.device_info_available", Can we distinguish these from the CrOS ones in the dashboard? If not, we should give them a different name.
vandebo@: Addressed review comments (except for the udev_device_get_property_value return value ownership semantics). PTAL. Thanks. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... File chrome/browser/media_gallery/media_device_notifications_linux.cc (left): http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:176: // Add entries, but overwrite entries for the same mount device. Keep track On 2012/08/09 17:52:35, vandebo wrote: > We probably want to keep the comment about overwriting earlier entries with > later ones. Done. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... File chrome/browser/media_gallery/media_device_notifications_linux.cc (right): http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:55: bool GetDeviceInfoHelper(const std::string& dev_path, On 2012/08/09 17:52:35, vandebo wrote: > nit: device_path Done. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:60: // libudev-related items. On 2012/08/09 17:52:35, vandebo wrote: > remove comment (what, not why) Done. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:61: udev* udev_ptr = udev_new(); On 2012/08/09 17:52:35, vandebo wrote: > This should be a scoped object. If there isn't already a helper in out udev > code you can use ScopedGenericObj Done. Thanks for pointing out ScopedGenericObj. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:64: // Get device file status. On 2012/08/09 17:52:35, vandebo wrote: > remove comment Done. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:71: // Identify the device type. On 2012/08/09 17:52:35, vandebo wrote: > remove comment Done. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:72: char dev_type; On 2012/08/09 17:52:35, vandebo wrote: > nit: consistent naming -> device_type Done. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:80: // Create a new udev_device object from device ID. On 2012/08/09 17:52:35, vandebo wrote: > remove comment Done. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:81: udev_device* device = udev_device_new_from_devnum(udev_ptr, dev_type, On 2012/08/09 17:52:35, vandebo wrote: > Same here Done. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:88: // Construct a device name using label or manufacturer(vendor and model) On 2012/08/09 17:52:35, vandebo wrote: > Name should be something that helps the user understand what device we're > talking about. So we might want to pass type into this function and do > different things for different types. i.e. for MTP devices or even removable > devices, the device manufacturer and model is probably a good name. But for > things that are usually mounted, then the path is probably the best "name" for > the device. Label, Serial, Vendor and Model properties return manufacturer details which very much helps the user to identify the device. Sample usb device properties: ID_VENDOR=Kingston ID_MODEL=DataTraveler_2.0 ID_SERIAL=Kingston_DataTraveler_2.0_89900000000000006CB02CDB-0:0 ID_FS_LABEL=KINGSTON http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:91: const char* dev_name = NULL; On 2012/08/09 17:52:35, vandebo wrote: > nit: device_name Done. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:97: // Eg: KnCompany_Model2010 On 2012/08/09 17:52:35, vandebo wrote: > why an underscore? name is for presentation to humans, so a space would be more > appropriate. No specific reason to use underscore. As we discussed, changed the separator to space. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:110: // details. On 2012/08/09 17:52:35, vandebo wrote: > We'll eventually want to have a function that given this id will tell us if the > device is attached to the system. So you might want to add a schema to the > start of the id string. i.e. uuid: mtp: etc. Done. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:225: base::strcasecmp(newiter->second.c_str(), mount_device.c_str()) != 0) { On 2012/08/09 17:52:35, vandebo wrote: > On 2012/08/09 08:10:03, kmadhusu wrote: > > When I use "==", the unit tests are failing. So, I added base::strcasecmp() > > check. > > case should be consistent, so lets understand what's going wrong instead of > working around it. I made a silly mistake in patch 6 that made the tests to fail. Removed strcasecmp call and use the comparison operator. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:241: std::string device_id; On 2012/08/09 17:52:35, vandebo wrote: > Move this to just before line 248 and just before line 261. They don't share > the value and we like to declare things just before we use them. > > Actually later comment makes this one moot. Done. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:248: AddNewDevice(mount_device, mount_point, &device_id); On 2012/08/09 17:52:35, vandebo wrote: > Seems like AddNewDevice should be responsible for adding it into mtab_. That > way you don't need to pop device_id back up to this scope. Done. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:275: std::pair<std::string, int> > DeviceMap; On 2012/08/09 17:52:35, vandebo wrote: > don't use a pair, use a struct Done. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:276: DeviceMap device_map; On 2012/08/09 17:52:35, vandebo wrote: > Move declaration of device_map to just before while loop. Done. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:278: MountPointDeviceMap& new_mtab = *mtab; On 2012/08/09 17:52:35, vandebo wrote: > Move new_mtab declaration to just before the for loop. Done. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:304: typedef std::map<std::string, int> MountPointsInfoMap; On 2012/08/09 17:52:35, vandebo wrote: > To make things simpler, I'd suggest pulling this loop apart into two loops. The > first one would resolve the mount point collisions and the second would just > populate the new_mtab. > > At the least, it's worth adding a comment here saying what we're going to do.... > We need to resolve mount entries that mount different devices to the same mount > point where last mount wins. Done. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:342: UMA_HISTOGRAM_BOOLEAN("MediaDeviceNotification.device_info_available", On 2012/08/09 17:54:47, vandebo wrote: > Can we distinguish these from the CrOS ones in the dashboard? If not, we should > give them a different name. We can use the same name. While viewing, we can filter the platform specific results in the dashboard. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... File chrome/browser/media_gallery/media_device_notifications_linux.h (right): http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.h:55: typedef std::pair<std::string, std::string> MountDeviceAndId; On 2012/08/09 17:52:35, vandebo wrote: > You don't have to fix it, but if you feel like this, we should change this to a > struct instead of a pair. Doing so will make things much easier to read. Done. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.h:64: virtual bool GetDeviceInfo(const std::string& dev_path, Moved this function to anonymous namespace and included a new constructor for unit testing. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.h:97: MountMap mtab_; On 2012/08/09 17:52:35, vandebo wrote: > This is more than the mtab now, right? mtab_ -> mount_info_map_ ? Done.
http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... File chrome/browser/media_gallery/media_device_notifications_linux.cc (right): http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:304: typedef std::map<std::string, int> MountPointsInfoMap; On 2012/08/09 23:41:29, kmadhusu wrote: > On 2012/08/09 17:52:35, vandebo wrote: > > To make things simpler, I'd suggest pulling this loop apart into two loops. > The > > first one would resolve the mount point collisions and the second would just > > populate the new_mtab. > > > > At the least, it's worth adding a comment here saying what we're going to > do.... > > We need to resolve mount entries that mount different devices to the same > mount > > point where last mount wins. > > Done. I guess it wasn't simpler. http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... File chrome/browser/media_gallery/media_device_notifications_linux.cc (right): http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:56: const char kMtpDeviceUniqueIdPrefix[] = "mtp: "; nit: I wouldn't put a space in these http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:68: // Device mount point. nit: comments here are a bit redundant http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:142: switch (media_device_type) { It's not the device type that defines the schema, but how we would be able to look it up... http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:291: CheckAndAddMediaDevice(mount_device, mount_point); Looks like this can be cleaned up a bit now... if (olditer != mount_info_map_.end() && mount_device == olditer->second.mount_device) continue http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:330: DeviceMap::iterator it = device_map.find(mount_device); In either case below, you set the same value... seems like device_map[mount_device] = entry_info; would be simpler. http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:413: MountDeviceAndId mount_device_and_id(mount_device, device_id); why not mount_info_map_[mount_point] = mount_device_and_id; http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... File chrome/browser/media_gallery/media_device_notifications_linux.h (right): http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.h:75: std::string device_unique_id; nit: device_id is probably sufficient
http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... File chrome/browser/media_gallery/media_device_notifications_linux.cc (right): http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:42: // Device property constants. nit: you may want to separate the udev constants from the delimiters. http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:61: MountPointEntryInfo() : entry_pos(-1) {} you don't use this ctor anywhere? http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:92: // Returns true on success, false on failure. On success, returns true and fill in |id| and |name|. http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:128: // Eg: KnCompany Model2010 nit: E.g. http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:129: const char *vendor_name = NULL, *model_name = NULL; If you put these on two lines, you can avoid violating the style guide. You can declare |model_name| a few lines down where it's actually used. Ditto below in the next section with the same pattern. http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:138: *name = UTF8ToUTF16(device_label); Shall we CHECK(IsStringUTF8(device_label)) and see if we get any crashes? I don't know if udev actually guarantees it's UTF-8. http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:142: switch (media_device_type) { This implies we will be adding mtp device handling code to this class too. I'm concerned when we do that, this class will get very complicated. http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:198: CHECK(!path.empty()); Move this out of both ctors and put it in an Init() method? Init() can either contain or call PopulateKnownFileSystemsInfo(). http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:330: DeviceMap::iterator it = device_map.find(mount_device); On 2012/08/10 00:32:58, vandebo wrote: > In either case below, you set the same value... seems like > device_map[mount_device] = entry_info; would be simpler. google3 has a preference to not use map's [] operator. Apparently util/gtl/map-util.h explains why, though I don't see it in the external version here. {A} InsertOrUpdate() in {A} looks rather useful. Maybe we should leave this as is, with a TODO to push map_util.h into base/ and then use InsertOrUpdate() all over the place in this file later? {A} http://code.google.com/p/protobuf/source/browse/trunk/src/google/protobuf/stu... http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... File chrome/browser/media_gallery/media_device_notifications_linux.h (right): http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.h:44: MediaDeviceNotificationsLinux(const FilePath& path, protected? http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.h:67: : mount_device(mount_device), nit: name the parameter can just be "device" so this line looks less confusing.
http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... File chrome/browser/media_gallery/media_device_notifications_linux.cc (right): http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:330: DeviceMap::iterator it = device_map.find(mount_device); On 2012/08/10 10:26:33, Lei Zhang wrote: > On 2012/08/10 00:32:58, vandebo wrote: > > In either case below, you set the same value... seems like > > device_map[mount_device] = entry_info; would be simpler. > > google3 has a preference to not use map's [] operator. Apparently > util/gtl/map-util.h explains why, though I don't see it in the external version > here. {A} InsertOrUpdate() in {A} looks rather useful. Maybe we should leave > this as is, with a TODO to push map_util.h into base/ and then use > InsertOrUpdate() all over the place in this file later? > > {A} > http://code.google.com/p/protobuf/source/browse/trunk/src/google/protobuf/stu... I audited base and found only six uses of insert on a map in four files. Two of those uses actually want to know if the value was already present and one was added by you. That's in contrast to the 47 occurrences of "std::map" in base. So the predominate style seems to be to use [] and not insert. I looked at the reasons in map-util.h and none of them apply in this particular context. If you think Chrome should be using the insert syntax, it should probably be a considered decision (chromium-dev) and appropriate support routines added.
Addressed review comments. PTAL. Thanks. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... File chrome/browser/media_gallery/media_device_notifications_linux.cc (right): http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:93: (dev_name = udev_device_get_property_value(device, kSerial))) { On 2012/08/09 17:52:35, vandebo wrote: > what are the ownership semantics of udev_device_get_property_value? i.e. does > the result need to be freed? I ran Chromium under ASAN and I did not find any memory leaks. So this code is fine. http://codereview.chromium.org/10829228/diff/1008/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:304: typedef std::map<std::string, int> MountPointsInfoMap; On 2012/08/10 00:32:58, vandebo wrote: > On 2012/08/09 23:41:29, kmadhusu wrote: > > On 2012/08/09 17:52:35, vandebo wrote: > > > To make things simpler, I'd suggest pulling this loop apart into two loops. > > The > > > first one would resolve the mount point collisions and the second would just > > > populate the new_mtab. > > > > > > At the least, it's worth adding a comment here saying what we're going to > > do.... > > > We need to resolve mount entries that mount different devices to the same > > mount > > > point where last mount wins. > > > > Done. > > I guess it wasn't simpler. As we discussed, reverted to my previous version of code. http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... File chrome/browser/media_gallery/media_device_notifications_linux.cc (right): http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:42: // Device property constants. On 2012/08/10 10:26:33, Lei Zhang wrote: > nit: you may want to separate the udev constants from the delimiters. Done. http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:56: const char kMtpDeviceUniqueIdPrefix[] = "mtp: "; On 2012/08/10 00:32:58, vandebo wrote: > nit: I wouldn't put a space in these Done. http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:61: MountPointEntryInfo() : entry_pos(-1) {} On 2012/08/10 10:26:33, Lei Zhang wrote: > you don't use this ctor anywhere? Removed. http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:68: // Device mount point. On 2012/08/10 00:32:58, vandebo wrote: > nit: comments here are a bit redundant Done. http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:92: // Returns true on success, false on failure. On 2012/08/10 10:26:33, Lei Zhang wrote: > On success, returns true and fill in |id| and |name|. Done. http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:128: // Eg: KnCompany Model2010 On 2012/08/10 10:26:33, Lei Zhang wrote: > nit: E.g. Done. http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:129: const char *vendor_name = NULL, *model_name = NULL; On 2012/08/10 10:26:33, Lei Zhang wrote: > If you put these on two lines, you can avoid violating the style guide. You can > declare |model_name| a few lines down where it's actually used. > > Ditto below in the next section with the same pattern. Done. http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:138: *name = UTF8ToUTF16(device_label); On 2012/08/10 10:26:33, Lei Zhang wrote: > Shall we CHECK(IsStringUTF8(device_label)) and see if we get any crashes? I > don't know if udev actually guarantees it's UTF-8. Done. http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:142: switch (media_device_type) { On 2012/08/10 10:26:33, Lei Zhang wrote: > This implies we will be adding mtp device handling code to this class too. I'm > concerned when we do that, this class will get very complicated. What vandebo@ meant is to add "UUID, VID, MID, SSID" prefix so that we know what type of id we are using for the specified device. E.g.: (1) If we use FS_UUID as device unique id, we will have UUID:FFF0-000F (2) If we use Vendor ID as device unique id, we will have VID:A091 Similary for model id and serial short id we will have MID:W23E and SSID:QWE123123 This will be helpful while checking whether the device with an id exists or not. We can easily identify the property from which the device id is constructed. http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:198: CHECK(!path.empty()); On 2012/08/10 10:26:33, Lei Zhang wrote: > Move this out of both ctors and put it in an Init() method? Init() can either > contain or call PopulateKnownFileSystemsInfo(). Done. http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:291: CheckAndAddMediaDevice(mount_device, mount_point); On 2012/08/10 00:32:58, vandebo wrote: > Looks like this can be cleaned up a bit now... > > if (olditer != mount_info_map_.end() && mount_device == > olditer->second.mount_device) > continue Done. http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:330: DeviceMap::iterator it = device_map.find(mount_device); On 2012/08/10 00:32:58, vandebo wrote: > In either case below, you set the same value... seems like > device_map[mount_device] = entry_info; would be simpler. Done. http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:413: MountDeviceAndId mount_device_and_id(mount_device, device_id); On 2012/08/10 00:32:58, vandebo wrote: > why not mount_info_map_[mount_point] = mount_device_and_id; Done. http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... File chrome/browser/media_gallery/media_device_notifications_linux.h (right): http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.h:44: MediaDeviceNotificationsLinux(const FilePath& path, On 2012/08/10 10:26:33, Lei Zhang wrote: > protected? Done. http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.h:67: : mount_device(mount_device), On 2012/08/10 10:26:33, Lei Zhang wrote: > nit: name the parameter can just be "device" so this line looks less confusing. Done. http://codereview.chromium.org/10829228/diff/2004/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.h:75: std::string device_unique_id; On 2012/08/10 00:32:58, vandebo wrote: > nit: device_id is probably sufficient Done.
http://codereview.chromium.org/10829228/diff/4013/chrome/browser/media_galler... File chrome/browser/media_gallery/media_device_notifications_linux.cc (right): http://codereview.chromium.org/10829228/diff/4013/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:148: *id += kVendorIdPrefix; It'd be pretty tough to parse these inline, but all the prefixes at the start. And you really only have to prefixes, uuid and the other one. If one of the vendor, model, short serial is missing that's ok, it's value in the string is just empty. E.g.: VendorModelSerial:Kn_DataTravel_134891 E.g.: VendorModelSerial:Kn__134891 E.g.: VendorModelSerial:Kn_DataTravel_
http://codereview.chromium.org/10829228/diff/4013/chrome/browser/media_galler... File chrome/browser/media_gallery/media_device_notifications_linux.cc (right): http://codereview.chromium.org/10829228/diff/4013/chrome/browser/media_galler... chrome/browser/media_gallery/media_device_notifications_linux.cc:148: *id += kVendorIdPrefix; On 2012/08/11 00:55:20, vandebo wrote: > It'd be pretty tough to parse these inline, but all the prefixes at the start. > And you really only have to prefixes, uuid and the other one. If one of the > vendor, model, short serial is missing that's ok, it's value in the string is > just empty. > > E.g.: VendorModelSerial:Kn_DataTravel_134891 > E.g.: VendorModelSerial:Kn__134891 > E.g.: VendorModelSerial:Kn_DataTravel_ Sure and also made code changes to use ":" as our delim. I was testing with a different device today and noticed that the ID_MODEL had a "_" in it.
LGTM with nits http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... File chrome/browser/media_gallery/media_device_notifications_linux.cc (right): http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:28: using base::SystemMonitor; This is already on line 172. http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:44: // Device property constants. nit: udev device property constants. http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:56: const char kColonDelim[] = ":"; nit: maybe call this kNonSpaceDelim? That way if we want to change what the value is, we don't have to rename all the uses? http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:92: ScopedGenericObj<struct udev*, ScopedReleaseUdevObject> udev_ptr(udev_new()); nit: we try not to put types into variable names. udev, or udev_object is probably better http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:93: CHECK(udev_ptr.get()); I'm not sure - should this be fatal, or should it just return false? http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:95: struct stat st; nit: st -> device_stat ? http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:139: *id += kFSUniqueIdPrefix; I don't think we want to append to id, we want to set it... *id = kFSUniqueIdPrefix + uuid; http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:145: // E.g. 1: VendorModelSerial:Kn:DataTravel_12.10:8000000000006CB02CDB One example is probably sufficient. http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:154: *id = kVendorModelSerialPrefix; The following might be easier to read, what do you think? *id = kVendorModelSerialPrefix + (vendor ? vendor : "") + kColonDelim + (model ? model : "") + kColonDelim + (serial_short ? serial_short : "") + kColonDelim; http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:198: for (size_t i = 0; i < arraysize(kKnownFileSystems); ++i) { nit: remove the {}'s ? http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:251: for (MountMap::const_iterator it = mount_info_map_.begin(); nit: for consistency, you may want to use old_iter here http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:255: MountPointDeviceMap::iterator newiter = new_mtab.find(mount_point); nit: new_iter http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:270: for (MountPointDeviceMap::iterator newiter = new_mtab.begin(); nit: new_iter http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:275: MountMap::iterator olditer = mount_info_map_.find(mount_point); nit: old_iter http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:279: continue; With the rest of the for body now a single statement, it makes sense to reverse the case of the above if and remove the the continue. http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:312: device_map[entry.mnt_fsname /* mount device */] = entry_info; Generally, we don't use inline comments. http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:316: // Helper map to store mount point entries. nit: two spaces http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... File chrome/browser/media_gallery/media_device_notifications_linux.h (right): http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.h:62: // Store mounted device path and id. nit: mounted device path is a confusing term, probably don't need a comment here at all.
Addressed nits. http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... File chrome/browser/media_gallery/media_device_notifications_linux.cc (right): http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:28: using base::SystemMonitor; On 2012/08/13 18:31:18, vandebo wrote: > This is already on line 172. Removed. http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:44: // Device property constants. On 2012/08/13 18:31:18, vandebo wrote: > nit: udev device property constants. Done. http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:56: const char kColonDelim[] = ":"; On 2012/08/13 18:31:18, vandebo wrote: > nit: maybe call this kNonSpaceDelim? That way if we want to change what the > value is, we don't have to rename all the uses? Done. http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:92: ScopedGenericObj<struct udev*, ScopedReleaseUdevObject> udev_ptr(udev_new()); On 2012/08/13 18:31:18, vandebo wrote: > nit: we try not to put types into variable names. udev, or udev_object is > probably better udev_ptr => udev_obj http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:93: CHECK(udev_ptr.get()); On 2012/08/13 18:31:18, vandebo wrote: > I'm not sure - should this be fatal, or should it just return false? thestig@ preferred CHECK. It is very unlikely to happen. I think we can leave it as it is. http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:95: struct stat st; On 2012/08/13 18:31:18, vandebo wrote: > nit: st -> device_stat ? Done. http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:139: *id += kFSUniqueIdPrefix; On 2012/08/13 18:31:18, vandebo wrote: > I don't think we want to append to id, we want to set it... > *id = kFSUniqueIdPrefix + uuid; I tried this format before. Compiler complains "error: invalid operands of types ‘const char [6]’ and ‘const char*’ to binary ‘operator+’" . So I am leaving it as it is. Fixed "+=" to "=" http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:145: // E.g. 1: VendorModelSerial:Kn:DataTravel_12.10:8000000000006CB02CDB On 2012/08/13 18:31:18, vandebo wrote: > One example is probably sufficient. Done. http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:154: *id = kVendorModelSerialPrefix; On 2012/08/13 18:31:18, vandebo wrote: > The following might be easier to read, what do you think? > *id = kVendorModelSerialPrefix + > (vendor ? vendor : "") + kColonDelim + > (model ? model : "") + kColonDelim + > (serial_short ? serial_short : "") + kColonDelim; Leaving as it is. http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:198: for (size_t i = 0; i < arraysize(kKnownFileSystems); ++i) { On 2012/08/13 18:31:18, vandebo wrote: > nit: remove the {}'s ? Done. http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:251: for (MountMap::const_iterator it = mount_info_map_.begin(); On 2012/08/13 18:31:18, vandebo wrote: > nit: for consistency, you may want to use old_iter here Done. http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:255: MountPointDeviceMap::iterator newiter = new_mtab.find(mount_point); On 2012/08/13 18:31:18, vandebo wrote: > nit: new_iter Done. http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:270: for (MountPointDeviceMap::iterator newiter = new_mtab.begin(); On 2012/08/13 18:31:18, vandebo wrote: > nit: new_iter Done. http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:275: MountMap::iterator olditer = mount_info_map_.find(mount_point); On 2012/08/13 18:31:18, vandebo wrote: > nit: old_iter Done. http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:279: continue; On 2012/08/13 18:31:18, vandebo wrote: > With the rest of the for body now a single statement, it makes sense to reverse > the case of the above if and remove the the continue. Since you stated this code set in your previous comment, I had this way. Fixed. http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:312: device_map[entry.mnt_fsname /* mount device */] = entry_info; On 2012/08/13 18:31:18, vandebo wrote: > Generally, we don't use inline comments. Done. http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.cc:316: // Helper map to store mount point entries. On 2012/08/13 18:31:18, vandebo wrote: > nit: two spaces Done. http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... File chrome/browser/media_gallery/media_device_notifications_linux.h (right): http://codereview.chromium.org/10829228/diff/11007/chrome/browser/media_galle... chrome/browser/media_gallery/media_device_notifications_linux.h:62: // Store mounted device path and id. On 2012/08/13 18:31:18, vandebo wrote: > nit: mounted device path is a confusing term, probably don't need a comment here > at all. Done.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/10829228/5009
Presubmit check for 10829228-5009 failed and returned exit status 1.
Running presubmit commit checks ...
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome
Presubmit checks took 1.3s to calculate.
jhawkins@: Could you do an OWNERS review for chrome/ dir? Thanks.
https://chromiumcodereview.appspot.com/10829228/diff/5009/chrome/browser/medi... File chrome/browser/media_gallery/media_device_notifications_linux.cc (right): https://chromiumcodereview.appspot.com/10829228/diff/5009/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.cc:91: CHECK(udev_obj.get()); CHECK should only be used to debug a crash in the wild. https://chromiumcodereview.appspot.com/10829228/diff/5009/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.cc:138: *id = kFSUniqueIdPrefix; kFSUniqueIdPrefix + uuid; https://chromiumcodereview.appspot.com/10829228/diff/5009/chrome/browser/medi... File chrome/browser/media_gallery/media_device_notifications_linux.h (right): https://chromiumcodereview.appspot.com/10829228/diff/5009/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.h:28: typedef bool (*GetDeviceInfoFunc)(const std::string& device_path, nit: Documentation. https://chromiumcodereview.appspot.com/10829228/diff/5009/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.h:62: struct MountDeviceAndId { nit: Documentation. https://chromiumcodereview.appspot.com/10829228/diff/5009/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.h:71: // Helper map to get new entries from mtab file. nit: s/Helper m/M/ https://chromiumcodereview.appspot.com/10829228/diff/5009/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.h:76: // Parse the mtab file and find all changes. nit: All of these method comments should be third-person active, e.g., Parses. https://chromiumcodereview.appspot.com/10829228/diff/5009/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.h:106: GetDeviceInfoFunc get_device_info_func_; nit: Documentation.
jhawkins: Added more documentation. PTAL. Thanks. https://chromiumcodereview.appspot.com/10829228/diff/5009/chrome/browser/medi... File chrome/browser/media_gallery/media_device_notifications_linux.cc (right): https://chromiumcodereview.appspot.com/10829228/diff/5009/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.cc:91: CHECK(udev_obj.get()); On 2012/08/13 20:30:37, James Hawkins wrote: > CHECK should only be used to debug a crash in the wild. thestig@ preferred a CHECK rather than a DCHECK or if. Since you and vandebo@ raised concerns about this, I am changing this to an if block. https://chromiumcodereview.appspot.com/10829228/diff/5009/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.cc:138: *id = kFSUniqueIdPrefix; On 2012/08/13 20:30:37, James Hawkins wrote: > kFSUniqueIdPrefix + uuid; I tried this code before. Compiler complains "error: invalid operands of types ‘const char [6]’ and ‘const char*’ to binary ‘operator+’" . So I am leaving it as it is." https://chromiumcodereview.appspot.com/10829228/diff/5009/chrome/browser/medi... File chrome/browser/media_gallery/media_device_notifications_linux.h (right): https://chromiumcodereview.appspot.com/10829228/diff/5009/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.h:28: typedef bool (*GetDeviceInfoFunc)(const std::string& device_path, On 2012/08/13 20:30:37, James Hawkins wrote: > nit: Documentation. Done. https://chromiumcodereview.appspot.com/10829228/diff/5009/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.h:62: struct MountDeviceAndId { On 2012/08/13 20:30:37, James Hawkins wrote: > nit: Documentation. Done. https://chromiumcodereview.appspot.com/10829228/diff/5009/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.h:71: // Helper map to get new entries from mtab file. On 2012/08/13 20:30:37, James Hawkins wrote: > nit: s/Helper m/M/ Done. https://chromiumcodereview.appspot.com/10829228/diff/5009/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.h:76: // Parse the mtab file and find all changes. On 2012/08/13 20:30:37, James Hawkins wrote: > nit: All of these method comments should be third-person active, e.g., Parses. Done. https://chromiumcodereview.appspot.com/10829228/diff/5009/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.h:106: GetDeviceInfoFunc get_device_info_func_; On 2012/08/13 20:30:37, James Hawkins wrote: > nit: Documentation. Done.
https://chromiumcodereview.appspot.com/10829228/diff/5009/chrome/browser/medi... File chrome/browser/media_gallery/media_device_notifications_linux.cc (right): https://chromiumcodereview.appspot.com/10829228/diff/5009/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.cc:91: CHECK(udev_obj.get()); On 2012/08/13 21:29:43, kmadhusu wrote: > On 2012/08/13 20:30:37, James Hawkins wrote: > > CHECK should only be used to debug a crash in the wild. > > thestig@ preferred a CHECK rather than a DCHECK or if. Since you and vandebo@ > raised concerns about this, I am changing this to an if block. Why use an if block? When can it ever be NULL? https://chromiumcodereview.appspot.com/10829228/diff/5009/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.cc:138: *id = kFSUniqueIdPrefix; On 2012/08/13 21:29:43, kmadhusu wrote: > On 2012/08/13 20:30:37, James Hawkins wrote: > > kFSUniqueIdPrefix + uuid; > > I tried this code before. Compiler complains "error: invalid operands of types > ‘const char [6]’ and ‘const char*’ to binary ‘operator+’" . So I am leaving it > as it is." *id = std::string(kFSUniqueIdPrefix) + uuid; https://chromiumcodereview.appspot.com/10829228/diff/5009/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.cc:138: *id = kFSUniqueIdPrefix; On 2012/08/13 21:29:43, kmadhusu wrote: > On 2012/08/13 20:30:37, James Hawkins wrote: > > kFSUniqueIdPrefix + uuid; > > I tried this code before. Compiler complains "error: invalid operands of types > ‘const char [6]’ and ‘const char*’ to binary ‘operator+’" . So I am leaving it > as it is." This should work: *id = std::string(kFSUniqueIdPrefix) + uuid; https://chromiumcodereview.appspot.com/10829228/diff/12015/chrome/browser/med... File chrome/browser/media_gallery/media_device_notifications_linux.cc (right): https://chromiumcodereview.appspot.com/10829228/diff/12015/chrome/browser/med... chrome/browser/media_gallery/media_device_notifications_linux.cc:134: if (IsStringUTF8(device_label)) When is |device_label| not UTF8? https://chromiumcodereview.appspot.com/10829228/diff/12015/chrome/browser/med... chrome/browser/media_gallery/media_device_notifications_linux.cc:194: CHECK(!mtab_path_.empty()); DCHECK https://chromiumcodereview.appspot.com/10829228/diff/12015/chrome/browser/med... chrome/browser/media_gallery/media_device_notifications_linux.cc:271: ++new_iter) { Optional nit: This can fit on the line above. https://chromiumcodereview.appspot.com/10829228/diff/12015/chrome/browser/med... File chrome/browser/media_gallery/media_device_notifications_linux.h (right): https://chromiumcodereview.appspot.com/10829228/diff/12015/chrome/browser/med... chrome/browser/media_gallery/media_device_notifications_linux.h:86: // Checks and add |mount_device| as media device given the |mount_point|. nit: s/add/adds/
jhawkins@: Addressed nits. PTAL. Thanks. https://chromiumcodereview.appspot.com/10829228/diff/5009/chrome/browser/medi... File chrome/browser/media_gallery/media_device_notifications_linux.cc (right): https://chromiumcodereview.appspot.com/10829228/diff/5009/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.cc:91: CHECK(udev_obj.get()); On 2012/08/14 16:16:29, James Hawkins wrote: > On 2012/08/13 21:29:43, kmadhusu wrote: > > On 2012/08/13 20:30:37, James Hawkins wrote: > > > CHECK should only be used to debug a crash in the wild. > > > > thestig@ preferred a CHECK rather than a DCHECK or if. Since you and vandebo@ > > raised concerns about this, I am changing this to an if block. > > Why use an if block? When can it ever be NULL? udev_new() creates a new udev library context. udev library reads the udev configuration file, and fills in the default values. For some reason, if the library is unable to set the default device path, system path values or if it is unable to allocate memory for struct, it returns NULL. It is very unlikely to happen. But it can return NULL on these circumstances. https://chromiumcodereview.appspot.com/10829228/diff/5009/chrome/browser/medi... chrome/browser/media_gallery/media_device_notifications_linux.cc:138: *id = kFSUniqueIdPrefix; On 2012/08/14 16:16:29, James Hawkins wrote: > On 2012/08/13 21:29:43, kmadhusu wrote: > > On 2012/08/13 20:30:37, James Hawkins wrote: > > > kFSUniqueIdPrefix + uuid; > > > > I tried this code before. Compiler complains "error: invalid operands of types > > ‘const char [6]’ and ‘const char*’ to binary ‘operator+’" . So I am leaving it > > as it is." > > This should work: > > *id = std::string(kFSUniqueIdPrefix) + uuid; Done. https://chromiumcodereview.appspot.com/10829228/diff/12015/chrome/browser/med... File chrome/browser/media_gallery/media_device_notifications_linux.cc (right): https://chromiumcodereview.appspot.com/10829228/diff/12015/chrome/browser/med... chrome/browser/media_gallery/media_device_notifications_linux.cc:134: if (IsStringUTF8(device_label)) On 2012/08/14 16:16:29, James Hawkins wrote: > When is |device_label| not UTF8? With the devices we tested, |device_label| is in UTF8. But we are not sure about all devices. As a precaution, we are doing that check here. https://chromiumcodereview.appspot.com/10829228/diff/12015/chrome/browser/med... chrome/browser/media_gallery/media_device_notifications_linux.cc:194: CHECK(!mtab_path_.empty()); On 2012/08/14 16:16:29, James Hawkins wrote: > DCHECK Done. https://chromiumcodereview.appspot.com/10829228/diff/12015/chrome/browser/med... chrome/browser/media_gallery/media_device_notifications_linux.cc:271: ++new_iter) { On 2012/08/14 16:16:29, James Hawkins wrote: > Optional nit: This can fit on the line above. Done. https://chromiumcodereview.appspot.com/10829228/diff/12015/chrome/browser/med... File chrome/browser/media_gallery/media_device_notifications_linux.h (right): https://chromiumcodereview.appspot.com/10829228/diff/12015/chrome/browser/med... chrome/browser/media_gallery/media_device_notifications_linux.h:86: // Checks and add |mount_device| as media device given the |mount_point|. On 2012/08/14 16:16:29, James Hawkins wrote: > nit: s/add/adds/ Done.
jhawkins@: ping
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/10829228/14012
Try job failure for 10829228-14012 (retry) on win for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/10829228/14012
Try job failure for 10829228-14012 (retry) on linux_rel for step "interactive_ui_tests". It's a second try, previously, steps "interactive_ui_tests, browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/10829228/14012
Change committed as 151811 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
