C++ Readability Review for thestig.

content/browser/media_device_notifications_linux.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/media_device_notifications_linux.h"
 
 #include <mntent.h>
 #include <stdio.h>
 
 #include "base/bind.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;

[ajl, 2012/03/23 22:47:39]

If you don't use this outside your project namespace (content) it would be kind
of nice to move this inside the namespace.

It would reduce its scope a little bit, and otherwise be completely harmless.

[Lei Zhang, 2012/03/24 03:01:26]

Done.

 
 namespace {
 
 const char* const kDCIMDirName = "DCIM";

[ajl, 2012/03/23 22:47:39]

There is a preference in google code to declare C string constants as char[]
rather than char*.  It is slightly safer and more efficient, since the array
can't possibly be reassigned to point to something else and the offset is
completely known at compile time.

Same elsewhere.

[Lei Zhang, 2012/03/24 03:01:26]

Done.

[vandebo (ex-Chrome), 2012/04/11 21:45:16]

ajl: For my benefit can you explain the decision here more?  Various places on
the internet claim that char foo[] = "constant" copies the string to the stack. 
That seems less efficient.

 
 // List of file systems we care about.
 const char* const kKnownFileSystems[] = {
   "ext2",
   "ext3",
   "ext4",
   "fat",
   "hfsplus",
   "iso9660",
   "msdos",
   "ntfs",
   "udf",
   "vfat",
 };
 
 }  // namespace
 
 namespace content {
 
 MediaDeviceNotificationsLinux::MediaDeviceNotificationsLinux(
     const FilePath& path)
     : initialized_(false),
       mtab_path_(path),
       current_device_id_(0U) {
   CHECK(!path.empty());
 
   // Put |kKnownFileSystems| in std::set to get O(log N) access time.
   for (size_t i = 0; i < arraysize(kKnownFileSystems); ++i)
     known_file_systems_.insert(kKnownFileSystems[i]);

[ajl, 2012/03/23 22:47:39]

For this particular case, I think it would help readability to put braces around
the for statement body even though it is a one liner, since you are putting the
body on a separate line from the start, and there is a closing brace after it
for an outer scope. Having braces around the inner scope makes it absolutely
clear which braces go with which statement.

Same in similar cases elsewhere (applies to if, while, anonymous nested scopes,
etc. also).

[Lei Zhang, 2012/03/24 03:01:26]

Done.

 }
 
 MediaDeviceNotificationsLinux::~MediaDeviceNotificationsLinux() {
 }
 
 void MediaDeviceNotificationsLinux::Init() {
   BrowserThread::PostTask(
       BrowserThread::FILE, FROM_HERE,
       base::Bind(&MediaDeviceNotificationsLinux::InitOnFileThread, this));
 }
 
 void MediaDeviceNotificationsLinux::OnFilePathChanged(const FilePath& path) {
   if (path != mtab_path_) {
     NOTREACHED();

[ajl, 2012/03/23 22:47:39]

How about a comment explaining what this means?

[Lei Zhang, 2012/03/24 03:01:26]

Done.

     return;
   }
 
   UpdateMtab();
 }
 
 void MediaDeviceNotificationsLinux::InitOnFileThread() {
   DCHECK(!initialized_);
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
   initialized_ = true;
 
   watcher_delegate_ = new WatcherDelegate(this);

[ajl, 2012/03/23 22:47:39]

Is there a method call (like scoped_ptr::reset()) you could use for this,
instead of the overloaded assignment operator?

That would make it much more obvious in the local context that watcher_delegate_
is not a raw pointer (and hence the reader does not need to worry about when
this is deleted).

[Lei Zhang, 2012/03/24 03:01:26]

Unfortunately no. See
http://src.chromium.org/viewvc/chrome/trunk/src/base/memory/ref_counted.h?rev...,
but I understand your concern. I've seen it elsewhere and went "err, where is
Foo getting deleted?"

   if (!file_watcher_.Watch(mtab_path_, watcher_delegate_)) {
     LOG(ERROR) << "Adding watch for " << mtab_path_.value() << " failed";
     return;
   }
 
   UpdateMtab();
 }
 
 void MediaDeviceNotificationsLinux::UpdateMtab() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
 
   MountMap new_mtab;
   ReadMtab(&new_mtab);
 
   // Check existing mtab entries for unaccounted mount points.
   // 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);
     // |mount_point| not in |new_mtab|.
     if (new_mtab.find(mount_point) == new_mtab.end()) {
       const SystemMonitor::DeviceIdType& device_id(it->second.second);
       RemoveOldDevice(device_id);
       mtab_.erase(it++);
       continue;

[ajl, 2012/03/23 22:47:39]

Do I understand correctly that the only thing this actually skips is the ++it on
line 103?  So it would be equivalent just to remove ++ on the previous line and
remove this?

If so, it seems good to do that, since it would be simpler.

[Lei Zhang, 2012/03/24 03:01:26]

The code is a bit subtle. When I call map::erase(), the current iterator is no
longer valid. i.e.

it = map.begin();
map.erase(it);
++it;

does not work.

What I'm actually doing is:


it = map.begin();
it2 = it;
++it2;
map.erase(it);
it = it2;

but just not as verbose.

     }
     // Existing mount point. Ignore and deal in the next loop.
     ++it;
   }
 
   // Check new mtab entries against existing ones.
   for (MountMap::iterator newiter = new_mtab.begin();
        newiter != new_mtab.end();
        ++newiter) {
     const MountPoint& mount_point(newiter->first);

[ajl, 2012/03/23 22:47:39]

It is true that the style guide allows using either constructor or
assignment-style notation for assigning pointers, references, and primitive
values, but there is a pretty overwhelming preference in google code for using
the assignment style instead.

In addition to following convention, another good reason for this style is that
it clearly distinguishes in notation between simple primitive assignment and
instantiating class-type objects by "calling" a constructor.

So, I would recommend changing to using assignment notation consistently for
initializing references in this code.

[Lei Zhang, 2012/03/24 03:01:26]

Done.

     const MountDeviceAndId& mount_device_and_id(newiter->second);
     const MountDevice& mount_device(newiter->second.first);

[ajl, 2012/03/23 22:47:39]

Why not use the temporary you created on the previous line here?  Same below.

[Lei Zhang, 2012/03/24 03:01:26]

Sure, but not for the next one. It's not const and I actually want to write to
it.

     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;

[ajl, 2012/03/23 22:47:39]

Can you use insert() here rather than the [] operator?  There is a preference in
google code against using [] for maps when possible (see comment near the top of
util/gtl/map-util.h for an explanation).

If you don't have access to map-util.h in chrome, there are some cases where
using [] might be ok where it would not be in google3 code. It seems like since
this is just inserting, and not relying on default initialization behavior or
overwriting existing values, that insert() should work pretty easily here, so I
think you should change it.

Same in similar cases elsewhere.

[Lei Zhang, 2012/03/24 03:01:26]

Done.

       }
       continue;
     }
 
     // Existing mount point. Check to see if a new device is mounted there.
     MountDeviceAndId& old_mount_device_and_id(olditer->second);
     if (mount_device == old_mount_device_and_id.first)
       continue;
 
     // New device mounted.
@@ skipping 94 matching lines @@
 
 MediaDeviceNotificationsLinux::WatcherDelegate::~WatcherDelegate() {
 }
 
 void MediaDeviceNotificationsLinux::WatcherDelegate::OnFilePathChanged(
     const FilePath& path) {
   notifier_->OnFilePathChanged(path);
 }
 
 }  // namespace content
+