Update documentation for base::ThreadChecker

Update the documentation for base::ThreadChecker to provide context for
deciding whether to publicly inherit from base::NonThreadSafe vs privately
having a base::ThreadChecker member. As per the Google and Chromium C++
style guides, the preference is for composition, rather than inheritance,
combined with good documentation.

BUG=none
TEST=existing


Review URL: https://chromiumcodereview.appspot.com/10659012

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

2 files changed, 27 insertions, 12 deletions

diff --git a/base/threading/non_thread_safe.h b/base/threading/non_thread_safe.h
index 3f60fed..da7b528 100644
--- a/base/threading/non_thread_safe.h
+++ b/base/threading/non_thread_safe.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -48,8 +48,13 @@ class NonThreadSafeDoNothing {
 //   }
 // }
 //
-// In Release mode, CalledOnValidThread will always return true.
+// Note that base::ThreadChecker offers identical functionality to
+// NonThreadSafe, but does not require inheritence. In general, it is preferable
+// to have a base::ThreadChecker as a member, rather than inherit from
+// NonThreadSafe. For more details about when to choose one over the other, see
+// the documentation for base::ThreadChecker.
 //
+// In Release mode, CalledOnValidThread will always return true.
 #ifndef NDEBUG
 class NonThreadSafe : public NonThreadSafeImpl {
 };
diff --git a/base/threading/thread_checker.h b/base/threading/thread_checker.h
index 0525791..9c79958 100644
--- a/base/threading/thread_checker.h
+++ b/base/threading/thread_checker.h
@@ -40,24 +40,34 @@ class ThreadCheckerDoNothing {
   void DetachFromThread() {}
 };
 
-// Before using this class, please consider using NonThreadSafe as it
-// makes it much easier to determine the nature of your class.
-//
 // ThreadChecker is a helper class used to help verify that some methods of a
-// class are called from the same thread.  One can inherit from this class and
-// use CalledOnValidThread() to verify.
+// class are called from the same thread. It provides identical functionality to
+// base::NonThreadSafe, but it is meant to be held as a member variable, rather
+// than inherited from base::NonThreadSafe.
+//
+// While inheriting from base::NonThreadSafe may give a clear indication about
+// the thread-safety of a class, it may also lead to violations of the style
+// guide with regard to multiple inheritence. The choice between having a
+// ThreadChecker member and inheriting from base::NonThreadSafe should be based
+// on whether:
+//  - Derived classes need to know the thread they belong to, as opposed to
+//    having that functionality fully encapsulated in the base class.
+//  - Derived classes should be able to reassign the base class to another
+//    thread, via DetachFromThread.
 //
-// Inheriting from class indicates that one must be careful when using the
-// class with multiple threads. However, it is up to the class document to
-// indicate how it can be used with threads.
+// If neither of these are true, then having a ThreadChecker member and calling
+// CalledOnValidThread is the preferable solution.
 //
 // Example:
-// class MyClass : public ThreadChecker {
+// class MyClass {
 //  public:
 //   void Foo() {
-//     DCHECK(CalledOnValidThread());
+//     DCHECK(thread_checker_.CalledOnValidThread());
 //     ... (do stuff) ...
 //   }
+//
+//  private:
+//   ThreadChecker thread_checker_;
 // }
 //
 // In Release mode, CalledOnValidThread will always return true.