Cleanup in DirectoryWatcher:

- share more code between different platforms
- put more code inside anonymous namespace
- add more DISALLOW_COPY_AND_ASSIGNs
- small #include cleanup

Review URL: http://codereview.chromium.org/42343

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@12007 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 63 insertions, 68 deletions

diff --git a/base/directory_watcher.h b/base/directory_watcher.h
index d6fc534..12040a6 100644
--- a/base/directory_watcher.h
+++ b/base/directory_watcher.h
@@ -7,7 +7,6 @@
 #ifndef BASE_DIRECTORY_WATCHER_H_
 #define BASE_DIRECTORY_WATCHER_H_
 
-#include <string>
 #include "base/basictypes.h"
 #include "base/ref_counted.h"
 
@@ -24,18 +23,26 @@ class DirectoryWatcher {
   };
 
   DirectoryWatcher();
-  ~DirectoryWatcher();
+  ~DirectoryWatcher() {}
 
   // Register interest in any changes in the directory |path|.
   // OnDirectoryChanged will be called back for each change within the dir.
   // If |recursive| is true, the delegate will be notified for each change
   // within the directory tree starting at |path|. Returns false on error.
-  bool Watch(const FilePath& path, Delegate* delegate, bool recursive);
+  bool Watch(const FilePath& path, Delegate* delegate, bool recursive) {
+    return impl_->Watch(path, delegate, recursive);
+  }
+
+  // Used internally to encapsulate different members on different platforms.
+  class PlatformDelegate : public base::RefCounted<PlatformDelegate> {
+   public:
+    virtual ~PlatformDelegate() {}
+    virtual bool Watch(const FilePath& path, Delegate* delegate,
+                       bool recursive) = 0;
+  };
 
  private:
-  class Impl;
-  friend class Impl;
-  scoped_refptr<Impl> impl_;
+  scoped_refptr<PlatformDelegate> impl_;
 
   DISALLOW_COPY_AND_ASSIGN(DirectoryWatcher);
 };
diff --git a/base/directory_watcher_inotify.cc b/base/directory_watcher_inotify.cc
index 7e906fa..c365352 100644
--- a/base/directory_watcher_inotify.cc
+++ b/base/directory_watcher_inotify.cc
@@ -167,6 +167,8 @@ class InotifyReaderNotifyTask : public Task {
  private:
   DirectoryWatcher::Delegate* delegate_;
   FilePath path_;
+
+  DISALLOW_COPY_AND_ASSIGN(InotifyReaderNotifyTask);
 };
 
 InotifyReader::InotifyReader()
@@ -264,21 +266,13 @@ void InotifyReader::OnInotifyEvent(inotify_event* event) {
   }
 }
 
