Change FilePathWatcher tests to use the new callback style FilePathWatcher::Watch().

base/files/file_path_watcher_browsertest.cc

Index: base/files/file_path_watcher_browsertest.cc
===================================================================
--- base/files/file_path_watcher_browsertest.cc	(revision 169722)
+++ base/files/file_path_watcher_browsertest.cc	(working copy)
@@ -4,8 +4,6 @@
 
 #include "base/files/file_path_watcher.h"
 
-#include <set>
-
 #if defined(OS_WIN)
 #include <windows.h>
 #include <aclapi.h>
@@ -13,6 +11,8 @@
 #include <sys/stat.h>
 #endif
 
+#include <set>
+
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
@@ -38,6 +38,9 @@
 
 class TestDelegate;
 
+const char kTestContent[] = "content";
+const char kNewTestContent[] = "new content";
+
 // Aggregates notifications from the test delegates and breaks the message loop
 // the test thread is waiting on once they all came in.
 class NotificationCollector
@@ -50,7 +53,7 @@
   void OnChange(TestDelegate* delegate) {
     loop_->PostTask(FROM_HERE,
                     base::Bind(&NotificationCollector::RecordChange, this,
-                               make_scoped_refptr(delegate)));
+                               base::Unretained(delegate)));
   }
 
   void Register(TestDelegate* delegate) {
@@ -70,6 +73,7 @@
   ~NotificationCollector() {}
 
   void RecordChange(TestDelegate* delegate) {
+    // Warning: |delegate| is Unretained. Do not dereference.
     ASSERT_TRUE(loop_->BelongsToCurrentThread());
     ASSERT_TRUE(delegates_.count(delegate));
     signaled_.insert(delegate);
@@ -89,46 +93,80 @@
   scoped_refptr<base::MessageLoopProxy> loop_;
 };
 
-// A mock FilePathWatcher::Delegate for testing. I'd rather use gmock, but it's
+// A mock class for testing. I'd rather use gmock, but it's
 // not thread safe for setting expectations, so the test code couldn't safely
 // reset expectations while the file watcher is running. In order to allow this,
 // we keep simple thread safe status flags in TestDelegate.
-class TestDelegate : public FilePathWatcher::Delegate {
+class TestDelegate : public SupportsWeakPtr<TestDelegate> {

[kmadhusu, 2012/11/28 20:13:06]

Do we need to update the comment?

Can we rename TestDelegate => TestFileChangeEvents or something similar?

[Lei Zhang, 2012/11/28 23:27:19]

Done.
I don't think anything has really changed in this CL. TestDelegate is still the
class that gets delegated the job of handling the callbacks from
FilePathWatcher.

  public:
   // The message loop specified by |loop| will be quit if a notification is
   // received while the delegate is |armed_|. Note that the testing code must
-  // guarantee |loop| outlives the file thread on which OnFilePathChanged runs.
+  // guarantee |loop| outlives the file thread on which OnFileChanged runs.

[kmadhusu, 2012/11/28 20:13:06]

Do we need to update this comment?

[Lei Zhang, 2012/11/28 23:27:19]

The comment is obsolete.

   explicit TestDelegate(NotificationCollector* collector)
       : collector_(collector) {
     collector_->Register(this);
   }
+  ~TestDelegate() {}
 
-  virtual void OnFilePathChanged(const FilePath&) {
-    collector_->OnChange(this);
+  void OnFileChanged(const FilePath& path, bool error) {
+    if (error)
+      ADD_FAILURE() << "Error " << path.value();
+    else
+      collector_->OnChange(this);
   }
 
-  virtual void OnFilePathError(const FilePath& path) {
-    ADD_FAILURE() << "Error " << path.value();
+ private:
+  scoped_refptr<NotificationCollector> collector_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestDelegate);
+};
+
+// Used by the DeleteDuringNotify test below.
+// Deletes the FilePathWatcher when it's notified.
+class Deleter : public SupportsWeakPtr<Deleter> {
+ public:
+  Deleter(FilePathWatcher* watcher, MessageLoop* loop)
+      : watcher_(watcher),
+        loop_(loop) {
   }
+  ~Deleter() {}
 
- protected:
-  virtual ~TestDelegate() {}
+  void OnFileChanged(const FilePath&, bool) {
+    watcher_.reset();
+    loop_->PostTask(FROM_HERE, MessageLoop::QuitClosure());
+  }
 
+  FilePathWatcher* watcher() const { return watcher_.get(); }
+
  private:
-  scoped_refptr<NotificationCollector> collector_;
+  scoped_ptr<FilePathWatcher> watcher_;
+  MessageLoop* loop_;
 
-  DISALLOW_COPY_AND_ASSIGN(TestDelegate);
+  DISALLOW_COPY_AND_ASSIGN(Deleter);
 };
 
-void SetupWatchDelegate(const FilePath& target,
-                        FilePathWatcher* watcher,
-                        FilePathWatcher::Delegate* delegate,
-                        bool* result,
-                        base::WaitableEvent* completion) {
-  *result = watcher->Watch(target, delegate);
+void SetupWatchCallbackForTestDelegate(const FilePath& target,

[kmadhusu, 2012/11/28 20:19:44]

SetupWatchCallbackForTestDelegate and SetupWatchCallbackForDeleter looks very
similar expect for the third input param. Can we use function templates?
Thereby, we can avoid SetupWatchForTestDelegate and SetupWatchForTestDeleter.

[Lei Zhang, 2012/11/28 23:27:19]

Done.

+                                       FilePathWatcher* watcher,
+                                       TestDelegate* delegate,
+                                       bool* result,
+                                       base::WaitableEvent* completion) {
+  *result = watcher->Watch(target,
+                           base::Bind(&TestDelegate::OnFileChanged,
+                                      delegate->AsWeakPtr()));
   completion->Signal();
 }
 
+void SetupWatchCallbackForDeleter(const FilePath& target,
+                                  FilePathWatcher* watcher,
+                                  Deleter* deleter,
+                                  bool* result,
+                                  base::WaitableEvent* completion) {

[kmadhusu, 2012/11/28 20:13:06]

nit: 
DCHECK(watcher)
DCHECK(deleter)
DCHECK(result)
DCHECK(completion)

[Lei Zhang, 2012/11/28 23:27:19]

Given how straight-forward this 4 line function is, we really don't need these
DCHECKs to document the fact that these pointers should be non-NULL. If it
crashes, the reason should be obvious.

We also shouldn't use them in tests. See the last bullet point in
http://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-

+  *result = watcher->Watch(target,
+                           base::Bind(&Deleter::OnFileChanged,
+                                      deleter->AsWeakPtr()));
+  completion->Signal();
+}
+
 void SetupWatchCallback(const FilePath& target,
                         FilePathWatcher* watcher,
                         const FilePathWatcher::Callback& callback) {
@@ -168,6 +206,10 @@
     loop_.RunUntilIdle();
   }
 
+  void DeleteDelegateOnFileThread(TestDelegate* delegate) {
+    file_thread_.message_loop_proxy()->DeleteSoon(FROM_HERE, delegate);
+  }
+
   FilePath test_file() {
     return temp_dir_.path().AppendASCII("FilePathWatcherTest");
   }
@@ -183,19 +225,32 @@
     return write_size == static_cast<int>(content.length());
   }
 
-  bool SetupWatch(const FilePath& target,
-                  FilePathWatcher* watcher,
-                  FilePathWatcher::Delegate* delegate) WARN_UNUSED_RESULT {
+  bool SetupWatchForTestDelegate(const FilePath& target,
+                                 FilePathWatcher* watcher,
+                                 TestDelegate* delegate) WARN_UNUSED_RESULT {

[kmadhusu, 2012/11/28 20:13:06]

WARN_UNUSED_RESULT is not required any more.

[Lei Zhang, 2012/11/28 23:27:19]

Why not? The caller should check and make sure SetupWatch succeeded.

     base::WaitableEvent completion(false, false);
     bool result;
     file_thread_.message_loop_proxy()->PostTask(
         FROM_HERE,
-        base::Bind(SetupWatchDelegate, target, watcher,
-                   make_scoped_refptr(delegate), &result, &completion));
+        base::Bind(SetupWatchCallbackForTestDelegate, target, watcher, delegate,
+                   &result, &completion));
     completion.Wait();
     return result;
   }
 
+  bool SetupWatchForDeleter(const FilePath& target,
+                            FilePathWatcher* watcher,
+                            Deleter* deleter) WARN_UNUSED_RESULT {

[kmadhusu, 2012/11/28 20:13:06]

WARN_UNUSED_RESULT is not required any more.

+    base::WaitableEvent completion(false, false);
+    bool result;
+    file_thread_.message_loop_proxy()->PostTask(
+        FROM_HERE,
+        base::Bind(SetupWatchCallbackForDeleter, target, watcher, deleter,
+                   &result, &completion));
+    completion.Wait();
+    return result;
+  }
+
   bool WaitForEvents() WARN_UNUSED_RESULT {
     collector_->Reset();
     loop_.Run();
@@ -208,55 +263,61 @@
   base::Thread file_thread_;
   ScopedTempDir temp_dir_;
   scoped_refptr<NotificationCollector> collector_;
+
+  DISALLOW_COPY_AND_ASSIGN(FilePathWatcherTest);
 };
 
 // Basic test: Create the file and verify that we notice.
 TEST_F(FilePathWatcherTest, NewFile) {
   FilePathWatcher watcher;
-  scoped_refptr<TestDelegate> delegate(new TestDelegate(collector()));
-  ASSERT_TRUE(SetupWatch(test_file(), &watcher, delegate.get()));
+  scoped_ptr<TestDelegate> delegate(new TestDelegate(collector()));
+  ASSERT_TRUE(SetupWatchForTestDelegate(test_file(), &watcher, delegate.get()));
 
-  ASSERT_TRUE(WriteFile(test_file(), "content"));
+  ASSERT_TRUE(WriteFile(test_file(), kTestContent));
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 // Verify that modifying the file is caught.
 TEST_F(FilePathWatcherTest, ModifiedFile) {
-  ASSERT_TRUE(WriteFile(test_file(), "content"));
+  ASSERT_TRUE(WriteFile(test_file(), kTestContent));
 
   FilePathWatcher watcher;
-  scoped_refptr<TestDelegate> delegate(new TestDelegate(collector()));
-  ASSERT_TRUE(SetupWatch(test_file(), &watcher, delegate.get()));
+  scoped_ptr<TestDelegate> delegate(new TestDelegate(collector()));
+  ASSERT_TRUE(SetupWatchForTestDelegate(test_file(), &watcher, delegate.get()));
 
   // Now make sure we get notified if the file is modified.
-  ASSERT_TRUE(WriteFile(test_file(), "new content"));
+  ASSERT_TRUE(WriteFile(test_file(), kNewTestContent));
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 // Verify that moving the file into place is caught.
 TEST_F(FilePathWatcherTest, MovedFile) {
   FilePath source_file(temp_dir_.path().AppendASCII("source"));
-  ASSERT_TRUE(WriteFile(source_file, "content"));
+  ASSERT_TRUE(WriteFile(source_file, kTestContent));
 
   FilePathWatcher watcher;
-  scoped_refptr<TestDelegate> delegate(new TestDelegate(collector()));
-  ASSERT_TRUE(SetupWatch(test_file(), &watcher, delegate.get()));
+  scoped_ptr<TestDelegate> delegate(new TestDelegate(collector()));
+  ASSERT_TRUE(SetupWatchForTestDelegate(test_file(), &watcher, delegate.get()));
 
   // Now make sure we get notified if the file is modified.
   ASSERT_TRUE(file_util::Move(source_file, test_file()));
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 TEST_F(FilePathWatcherTest, DeletedFile) {
-  ASSERT_TRUE(WriteFile(test_file(), "content"));
+  ASSERT_TRUE(WriteFile(test_file(), kTestContent));
 
   FilePathWatcher watcher;
-  scoped_refptr<TestDelegate> delegate(new TestDelegate(collector()));
-  ASSERT_TRUE(SetupWatch(test_file(), &watcher, delegate.get()));
+  scoped_ptr<TestDelegate> delegate(new TestDelegate(collector()));
+  ASSERT_TRUE(SetupWatchForTestDelegate(test_file(), &watcher, delegate.get()));
 
   // Now make sure we get notified if the file is deleted.
   file_util::Delete(test_file(), false);
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 TEST_F(FilePathWatcherTest, Callback) {
@@ -280,7 +341,7 @@
 
   // The watch has been installed. Trigger a file event now, which will unblock
   // the loop again.
-  ASSERT_TRUE(WriteFile(test_file(), "content"));
+  ASSERT_TRUE(WriteFile(test_file(), kTestContent));
   loop_.Run();
   EXPECT_TRUE(called_back);
 
@@ -299,67 +360,52 @@
   loop_.Run();
 }
 
-// Used by the DeleteDuringNotify test below.
-// Deletes the FilePathWatcher when it's notified.
-class Deleter : public FilePathWatcher::Delegate {
- public:
-  Deleter(FilePathWatcher* watcher, MessageLoop* loop)
-      : watcher_(watcher),
-        loop_(loop) {
-  }
-
-  virtual void OnFilePathChanged(const FilePath& path) {
-    watcher_.reset();
-    loop_->PostTask(FROM_HERE, MessageLoop::QuitClosure());
-  }
-
-  scoped_ptr<FilePathWatcher> watcher_;
-  MessageLoop* loop_;
-
- private:
-  virtual ~Deleter() {}
-};
-
 // Verify that deleting a watcher during the callback doesn't crash.
 TEST_F(FilePathWatcherTest, DeleteDuringNotify) {
   FilePathWatcher* watcher = new FilePathWatcher;
   // Takes ownership of watcher.
-  scoped_refptr<Deleter> deleter(new Deleter(watcher, &loop_));
-  ASSERT_TRUE(SetupWatch(test_file(), watcher, deleter.get()));
+  scoped_ptr<Deleter> deleter(new Deleter(watcher, &loop_));
+  ASSERT_TRUE(SetupWatchForDeleter(test_file(), watcher, deleter.get()));
 
-  ASSERT_TRUE(WriteFile(test_file(), "content"));
+  ASSERT_TRUE(WriteFile(test_file(), kTestContent));
   ASSERT_TRUE(WaitForEvents());
 
   // We win if we haven't crashed yet.
   // Might as well double-check it got deleted, too.
-  ASSERT_TRUE(deleter->watcher_.get() == NULL);
+  ASSERT_TRUE(deleter->watcher() == NULL);

[kmadhusu, 2012/11/28 20:13:06]

Do we need to Delete the |deleter| on file thread?

[Lei Zhang, 2012/11/28 23:27:19]

No, once the watcher is gone, the callback that contains the weak pointer is
gone too. Thus it is safe to delete the Deleter on any thread.

 }
 
 // Verify that deleting the watcher works even if there is a pending
 // notification.
 // Flaky on MacOS. http://crbug.com/85930
 #if defined(OS_MACOSX)
-#define MAYBE_DestroyWithPendingNotification DISABLED_DestroyWithPendingNotification
+#define MAYBE_DestroyWithPendingNotification \
+    DISABLED_DestroyWithPendingNotification
 #else
 #define MAYBE_DestroyWithPendingNotification DestroyWithPendingNotification
 #endif
 TEST_F(FilePathWatcherTest, MAYBE_DestroyWithPendingNotification) {
-  scoped_refptr<TestDelegate> delegate(new TestDelegate(collector()));
+  scoped_ptr<TestDelegate> delegate(new TestDelegate(collector()));
   FilePathWatcher* watcher = new FilePathWatcher;
-  ASSERT_TRUE(SetupWatch(test_file(), watcher, delegate.get()));
-  ASSERT_TRUE(WriteFile(test_file(), "content"));
+  ASSERT_TRUE(SetupWatchForTestDelegate(test_file(), watcher, delegate.get()));
+  ASSERT_TRUE(WriteFile(test_file(), kTestContent));
   file_thread_.message_loop_proxy()->DeleteSoon(FROM_HERE, watcher);
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 TEST_F(FilePathWatcherTest, MultipleWatchersSingleFile) {
   FilePathWatcher watcher1, watcher2;
-  scoped_refptr<TestDelegate> delegate1(new TestDelegate(collector()));
-  scoped_refptr<TestDelegate> delegate2(new TestDelegate(collector()));
-  ASSERT_TRUE(SetupWatch(test_file(), &watcher1, delegate1.get()));
-  ASSERT_TRUE(SetupWatch(test_file(), &watcher2, delegate2.get()));
+  scoped_ptr<TestDelegate> delegate1(new TestDelegate(collector()));
+  scoped_ptr<TestDelegate> delegate2(new TestDelegate(collector()));
+  ASSERT_TRUE(SetupWatchForTestDelegate(test_file(), &watcher1,
+                                        delegate1.get()));
+  ASSERT_TRUE(SetupWatchForTestDelegate(test_file(), &watcher2,
+                                        delegate2.get()));
 
-  ASSERT_TRUE(WriteFile(test_file(), "content"));
+  ASSERT_TRUE(WriteFile(test_file(), kTestContent));
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate1.release());
+  DeleteDelegateOnFileThread(delegate2.release());
 }
 
 // Verify that watching a file whose parent directory doesn't exist yet works if
@@ -368,12 +414,12 @@
   FilePathWatcher watcher;
   FilePath dir(temp_dir_.path().AppendASCII("dir"));
   FilePath file(dir.AppendASCII("file"));
-  scoped_refptr<TestDelegate> delegate(new TestDelegate(collector()));
-  ASSERT_TRUE(SetupWatch(file, &watcher, delegate.get()));
+  scoped_ptr<TestDelegate> delegate(new TestDelegate(collector()));
+  ASSERT_TRUE(SetupWatchForTestDelegate(file, &watcher, delegate.get()));
 
   ASSERT_TRUE(file_util::CreateDirectory(dir));
 
-  ASSERT_TRUE(WriteFile(file, "content"));
+  ASSERT_TRUE(WriteFile(file, kTestContent));
 
   VLOG(1) << "Waiting for file creation";
   ASSERT_TRUE(WaitForEvents());
@@ -385,6 +431,7 @@
   ASSERT_TRUE(file_util::Delete(file, false));
   VLOG(1) << "Waiting for file deletion";
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 // Exercises watch reconfiguration for the case that directories on the path
@@ -400,8 +447,8 @@
 
   FilePathWatcher watcher;
   FilePath file(path.AppendASCII("file"));
-  scoped_refptr<TestDelegate> delegate(new TestDelegate(collector()));
-  ASSERT_TRUE(SetupWatch(file, &watcher, delegate.get()));
+  scoped_ptr<TestDelegate> delegate(new TestDelegate(collector()));
+  ASSERT_TRUE(SetupWatchForTestDelegate(file, &watcher, delegate.get()));
 
   FilePath sub_path(temp_dir_.path());
   for (std::vector<std::string>::const_iterator d(dir_names.begin());
@@ -410,13 +457,14 @@
     ASSERT_TRUE(file_util::CreateDirectory(sub_path));
   }
   VLOG(1) << "Create File";
-  ASSERT_TRUE(WriteFile(file, "content"));
+  ASSERT_TRUE(WriteFile(file, kTestContent));
   VLOG(1) << "Waiting for file creation";
   ASSERT_TRUE(WaitForEvents());
 
   ASSERT_TRUE(WriteFile(file, "content v2"));
   VLOG(1) << "Waiting for file modification";
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 #if defined(OS_MACOSX)
@@ -428,28 +476,30 @@
   FilePath dir(temp_dir_.path().AppendASCII("dir"));
   FilePath file(dir.AppendASCII("file"));
   ASSERT_TRUE(file_util::CreateDirectory(dir));
-  ASSERT_TRUE(WriteFile(file, "content"));
-  scoped_refptr<TestDelegate> delegate(new TestDelegate(collector()));
-  ASSERT_TRUE(SetupWatch(file, &watcher, delegate.get()));
+  ASSERT_TRUE(WriteFile(file, kTestContent));
+  scoped_ptr<TestDelegate> delegate(new TestDelegate(collector()));
+  ASSERT_TRUE(SetupWatchForTestDelegate(file, &watcher, delegate.get()));
 
   ASSERT_TRUE(file_util::Delete(dir, true));
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 // Tests that a file that is deleted and reappears is tracked correctly.
 TEST_F(FilePathWatcherTest, DeleteAndRecreate) {
-  ASSERT_TRUE(WriteFile(test_file(), "content"));
+  ASSERT_TRUE(WriteFile(test_file(), kTestContent));
   FilePathWatcher watcher;
-  scoped_refptr<TestDelegate> delegate(new TestDelegate(collector()));
-  ASSERT_TRUE(SetupWatch(test_file(), &watcher, delegate.get()));
+  scoped_ptr<TestDelegate> delegate(new TestDelegate(collector()));
+  ASSERT_TRUE(SetupWatchForTestDelegate(test_file(), &watcher, delegate.get()));
 
   ASSERT_TRUE(file_util::Delete(test_file(), false));
   VLOG(1) << "Waiting for file deletion";
   ASSERT_TRUE(WaitForEvents());
 
-  ASSERT_TRUE(WriteFile(test_file(), "content"));
+  ASSERT_TRUE(WriteFile(test_file(), kTestContent));
   VLOG(1) << "Waiting for file creation";
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 TEST_F(FilePathWatcherTest, WatchDirectory) {
@@ -457,14 +507,14 @@
   FilePath dir(temp_dir_.path().AppendASCII("dir"));
   FilePath file1(dir.AppendASCII("file1"));
   FilePath file2(dir.AppendASCII("file2"));
-  scoped_refptr<TestDelegate> delegate(new TestDelegate(collector()));
-  ASSERT_TRUE(SetupWatch(dir, &watcher, delegate.get()));
+  scoped_ptr<TestDelegate> delegate(new TestDelegate(collector()));
+  ASSERT_TRUE(SetupWatchForTestDelegate(dir, &watcher, delegate.get()));
 
   ASSERT_TRUE(file_util::CreateDirectory(dir));
   VLOG(1) << "Waiting for directory creation";
   ASSERT_TRUE(WaitForEvents());
 
-  ASSERT_TRUE(WriteFile(file1, "content"));
+  ASSERT_TRUE(WriteFile(file1, kTestContent));
   VLOG(1) << "Waiting for file1 creation";
   ASSERT_TRUE(WaitForEvents());
 
@@ -479,9 +529,10 @@
   VLOG(1) << "Waiting for file1 deletion";
   ASSERT_TRUE(WaitForEvents());
 
-  ASSERT_TRUE(WriteFile(file2, "content"));
+  ASSERT_TRUE(WriteFile(file2, kTestContent));
   VLOG(1) << "Waiting for file2 creation";
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 TEST_F(FilePathWatcherTest, MoveParent) {
@@ -491,14 +542,16 @@
   FilePath dest(temp_dir_.path().AppendASCII("dest"));
   FilePath subdir(dir.AppendASCII("subdir"));
   FilePath file(subdir.AppendASCII("file"));
-  scoped_refptr<TestDelegate> file_delegate(new TestDelegate(collector()));
-  ASSERT_TRUE(SetupWatch(file, &file_watcher, file_delegate.get()));
-  scoped_refptr<TestDelegate> subdir_delegate(new TestDelegate(collector()));
-  ASSERT_TRUE(SetupWatch(subdir, &subdir_watcher, subdir_delegate.get()));
+  scoped_ptr<TestDelegate> file_delegate(new TestDelegate(collector()));
+  ASSERT_TRUE(SetupWatchForTestDelegate(file, &file_watcher,
+                                        file_delegate.get()));
+  scoped_ptr<TestDelegate> subdir_delegate(new TestDelegate(collector()));
+  ASSERT_TRUE(SetupWatchForTestDelegate(subdir, &subdir_watcher,
+                                        subdir_delegate.get()));
 
   // Setup a directory hierarchy.
   ASSERT_TRUE(file_util::CreateDirectory(subdir));
-  ASSERT_TRUE(WriteFile(file, "content"));
+  ASSERT_TRUE(WriteFile(file, kTestContent));
   VLOG(1) << "Waiting for file creation";
   ASSERT_TRUE(WaitForEvents());
 
@@ -506,6 +559,8 @@
   file_util::Move(dir, dest);
   VLOG(1) << "Waiting for directory move";
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(file_delegate.release());
+  DeleteDelegateOnFileThread(subdir_delegate.release());
 }
 
 TEST_F(FilePathWatcherTest, MoveChild) {
@@ -520,16 +575,20 @@
 
   // Setup a directory hierarchy.
   ASSERT_TRUE(file_util::CreateDirectory(source_subdir));
-  ASSERT_TRUE(WriteFile(source_file, "content"));
+  ASSERT_TRUE(WriteFile(source_file, kTestContent));
 
-  scoped_refptr<TestDelegate> file_delegate(new TestDelegate(collector()));
-  ASSERT_TRUE(SetupWatch(dest_file, &file_watcher, file_delegate.get()));
-  scoped_refptr<TestDelegate> subdir_delegate(new TestDelegate(collector()));
-  ASSERT_TRUE(SetupWatch(dest_subdir, &subdir_watcher, subdir_delegate.get()));
+  scoped_ptr<TestDelegate> file_delegate(new TestDelegate(collector()));
+  ASSERT_TRUE(SetupWatchForTestDelegate(dest_file, &file_watcher,
+                                        file_delegate.get()));
+  scoped_ptr<TestDelegate> subdir_delegate(new TestDelegate(collector()));
+  ASSERT_TRUE(SetupWatchForTestDelegate(dest_subdir, &subdir_watcher,
+                                        subdir_delegate.get()));
 
   // Move the directory into place, s.t. the watched file appears.
   ASSERT_TRUE(file_util::Move(source_dir, dest_dir));
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(file_delegate.release());
+  DeleteDelegateOnFileThread(subdir_delegate.release());
 }
 
 #if !defined(OS_LINUX)
@@ -538,14 +597,15 @@
 
 // Verify that changing attributes on a file is caught
 TEST_F(FilePathWatcherTest, FileAttributesChanged) {
-  ASSERT_TRUE(WriteFile(test_file(), "content"));
+  ASSERT_TRUE(WriteFile(test_file(), kTestContent));
   FilePathWatcher watcher;
-  scoped_refptr<TestDelegate> delegate(new TestDelegate(collector()));
-  ASSERT_TRUE(SetupWatch(test_file(), &watcher, delegate.get()));
+  scoped_ptr<TestDelegate> delegate(new TestDelegate(collector()));
+  ASSERT_TRUE(SetupWatchForTestDelegate(test_file(), &watcher, delegate.get()));
 
   // Now make sure we get notified if the file is modified.
   ASSERT_TRUE(file_util::MakeFileUnreadable(test_file()));
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 #endif  // !OS_LINUX
@@ -555,44 +615,47 @@
 // Verify that creating a symlink is caught.
 TEST_F(FilePathWatcherTest, CreateLink) {
   FilePathWatcher watcher;
-  scoped_refptr<TestDelegate> delegate(new TestDelegate(collector()));
+  scoped_ptr<TestDelegate> delegate(new TestDelegate(collector()));
   // Note that we are watching the symlink
-  ASSERT_TRUE(SetupWatch(test_link(), &watcher, delegate.get()));
+  ASSERT_TRUE(SetupWatchForTestDelegate(test_link(), &watcher, delegate.get()));
 
   // Now make sure we get notified if the link is created.
   // Note that test_file() doesn't have to exist.
   ASSERT_TRUE(file_util::CreateSymbolicLink(test_file(), test_link()));
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 // Verify that deleting a symlink is caught.
 TEST_F(FilePathWatcherTest, DeleteLink) {
   // Unfortunately this test case only works if the link target exists.
   // TODO(craig) fix this as part of crbug.com/91561.
-  ASSERT_TRUE(WriteFile(test_file(), "content"));
+  ASSERT_TRUE(WriteFile(test_file(), kTestContent));
   ASSERT_TRUE(file_util::CreateSymbolicLink(test_file(), test_link()));
   FilePathWatcher watcher;
-  scoped_refptr<TestDelegate> delegate(new TestDelegate(collector()));
-  ASSERT_TRUE(SetupWatch(test_link(), &watcher, delegate.get()));
+  scoped_ptr<TestDelegate> delegate(new TestDelegate(collector()));
+  ASSERT_TRUE(SetupWatchForTestDelegate(test_link(), &watcher, delegate.get()));
 
   // Now make sure we get notified if the link is deleted.
   ASSERT_TRUE(file_util::Delete(test_link(), false));
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 // Verify that modifying a target file that a link is pointing to
 // when we are watching the link is caught.
 TEST_F(FilePathWatcherTest, ModifiedLinkedFile) {
-  ASSERT_TRUE(WriteFile(test_file(), "content"));
+  ASSERT_TRUE(WriteFile(test_file(), kTestContent));
   ASSERT_TRUE(file_util::CreateSymbolicLink(test_file(), test_link()));
   FilePathWatcher watcher;
-  scoped_refptr<TestDelegate> delegate(new TestDelegate(collector()));
+  scoped_ptr<TestDelegate> delegate(new TestDelegate(collector()));
   // Note that we are watching the symlink.
-  ASSERT_TRUE(SetupWatch(test_link(), &watcher, delegate.get()));
+  ASSERT_TRUE(SetupWatchForTestDelegate(test_link(), &watcher, delegate.get()));
 
   // Now make sure we get notified if the file is modified.
-  ASSERT_TRUE(WriteFile(test_file(), "new content"));
+  ASSERT_TRUE(WriteFile(test_file(), kNewTestContent));
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 // Verify that creating a target file that a link is pointing to
@@ -600,28 +663,30 @@
 TEST_F(FilePathWatcherTest, CreateTargetLinkedFile) {
   ASSERT_TRUE(file_util::CreateSymbolicLink(test_file(), test_link()));
   FilePathWatcher watcher;
-  scoped_refptr<TestDelegate> delegate(new TestDelegate(collector()));
+  scoped_ptr<TestDelegate> delegate(new TestDelegate(collector()));
   // Note that we are watching the symlink.
-  ASSERT_TRUE(SetupWatch(test_link(), &watcher, delegate.get()));
+  ASSERT_TRUE(SetupWatchForTestDelegate(test_link(), &watcher, delegate.get()));
 
   // Now make sure we get notified if the target file is created.
-  ASSERT_TRUE(WriteFile(test_file(), "content"));
+  ASSERT_TRUE(WriteFile(test_file(), kTestContent));
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 // Verify that deleting a target file that a link is pointing to
 // when we are watching the link is caught.
 TEST_F(FilePathWatcherTest, DeleteTargetLinkedFile) {
-  ASSERT_TRUE(WriteFile(test_file(), "content"));
+  ASSERT_TRUE(WriteFile(test_file(), kTestContent));
   ASSERT_TRUE(file_util::CreateSymbolicLink(test_file(), test_link()));
   FilePathWatcher watcher;
-  scoped_refptr<TestDelegate> delegate(new TestDelegate(collector()));
+  scoped_ptr<TestDelegate> delegate(new TestDelegate(collector()));
   // Note that we are watching the symlink.
-  ASSERT_TRUE(SetupWatch(test_link(), &watcher, delegate.get()));
+  ASSERT_TRUE(SetupWatchForTestDelegate(test_link(), &watcher, delegate.get()));
 
   // Now make sure we get notified if the target file is deleted.
   ASSERT_TRUE(file_util::Delete(test_file(), false));
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 // Verify that watching a file whose parent directory is a link that
@@ -632,12 +697,12 @@
   FilePath link_dir(temp_dir_.path().AppendASCII("dir.lnk"));
   FilePath file(dir.AppendASCII("file"));
   FilePath linkfile(link_dir.AppendASCII("file"));
-  scoped_refptr<TestDelegate> delegate(new TestDelegate(collector()));
+  scoped_ptr<TestDelegate> delegate(new TestDelegate(collector()));
   // dir/file should exist.
   ASSERT_TRUE(file_util::CreateDirectory(dir));
-  ASSERT_TRUE(WriteFile(file, "content"));
+  ASSERT_TRUE(WriteFile(file, kTestContent));
   // Note that we are watching dir.lnk/file which doesn't exist yet.
-  ASSERT_TRUE(SetupWatch(linkfile, &watcher, delegate.get()));
+  ASSERT_TRUE(SetupWatchForTestDelegate(linkfile, &watcher, delegate.get()));
 
   ASSERT_TRUE(file_util::CreateSymbolicLink(dir, link_dir));
   VLOG(1) << "Waiting for link creation";
@@ -650,6 +715,7 @@
   ASSERT_TRUE(file_util::Delete(file, false));
   VLOG(1) << "Waiting for file deletion";
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 // Verify that watching a file whose parent directory is a
@@ -660,15 +726,15 @@
   FilePath link_dir(temp_dir_.path().AppendASCII("dir.lnk"));
   FilePath file(dir.AppendASCII("file"));
   FilePath linkfile(link_dir.AppendASCII("file"));
-  scoped_refptr<TestDelegate> delegate(new TestDelegate(collector()));
+  scoped_ptr<TestDelegate> delegate(new TestDelegate(collector()));
   // Now create the link from dir.lnk pointing to dir but
   // neither dir nor dir/file exist yet.
   ASSERT_TRUE(file_util::CreateSymbolicLink(dir, link_dir));
   // Note that we are watching dir.lnk/file.
-  ASSERT_TRUE(SetupWatch(linkfile, &watcher, delegate.get()));
+  ASSERT_TRUE(SetupWatchForTestDelegate(linkfile, &watcher, delegate.get()));
 
   ASSERT_TRUE(file_util::CreateDirectory(dir));
-  ASSERT_TRUE(WriteFile(file, "content"));
+  ASSERT_TRUE(WriteFile(file, kTestContent));
   VLOG(1) << "Waiting for dir/file creation";
   ASSERT_TRUE(WaitForEvents());
 
@@ -679,6 +745,7 @@
   ASSERT_TRUE(file_util::Delete(file, false));
   VLOG(1) << "Waiting for file deletion";
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 // Verify that watching a file with a symlink on the path
@@ -689,13 +756,13 @@
   FilePath link_dir(temp_dir_.path().AppendASCII("dir.lnk"));
   FilePath file(dir.AppendASCII("file"));
   FilePath linkfile(link_dir.AppendASCII("file"));
-  scoped_refptr<TestDelegate> delegate(new TestDelegate(collector()));
+  scoped_ptr<TestDelegate> delegate(new TestDelegate(collector()));
   ASSERT_TRUE(file_util::CreateDirectory(dir));
   ASSERT_TRUE(file_util::CreateSymbolicLink(dir, link_dir));
   // Note that we are watching dir.lnk/file but the file doesn't exist yet.
-  ASSERT_TRUE(SetupWatch(linkfile, &watcher, delegate.get()));
+  ASSERT_TRUE(SetupWatchForTestDelegate(linkfile, &watcher, delegate.get()));
 
-  ASSERT_TRUE(WriteFile(file, "content"));
+  ASSERT_TRUE(WriteFile(file, kTestContent));
   VLOG(1) << "Waiting for file creation";
   ASSERT_TRUE(WaitForEvents());
 
@@ -706,6 +773,7 @@
   ASSERT_TRUE(file_util::Delete(file, false));
   VLOG(1) << "Waiting for file deletion";
   ASSERT_TRUE(WaitForEvents());
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 #endif  // OS_LINUX
@@ -815,11 +883,11 @@
   // Setup a directory hierarchy.
   ASSERT_TRUE(file_util::CreateDirectory(test_dir1));
   ASSERT_TRUE(file_util::CreateDirectory(test_dir2));
-  ASSERT_TRUE(WriteFile(test_file, "content"));
+  ASSERT_TRUE(WriteFile(test_file, kTestContent));
 
   FilePathWatcher watcher;
-  scoped_refptr<TestDelegate> delegate(new TestDelegate(collector()));
-  ASSERT_TRUE(SetupWatch(test_file, &watcher, delegate.get()));
+  scoped_ptr<TestDelegate> delegate(new TestDelegate(collector()));
+  ASSERT_TRUE(SetupWatchForTestDelegate(test_file, &watcher, delegate.get()));
 
   // We should not get notified in this case as it hasn't affected our ability
   // to access the file.
@@ -835,6 +903,7 @@
   ASSERT_TRUE(ChangeFilePermissions(test_dir1, Execute, false));
   ASSERT_TRUE(WaitForEvents());
   ASSERT_TRUE(ChangeFilePermissions(test_dir1, Execute, true));
+  DeleteDelegateOnFileThread(delegate.release());
 }
 
 #endif  // OS_MACOSX