Break cyclic dependency between CertDatabase and NSSCertDatabase.

net/cert/cert_database.h

Index: net/cert/cert_database.h
diff --git a/net/cert/cert_database.h b/net/cert/cert_database.h
index 865d685097d691e763c156c8cc714b995016f22f..250738e573b45de83e3a3b70bc0b3cb34d3f91a5 100644
--- a/net/cert/cert_database.h
+++ b/net/cert/cert_database.h
@@ -16,8 +16,6 @@ template <class ObserverType> class ObserverListThreadSafe;
 
 namespace net {
 
-class NSSCertDatabase;
-
 // This class provides cross-platform functions to verify and add user
 // certificates, and to observe changes to the underlying certificate stores.
 
@@ -28,15 +26,16 @@ class NSSCertDatabase;
 class NET_EXPORT CertDatabase {
  public:
   // A CertDatabase::Observer will be notified on certificate database changes.
-  // The change could be either a new user certificate is added or trust on
-  // a certificate is changed.  Observers can register themselves
-  // via CertDatabase::AddObserver, and can un-register with
+  // The change could be either a user certificate is added/removed or trust on
+  // a certificate is changed. Observers can be registered via
+  // CertDatabase::AddObserver, and can un-register with
   // CertDatabase::RemoveObserver.
   class NET_EXPORT Observer {
    public:
     virtual ~Observer() {}
 
     // Will be called when a new certificate is added.
+    // Called with |cert| == NULL after an import of multiple certificates.

[Ryan Sleevi, 2014/07/10 21:17:14]

I feel like this is committing too much of the interface to the header

Perhaps just

// Will be called when a new certificate is added. If the imported
// cert can be determined, |cert| will be non-NULL, but if not, or
// if multiple certificates were imported, |cert| may be NULL.

[pneubeck (no reviews), 2014/07/11 08:39:07]

Done.

     virtual void OnCertAdded(const X509Certificate* cert) {}
 
     // Will be called when a certificate is removed.
@@ -44,6 +43,7 @@ class NET_EXPORT CertDatabase {
 
     // Will be called when a CA certificate was added, removed, or its trust
     // changed. This can also mean that a client certificate's trust changed.
+    // Called with |cert| == NULL after an import of multiple certificates.

[Ryan Sleevi, 2014/07/10 21:17:13]

This comment feels like an implementation detail that shouldn't be in the
header.

[pneubeck (no reviews), 2014/07/11 08:39:07]

Done.

     virtual void OnCACertChanged(const X509Certificate* cert) {}
 
    protected:
@@ -93,11 +93,10 @@ class NET_EXPORT CertDatabase {
   void OnAndroidKeyChainChanged();
 #endif
 
-#if defined(USE_NSS)
-  // Observe events from the |source| and forward them to observers of this
-  // CertDatabase.
-  void ObserveNSSCertDatabase(NSSCertDatabase* source);
-#endif
+  // Broadcasts notifications to all registered observers.

[Ryan Sleevi, 2014/07/10 21:17:13]

Perhaps reword, to indicate

// Synthetically injects notifications to all observers. In general, this should
only be called by the creator of the CertDatabase

An alternative design that I realized after our discussion would be something
like

class CertDatabase {
 public:
  class Source {
   protected:
    explicit Source(CertDatabase* db);

    void NotifyOfCertAdded(...);
    ...

   private:
    CertDatabase* const cert_db_;
  };

  explicit CertDatabase(Source* source);

 private:
  friend class Source;
  void NotifyObservers...(...);
};

Then, the thing which creates the CertDatabase (and presumably, the
NSSCertDatabase) can create a

(within a .cc)
class NSSCertDatabaseSource : public CertDatabase::Source,
                              public NSSCertDatabase::Observer {
 public:
  // NSSCertDatabase::Observer implementation
  virtual void OnNSSCertDatabase...(...) OVERRIDE {
    NotifyOfCertAdded(...);
  }
}

While this design still has a circular dependency (Source -> CertDB -> Source),
it keeps that design localized, and restricts the capability to inject
notifications to the creator (who has to supply the source)

But this relies on CertDatabase no longer being a Singleton, so it's
understandable that this is likely not yet viable.

+  void NotifyObserversOfCertAdded(const X509Certificate* cert);
+  void NotifyObserversOfCertRemoved(const X509Certificate* cert);
+  void NotifyObserversOfCACertChanged(const X509Certificate* cert);
 
  private:
   friend struct DefaultSingletonTraits<CertDatabase>;
@@ -105,14 +104,9 @@ class NET_EXPORT CertDatabase {
   CertDatabase();
   ~CertDatabase();
 
-  // Broadcasts notifications to all registered observers.
-  void NotifyObserversOfCertAdded(const X509Certificate* cert);
-  void NotifyObserversOfCertRemoved(const X509Certificate* cert);
-  void NotifyObserversOfCACertChanged(const X509Certificate* cert);
-
   const scoped_refptr<ObserverListThreadSafe<Observer> > observer_list_;
 
-#if defined(USE_NSS) || (defined(OS_MACOSX) && !defined(OS_IOS))
+#if defined(OS_MACOSX) && !defined(OS_IOS)
   class Notifier;
   friend class Notifier;
   scoped_ptr<Notifier> notifier_;