-}  // namespace
-
-// Private implementation class implementing the behavior of DirectoryWatcher.
-class DirectoryWatcher::Impl : public base::RefCounted<DirectoryWatcher::Impl> {
+class DirectoryWatcherImpl : public DirectoryWatcher::PlatformDelegate {
  public:
-  Impl(DirectoryWatcher::Delegate* delegate)
-      : delegate_(delegate),
-        watch_(InotifyReader::kInvalidWatch) {
-  }
-
-  ~Impl();
+  DirectoryWatcherImpl() : watch_(InotifyReader::kInvalidWatch) {}
+  ~DirectoryWatcherImpl();
 
-  // Register interest in any changes in |path|.
-  // Returns false on error.
-  bool Watch(const FilePath& path);
+  virtual bool Watch(const FilePath& path, DirectoryWatcher::Delegate* delegate,
+                     bool recursive);
 
  private:
   // Delegate to notify upon changes.
@@ -289,31 +283,19 @@ class DirectoryWatcher::Impl : public base::RefCounted<DirectoryWatcher::Impl> {
 
   // Watch returned by InotifyReader.
   InotifyReader::Watch watch_;
+
+  DISALLOW_COPY_AND_ASSIGN(DirectoryWatcherImpl);
 };
 
-DirectoryWatcher::Impl::~Impl() {
+DirectoryWatcherImpl::~DirectoryWatcherImpl() {
   if (watch_ != InotifyReader::kInvalidWatch)
     Singleton<InotifyReader>::get()->RemoveWatch(watch_, delegate_);
 }
 
-bool DirectoryWatcher::Impl::Watch(const FilePath& path) {
+bool DirectoryWatcherImpl::Watch(const FilePath& path,
+    DirectoryWatcher::Delegate* delegate, bool recursive) {
   DCHECK(watch_ == InotifyReader::kInvalidWatch);  // Can only watch one path.
 
-  path_ = path;
-  watch_ = Singleton<InotifyReader>::get()->AddWatch(path, delegate_);
-
-  return watch_ != InotifyReader::kInvalidWatch;
-}
-
-DirectoryWatcher::DirectoryWatcher() {
-}
-
-DirectoryWatcher::~DirectoryWatcher() {
-  // Declared in .cc file for access to ~DirectoryWatcher::Impl.
-}
-
-bool DirectoryWatcher::Watch(const FilePath& path,
-                             Delegate* delegate, bool recursive) {
   if (recursive) {
     // TODO(phajdan.jr): Support recursive watches.
     // Unfortunately inotify has no "native" support for them, but it can be
@@ -324,6 +306,16 @@ bool DirectoryWatcher::Watch(const FilePath& path,
     NOTIMPLEMENTED();
     return false;
   }
-  impl_ = new DirectoryWatcher::Impl(delegate);
-  return impl_->Watch(path);
+
+  delegate_ = delegate;
+  path_ = path;
+  watch_ = Singleton<InotifyReader>::get()->AddWatch(path, delegate_);
+
+  return watch_ != InotifyReader::kInvalidWatch;
+}
+
+}  // namespace
+
+DirectoryWatcher::DirectoryWatcher() {
+  impl_ = new DirectoryWatcherImpl();
 }
diff --git a/base/directory_watcher_win.cc b/base/directory_watcher_win.cc
index 21fdd38..a1e3ff7 100644
--- a/base/directory_watcher_win.cc
+++ b/base/directory_watcher_win.cc
@@ -7,18 +7,18 @@
 #include "base/file_path.h"
 #include "base/logging.h"
 #include "base/object_watcher.h"
+#include "base/ref_counted.h"
 
-// Private implementation class implementing the behavior of DirectoryWatcher.
-class DirectoryWatcher::Impl : public base::RefCounted<DirectoryWatcher::Impl>,
-                               public base::ObjectWatcher::Delegate {
+namespace {
+
+class DirectoryWatcherImpl : public DirectoryWatcher::PlatformDelegate,
+                             public base::ObjectWatcher::Delegate {
  public:
-  Impl(DirectoryWatcher::Delegate* delegate)
-      : delegate_(delegate), handle_(INVALID_HANDLE_VALUE) {}
-  ~Impl();
+  DirectoryWatcherImpl() : handle_(INVALID_HANDLE_VALUE) {}
+  virtual ~DirectoryWatcherImpl();
 
-  // Register interest in any changes in |path|.
-  // Returns false on error.
-  bool Watch(const FilePath& path);
+  virtual bool Watch(const FilePath& path, DirectoryWatcher::Delegate* delegate,
+                     bool recursive);
 
   // Callback from MessageLoopForIO.
   virtual void OnObjectSignaled(HANDLE object);
@@ -32,20 +32,27 @@ class DirectoryWatcher::Impl : public base::RefCounted<DirectoryWatcher::Impl>,
   HANDLE handle_;
   // ObjectWatcher to watch handle_ for events.
   base::ObjectWatcher watcher_;
+
+  DISALLOW_COPY_AND_ASSIGN(DirectoryWatcherImpl);
 };
 
-DirectoryWatcher::Impl::~Impl() {
+DirectoryWatcherImpl::~DirectoryWatcherImpl() {
   if (handle_ != INVALID_HANDLE_VALUE) {
     watcher_.StopWatching();
     FindCloseChangeNotification(handle_);
   }
 }
 
-bool DirectoryWatcher::Impl::Watch(const FilePath& path) {
+bool DirectoryWatcherImpl::Watch(const FilePath& path,
+    DirectoryWatcher::Delegate* delegate, bool recursive) {
   DCHECK(path_.value().empty());  // Can only watch one path.
 
-  // NOTE: If you want to change this code to *not* watch subdirectories, have a
-  // look at http://code.google.com/p/chromium/issues/detail?id=5072 first.
+  if (!recursive) {
+    // See http://crbug.com/5072.
+    NOTIMPLEMENTED();
+    return false;
+  }
+
   handle_ = FindFirstChangeNotification(
       path.value().c_str(),
       TRUE,  // Watch subtree.
@@ -54,16 +61,17 @@ bool DirectoryWatcher::Impl::Watch(const FilePath& path) {
   if (handle_ == INVALID_HANDLE_VALUE)
     return false;
 
+  delegate_ = delegate;
   path_ = path;
   watcher_.StartWatching(handle_, this);
 
   return true;
 }
 
-void DirectoryWatcher::Impl::OnObjectSignaled(HANDLE object) {
+void DirectoryWatcherImpl::OnObjectSignaled(HANDLE object) {
   DCHECK(object == handle_);
   // Make sure we stay alive through the body of this function.
-  scoped_refptr<DirectoryWatcher::Impl> keep_alive(this);
+  scoped_refptr<DirectoryWatcherImpl> keep_alive(this);
 
   delegate_->OnDirectoryChanged(path_);
 
@@ -73,20 +81,8 @@ void DirectoryWatcher::Impl::OnObjectSignaled(HANDLE object) {
   watcher_.StartWatching(object, this);
 }
 
-DirectoryWatcher::DirectoryWatcher() {
-}
-
-DirectoryWatcher::~DirectoryWatcher() {
-  // Declared in .cc file for access to ~DirectoryWatcher::Impl.
-}
+}  // namespace
 
-bool DirectoryWatcher::Watch(const FilePath& path,
-                             Delegate* delegate, bool recursive) {
-  if (!recursive) {
-    // See http://crbug.com/5072.
-    NOTIMPLEMENTED();
-    return false;
-  }
-  impl_ = new DirectoryWatcher::Impl(delegate);
-  return impl_->Watch(path);
+DirectoryWatcher::DirectoryWatcher() {
+  impl_ = new DirectoryWatcherImpl();
 }