Make members of Singleton<T> private and only visible to the singleton type.

net/base/cert_database_nss_unittest.cc

Index: net/base/cert_database_nss_unittest.cc
diff --git a/net/base/cert_database_nss_unittest.cc b/net/base/cert_database_nss_unittest.cc
index 5056e5dfbfdac1d57476b46c94c9786de268e00f..d2428c59a923419c35abc246de0b89711ed3e178 100644
--- a/net/base/cert_database_nss_unittest.cc
+++ b/net/base/cert_database_nss_unittest.cc
@@ -104,15 +104,11 @@ bool ReadCertIntoList(const std::string& name, CertificateList* certs) {
 class CertDatabaseNSSTest : public testing::Test {
  public:
   virtual void SetUp() {

[M-A Ruel, 2010/12/10 14:52:18]

Isn't there already a base class that creates a temp dir for you? If not, it
would probably be a good idea but that's outside the scope of this test.

-    if (!temp_db_initialized_) {
-      ScopedTempDir* temp_db_dir = Singleton<
-          ScopedTempDir,
-          DefaultSingletonTraits<ScopedTempDir>,
-          CertDatabaseNSSTest>::get();
-      ASSERT_TRUE(temp_db_dir->CreateUniqueTempDir());
+    if (!temp_db_dir_.get()) {
+      temp_db_dir_.reset(new ScopedTempDir());
+      ASSERT_TRUE(temp_db_dir_->CreateUniqueTempDir());
       ASSERT_TRUE(
-          base::OpenTestNSSDB(temp_db_dir->path(), "CertDatabaseNSSTest db"));
-      temp_db_initialized_ = true;
+          base::OpenTestNSSDB(temp_db_dir_->path(), "CertDatabaseNSSTest db"));
     }
     slot_.reset(base::GetDefaultNSSKeySlot());
 
@@ -121,7 +117,7 @@ class CertDatabaseNSSTest : public testing::Test {
   }
   virtual void TearDown() {
     // Don't try to cleanup if the setup failed.
-    ASSERT_TRUE(temp_db_initialized_);
+    ASSERT_TRUE(temp_db_dir_.get());
     ASSERT_TRUE(slot_.get());
 
     EXPECT_TRUE(CleanupSlotContents(slot_.get()));
@@ -133,11 +129,11 @@ class CertDatabaseNSSTest : public testing::Test {
   CertDatabase cert_db_;
 
  private:
-  static bool temp_db_initialized_;
+  static scoped_ptr<ScopedTempDir> temp_db_dir_;
 };
 
 // static
-bool CertDatabaseNSSTest::temp_db_initialized_ = false;
+scoped_ptr<ScopedTempDir> CertDatabaseNSSTest::temp_db_dir_;

[M-A Ruel, 2010/12/10 14:52:18]

And why do you think it's better to use a scoped_ptr here? Even if it's harmless
in a unit test, I'm afraid people would copy-paste it on real source code.

[Satish, 2010/12/10 15:04:57]

When getting rid of the singleton I considered either a LazyInstance or a
scopedptr, and scopedptr seemed to fit with the class. However it is different
from the singleton because the ScopedTempDir was earlier living across tests,
intentional or not. I can switch it to LazyInstance outside this class if you
think that is better.

[M-A Ruel, 2010/12/10 15:09:42]

I'm simply afraid of showing bad use cases. A global scoped_ptr is a bad
example.

[Satish, 2010/12/10 16:10:11]

I agree, though this is really just declaring the class's static variable it
could be just used as a global during cut/paste. But a global LazyInstance would
stay alive after these tests finish. I thought the //static comment was
sufficient to identify this is not a plain global. Do you have any suggestion on
what I should change this to?

[M-A Ruel, 2010/12/10 16:18:37]

A class member variable. Creating the directory on each unit test doesn't take
long and reduce inter-tests side-effects. I don't think there's any need for any
global state here.

[Satish, 2010/12/10 17:13:48]

Done.

 
 TEST_F(CertDatabaseNSSTest, ListCerts) {
   // This test isn't terribly useful, though it will at least let valgrind test