C++ Readability Review for thestig.

content/browser/media_device_notifications_linux.cc

Index: content/browser/media_device_notifications_linux.cc
===================================================================
--- content/browser/media_device_notifications_linux.cc	(revision 128430)
+++ content/browser/media_device_notifications_linux.cc	(working copy)
@@ -8,16 +8,15 @@
 #include <stdio.h>
 
 #include "base/bind.h"
+#include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/string_util.h"
 #include "base/system_monitor/system_monitor.h"
 #include "content/public/browser/browser_thread.h"
 
-using base::SystemMonitor;
-
 namespace {
 
-const char* const kDCIMDirName = "DCIM";
+const char kDCIMDirName[] = "DCIM";
 
 // List of file systems we care about.
 const char* const kKnownFileSystems[] = {
@@ -37,6 +36,44 @@
 
 namespace content {
 
+using base::SystemMonitor;
+
+// A simple pass-through class. MediaDeviceNotificationsLinux cannot directly
+// inherit from FilePathWatcher::Delegate due to multiple inheritance.
+class MediaDeviceNotificationsLinux::WatcherDelegate
+    : public base::files::FilePathWatcher::Delegate {
+ public:
+  explicit WatcherDelegate(MediaDeviceNotificationsLinux* notifier);
+
+  // base::files::FilePathWatcher::Delegate implementation.
+  virtual void OnFilePathChanged(const FilePath& path) OVERRIDE;
+
+ private:
+  friend class base::RefCountedThreadSafe<WatcherDelegate>;
+
+  // Avoids code deleting the object while there are references to it.
+  virtual ~WatcherDelegate();
+
+  // The MediaDeviceNotificationsLinux instance that owns this WatcherDelegate.
+  // No need to reference it, as that would create a circular reference.

[ajl, 2012/04/03 23:35:57]

What do you mean by "reference" here - are you referring to reference counts? 
If so, that should be stated explicitly.

In purely literal terms this pointer *does* reference the
MediaDeviceNotificationsLinux instance (it points or refers to it), which makes
the comment confusing.

[Lei Zhang, 2012/04/04 05:31:29]

Done.

+  MediaDeviceNotificationsLinux* notifier_;
+
+  DISALLOW_COPY_AND_ASSIGN(WatcherDelegate);
+};
+
+MediaDeviceNotificationsLinux::WatcherDelegate::WatcherDelegate(
+    MediaDeviceNotificationsLinux* notifier)
+    : notifier_(notifier) {
+}
+
+MediaDeviceNotificationsLinux::WatcherDelegate::~WatcherDelegate() {
+}
+
+void MediaDeviceNotificationsLinux::WatcherDelegate::OnFilePathChanged(
+    const FilePath& path) {
+  notifier_->OnFilePathChanged(path);
+}
+
 MediaDeviceNotificationsLinux::MediaDeviceNotificationsLinux(
     const FilePath& path)
     : initialized_(false),
@@ -45,8 +82,9 @@
   CHECK(!path.empty());
 
   // Put |kKnownFileSystems| in std::set to get O(log N) access time.
-  for (size_t i = 0; i < arraysize(kKnownFileSystems); ++i)
+  for (size_t i = 0; i < arraysize(kKnownFileSystems); ++i) {
     known_file_systems_.insert(kKnownFileSystems[i]);
+  }
 }
 
 MediaDeviceNotificationsLinux::~MediaDeviceNotificationsLinux() {
@@ -60,6 +98,7 @@
 
 void MediaDeviceNotificationsLinux::OnFilePathChanged(const FilePath& path) {
   if (path != mtab_path_) {
+    // This cannot happen unless FileWatcher is buggy.

[ajl, 2012/04/03 23:35:57]

Thanks for adding a comment here.  I think it would be useful to indicate what
exactly NOTREACHED does as well as why you call it.

[Lei Zhang, 2012/04/04 05:31:29]

Done.

     NOTREACHED();
     return;
   }
@@ -91,10 +130,10 @@
   // These mount points must have been removed in the new mtab.
   MountMap::iterator it = mtab_.begin();
   while (it != mtab_.end()) {
-    const MountPoint& mount_point(it->first);
+    const std::string& mount_point = it->first;
     // |mount_point| not in |new_mtab|.
     if (new_mtab.find(mount_point) == new_mtab.end()) {
-      const SystemMonitor::DeviceIdType& device_id(it->second.second);
+      const SystemMonitor::DeviceIdType& device_id = it->second.second;
       RemoveOldDevice(device_id);
       mtab_.erase(it++);

[Lei Zhang, 2012/04/04 05:31:29]

Ok, I'll put all the erase() calls in a separate loop below.

       continue;
@@ -107,22 +146,22 @@
   for (MountMap::iterator newiter = new_mtab.begin();
        newiter != new_mtab.end();
        ++newiter) {
-    const MountPoint& mount_point(newiter->first);
-    const MountDeviceAndId& mount_device_and_id(newiter->second);
-    const MountDevice& mount_device(newiter->second.first);
-    SystemMonitor::DeviceIdType& id(newiter->second.second);
+    const std::string& mount_point = newiter->first;
+    const MountDeviceAndId& mount_device_and_id = newiter->second;
+    const std::string& mount_device = mount_device_and_id.first;
+    SystemMonitor::DeviceIdType& id = newiter->second.second;
     MountMap::iterator olditer = mtab_.find(mount_point);
     // Check to see if it is a new mount point.
     if (olditer == mtab_.end()) {
       if (IsMediaDevice(mount_point)) {
         AddNewDevice(mount_device, mount_point, &id);
-        mtab_[mount_point] = mount_device_and_id;
+        mtab_.insert(std::make_pair(mount_point, mount_device_and_id));
       }
       continue;
     }
 
     // Existing mount point. Check to see if a new device is mounted there.
-    MountDeviceAndId& old_mount_device_and_id(olditer->second);
+    MountDeviceAndId& old_mount_device_and_id = olditer->second;
     if (mount_device == old_mount_device_and_id.first)
       continue;
 
@@ -141,11 +180,11 @@
     return;
 
   MountMap& new_mtab = *mtab;
-  struct mntent entry;
+  mntent entry;
   char buf[512];
   SystemMonitor::DeviceIdType mount_position = 0;
-  typedef std::pair<MountPoint, SystemMonitor::DeviceIdType> MountPointAndId;
-  typedef std::map<MountDevice, MountPointAndId> DeviceMap;
+  typedef std::pair<std::string, SystemMonitor::DeviceIdType> MountPointAndId;
+  typedef std::map<std::string, MountPointAndId> DeviceMap;
   DeviceMap device_map;
   while (getmntent_r(fp, &entry, buf, sizeof(buf))) {
     // We only care about real file systems.
@@ -162,20 +201,21 @@
   for (DeviceMap::iterator device_it = device_map.begin();
        device_it != device_map.end();
        ++device_it) {
-    const MountDevice& device = device_it->first;
-    const MountPoint& mount_point = device_it->second.first;
+    const std::string& device = device_it->first;
+    const std::string& mount_point = device_it->second.first;
     const SystemMonitor::DeviceIdType& position = device_it->second.second;
 
     // No device at |mount_point|, save |device| to it.
     MountMap::iterator mount_it = new_mtab.find(mount_point);
     if (mount_it == new_mtab.end()) {
-      new_mtab[mount_point] = std::make_pair(device, position);
+      new_mtab.insert(std::make_pair(mount_point,
+                                     std::make_pair(device, position)));
       continue;
     }
 
     // There is already a device mounted at |mount_point|. Check to see if
     // the existing mount entry is newer than the current one.
-    MountDevice& existing_device = mount_it->second.first;
+    std::string& existing_device = mount_it->second.first;
     SystemMonitor::DeviceIdType& existing_position = mount_it->second.second;
     if (existing_position > position)
       continue;
@@ -187,9 +227,9 @@
 }
 
 bool MediaDeviceNotificationsLinux::IsMediaDevice(
-    const MountPoint& mount_point) {
+    const std::string& mount_point) {
   FilePath dcim_path(mount_point);
-  FilePath::StringType dcim_dir(kDCIMDirName);
+  FilePath::StringType dcim_dir = kDCIMDirName;
   if (!file_util::DirectoryExists(dcim_path.Append(dcim_dir))) {
     // Check for lowercase 'dcim' as well.
     FilePath dcim_path_lower(dcim_path.Append(StringToLowerASCII(dcim_dir)));
@@ -201,8 +241,8 @@
 }
 
 void MediaDeviceNotificationsLinux::AddNewDevice(
-    const MountDevice& mount_device,
-    const MountPoint& mount_point,
+    const std::string& mount_device,
+    const std::string& mount_point,
     base::SystemMonitor::DeviceIdType* device_id) {
   *device_id = current_device_id_++;
   base::SystemMonitor* system_monitor = base::SystemMonitor::Get();
@@ -217,17 +257,4 @@
   system_monitor->ProcessMediaDeviceDetached(device_id);
 }
 
-MediaDeviceNotificationsLinux::WatcherDelegate::WatcherDelegate(
-    MediaDeviceNotificationsLinux* notifier)
-    : notifier_(notifier) {
-}
-
-MediaDeviceNotificationsLinux::WatcherDelegate::~WatcherDelegate() {
-}
-
-void MediaDeviceNotificationsLinux::WatcherDelegate::OnFilePathChanged(
-    const FilePath& path) {
-  notifier_->OnFilePathChanged(path);
-}
-
 }  // namespace content