Break cyclic dependency between CertDatabase and NSSCertDatabase.

net/cert/cert_database.h

 // 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.
 
 #ifndef NET_CERT_CERT_DATABASE_H_
 #define NET_CERT_CERT_DATABASE_H_
 
 #include "base/basictypes.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "net/base/net_export.h"
 #include "net/cert/x509_certificate.h"
 
 template <typename T> struct DefaultSingletonTraits;
 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.
 
 // TODO(gauravsh): This class could be augmented with methods
 // for all operations that manipulate the underlying system
 // certificate store.
 
 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.
     virtual void OnCertRemoved(const X509Certificate* cert) {}
 
     // 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:
     Observer() {}
 
    private:
     DISALLOW_COPY_AND_ASSIGN(Observer);
   };
 
   // Returns the CertDatabase singleton.
@@ skipping 29 matching lines @@
   // KeyStore used for storing client certificates. When the Java side replaces
   // the KeyStore used for client certificates, notifies the observers as if a
   // new client certificate was added.
   void OnAndroidKeyStoreChanged();
 
   // On Android, the system database is used. When the system notifies the
   // application that the certificates changed, the observers must be notified.
   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>;
 
   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_;
 #endif
 
   DISALLOW_COPY_AND_ASSIGN(CertDatabase);
 };
 
 }  // namespace net
 
 #endif  // NET_CERT_CERT_DATABASE_H_