C++ Readability Review for thestig.

content/browser/media_device_notifications_linux.h

 // 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.

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

The style guide requires a descriptive file-level comment after the copyright
notice:

http://www.corp.google.com/eng/doc/cppguide.xml?showone=File_Comments#File_Co...

Clearly for Chrome code, because of the different licensing, you can't follow
the standard google3 file comment format exactly, but it seems like you ought to
still be able to include a file description.

Same in all other files.

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

We don't identify the original author in Chromium, but I will add a description
here. Though Chromium code almost never do this for .cc files, so I won't
either. I'm not sure what I'd put there any way. "MediaDeviceNotificationsLinux
implementation" and "MediaDeviceNotificationsLinux unit test" ?

 
 #ifndef CONTENT_BROWSER_MEDIA_DEVICE_NOTIFICATIONS_LINUX_H_
 #define CONTENT_BROWSER_MEDIA_DEVICE_NOTIFICATIONS_LINUX_H_
 #pragma once
 
 #include <map>
 #include <set>
 #include <string>
 #include <utility>
 
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
 #include "base/file_path.h"
 #include "base/files/file_path_watcher.h"
 #include "base/memory/ref_counted.h"
 #include "base/system_monitor/system_monitor.h"
 #include "content/common/content_export.h"

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

Are all of these #includes actually necessary in this immediate file?  The style
guide prefers avoiding unnecessary includes:

http://www.corp.google.com/eng/doc/cppguide.xml?showone=File_Comments#File_Co...

And encourages the use of forward declarations instead of includes where
possible.

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

Yes, except for FilePath, which I forward declared.

 
 namespace content {
 
 class CONTENT_EXPORT MediaDeviceNotificationsLinux
     : public base::RefCountedThreadSafe<MediaDeviceNotificationsLinux> {
  public:
   explicit MediaDeviceNotificationsLinux(const FilePath& path);
 
   // Must be called for MediaDeviceNotificationsLinux to work.
   void Init();
 
   void OnFilePathChanged(const FilePath& path);
 
  private:
   friend class base::RefCountedThreadSafe<MediaDeviceNotificationsLinux>;
 
   typedef std::string MountDevice;

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

The use of a typedef here strikes me as superfluous and actually somewhat
harmful for readability.

These typedefs are useful when the full type name is awkwardly long or
complicated, or when the typedef name gives some useful information about the
use of the type that the full name doesn't include.  On the other hand, though,
by hiding the actual type, they create some additional burden on readers since
readers must consult the typedef definition to understand the actual type and
how it can be used, while if the real time is a well understood standard type if
there was no typedef they might not need to consult any external resources.

In this case, I don't think the typedef makes the name usefully shorter (the
full name is fairly short), and I don't think it adds any useful information
either (a good variable name should make clear that string value contains a
mount device).  It does, however, hide some important details that are useful to
readers - namely the actual type of the object (string).

Based on that, the typedef here seems like a net negative and I would prefer if
you removed it.  Same with MountPoint also, and in similar cases.

I think MountDeviceAndId shortens the name enough that it seems good to keep,
however, and while MountMap is kind of borderline I am ok with keeping it also.

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

Done.

   typedef std::string MountPoint;
   typedef std::pair<MountDevice,
                     base::SystemMonitor::DeviceIdType> MountDeviceAndId;
   typedef std::map<MountPoint, MountDeviceAndId> MountMap;
 
   // A simple pass-through class. MediaDeviceNotificationsLinux cannot directly
   // inherit from FilePathWatcher::Delegate due to multiple inheritance.
   class WatcherDelegate : public base::files::FilePathWatcher::Delegate {

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

Do you actually need the full definition of this class in the header file? 
Since it is private it can't be used by external users and the only reference to
it in this file is in the declaration of a scoped_refptr.

If it is like the standard google3 smart pointer classes, you really only need
the type declaration for the scoped_refptr.

In that case, you could move the definition of the class to your .cc file and
only have a forward declaration in this header file.  That would improve
readability of the header by making it shorter and reducing the amount of
extraneous (to external users) information, and also follow the general
principle of minimizing the extent and scope of all definitions.

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

Done.

    public:
     explicit WatcherDelegate(MediaDeviceNotificationsLinux* notifier);
 
     // base::files::FilePathWatcher::Delegate implementation.
     virtual void OnFilePathChanged(const FilePath& path) OVERRIDE;
 
    private:
     friend class base::RefCountedThreadSafe<WatcherDelegate>;
 
     virtual ~WatcherDelegate();
     MediaDeviceNotificationsLinux* notifier_;

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

Is this owned or not?  If owned, when is it deleted?  If not owned, who does own
it?

There should be a comment answering those questions here.

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

Done.

 
     DISALLOW_COPY_AND_ASSIGN(WatcherDelegate);
   };
 
   virtual ~MediaDeviceNotificationsLinux();

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

For me, it would have been useful to have a comment with these explaining why
the destructor is private (so it can only be deleted by the reference-counting
smart pointer I guess?).

Maybe this is completely obvious to everyone in chrome, but if there is any
possibility of confusion a comment seems good.

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

Done. Ya, it seems everyone just does this without a comment.

 
   void InitOnFileThread();
 
   // Parse the mtab file and find all changes.
   void UpdateMtab();
 
   // Read the mtab file entries into |mtab|.
   void ReadMtab(MountMap* mtab);
 
   // For a new device mounted at |mount_point|, see if it is a media device by
@@ skipping 32 matching lines @@
 
   // Set of known file systems that we care about.
   std::set<std::string> known_file_systems_;
 
   DISALLOW_COPY_AND_ASSIGN(MediaDeviceNotificationsLinux);
 };
 
 }  // namespace content
 
 #endif  // CONTENT_BROWSER_MEDIA_DEVICE_NOTIFICATIONS_LINUX_H_
+