Move the NSS functions out of CertDatabase into a new NSSCertDatabase class.

net/base/cert_database.cc

Index: net/base/cert_database.cc
diff --git a/net/base/cert_database.cc b/net/base/cert_database.cc
index 7930750757de371ef6df10bfbffbeb98da1cf4cb..eb8f89af8551aa8c9876264ef6503d63efdaa28d 100644
--- a/net/base/cert_database.cc
+++ b/net/base/cert_database.cc
@@ -4,39 +4,86 @@
 
 #include "net/base/cert_database.h"
 
+#include "base/memory/ref_counted.h"
 #include "base/memory/singleton.h"
 #include "base/observer_list_threadsafe.h"
 
-namespace net {
+#if defined(USE_NSS)
+#include "net/base/nss_cert_database.h"
+#endif
 
-CertDatabase::ImportCertFailure::ImportCertFailure(
-    X509Certificate* cert, int err)
-    : certificate(cert), net_error(err) {
-}
+namespace net {
 
-CertDatabase::ImportCertFailure::~ImportCertFailure() {
-}
+namespace {
 
 // CertDatabaseNotifier notifies registered observers when new user certificates
 // are added to the database.
+#if defined(USE_NSS)
+class CertDatabaseNotifier : public NSSCertDatabase::Observer {

[Ryan Sleevi, 2012/09/05 21:44:17]

nit: Add a comment about why this inheritance (that it's forwarding along
notifications)

[Ryan Sleevi, 2012/09/05 21:44:17]

nit: I'm generally not a fan of conditional inheritance like this, since it
forces the implementation details to be 'leaked' through the interface.

It's a nit because this is a private class within an implementation file, so the
leaking is localized to this file. However, it seems possible to fully
accomodate this with encapsulation, rather than inheritance, and that might make
it easier to understand the relationship of these interfaces

class CertDatabaseNotifier {
 public:
  ...

 private:
#if defined(USE_NSS)
  class NSSObserver : public NSSCertDatabase::Observer {
   public:
    explicit NSSObserver(CertDatabaseNotifier* notifier)
        : notifier_(notifier) {
      NSSCertDatabase::GetInstance()->AddNotifier(this);
    }

    // NSSCertDatabase::Observer implementation
    virtual void OnCertAdded(...) OVERRIDE {
      notifier->NotifyObserversOfCertAdded(...);
    }

    ...
  };

  NSSObserver observer_;
#endif
};

This uses the "bridge class" concept to glue these two together, rather than
directly inheriting.

Like I said, it's not major, and does involve a little bit more wire-work to get
things structured, but I think it is more in-line with Chromium convention and
patterns.

[Joao da Silva, 2012/09/06 15:11:41]

Done.

[Joao da Silva, 2012/09/06 15:11:41]

Done.

+#else
 class CertDatabaseNotifier {
+#endif
  public:
   CertDatabaseNotifier()
       : observer_list_(new ObserverListThreadSafe<CertDatabase::Observer>) {
+#if defined(USE_NSS)
+    // Observe events from the NSSCertDatabase to propagate them to the
+    // observers of CertDatabase.
+    NSSCertDatabase nss_cert_db;
+    nss_cert_db.AddObserver(this);
+#endif
   }
 
   static CertDatabaseNotifier* GetInstance() {
     return Singleton<CertDatabaseNotifier>::get();
   }
 
-  friend struct DefaultSingletonTraits<CertDatabaseNotifier>;
-  friend class CertDatabase;
+#if defined(USE_NSS)

[Ryan Sleevi, 2012/09/05 21:44:17]

nit:
// NSSCertDatabase::Observer implementation

to clarify why these methods are #if defined(USE_NSS)

[Joao da Silva, 2012/09/06 15:11:41]

Done (though the #ifdef is gone)

+  virtual void OnCertAdded(const X509Certificate* cert) OVERRIDE {
+    NotifyObserversOfCertAdded(cert);
+  }
+
+  virtual void OnCertRemoved(const X509Certificate* cert) OVERRIDE {
+    NotifyObserversOfCertRemoved(cert);
+  }
+
+  virtual void OnCertTrustChanged(const X509Certificate* cert) OVERRIDE {
+    NotifyObserversOfCertTrustChanged(cert);
+  }
+#endif
+
+  void NotifyObserversOfCertAdded(const X509Certificate* cert) {
+    observer_list_->Notify(
+        &CertDatabase::Observer::OnCertAdded, make_scoped_refptr(cert));
+  }
+
+  void NotifyObserversOfCertRemoved(const X509Certificate* cert) {
+    observer_list_->Notify(
+        &CertDatabase::Observer::OnCertRemoved, make_scoped_refptr(cert));
+  }
+
+  void NotifyObserversOfCertTrustChanged(const X509Certificate* cert) {
+    observer_list_->Notify(
+        &CertDatabase::Observer::OnCertTrustChanged, make_scoped_refptr(cert));
+  }
 
  private:
+  friend struct DefaultSingletonTraits<CertDatabaseNotifier>;
+  friend class net::CertDatabase;
+
   const scoped_refptr<ObserverListThreadSafe<CertDatabase::Observer> >
       observer_list_;
 };
 
+}  // namespace
+
+CertDatabase::CertDatabase() {
+#if defined(USE_NSS)
+  // Make sure the NSS initialization has been performed.
+  NSSCertDatabase::EnsureInit();
+#endif
+}
+
 void CertDatabase::AddObserver(Observer* observer) {
   CertDatabaseNotifier::GetInstance()->observer_list_->AddObserver(observer);
 }
@@ -45,21 +92,17 @@ void CertDatabase::RemoveObserver(Observer* observer) {
   CertDatabaseNotifier::GetInstance()->observer_list_->RemoveObserver(observer);
 }
 
-void CertDatabase::NotifyObserversOfUserCertAdded(const X509Certificate* cert) {
-  CertDatabaseNotifier::GetInstance()->observer_list_->Notify(
-      &CertDatabase::Observer::OnUserCertAdded, make_scoped_refptr(cert));
+void CertDatabase::NotifyObserversOfCertAdded(const X509Certificate* cert) {
+  CertDatabaseNotifier::GetInstance()->NotifyObserversOfCertAdded(cert);
 }
 
-void CertDatabase::NotifyObserversOfUserCertRemoved(
-    const X509Certificate* cert) {
-  CertDatabaseNotifier::GetInstance()->observer_list_->Notify(
-      &CertDatabase::Observer::OnUserCertRemoved, make_scoped_refptr(cert));
+void CertDatabase::NotifyObserversOfCertRemoved(const X509Certificate* cert) {
+  CertDatabaseNotifier::GetInstance()->NotifyObserversOfCertRemoved(cert);
 }
 
 void CertDatabase::NotifyObserversOfCertTrustChanged(
     const X509Certificate* cert) {
-  CertDatabaseNotifier::GetInstance()->observer_list_->Notify(
-      &CertDatabase::Observer::OnCertTrustChanged, make_scoped_refptr(cert));
+  CertDatabaseNotifier::GetInstance()->NotifyObserversOfCertTrustChanged(cert);
 }
 
 }  // namespace net