Break cyclic dependency between CertDatabase and NSSCertDatabase.

Break cyclic dependency between CertDatabase and NSSCertDatabase.

Before:
- CertDatabase::ObserveNSSCertDatabase(NSSCertDatabase* source) and implementation referenced NSSCertDatabase
- NSSCertDatabase() referenced CertDatabase::GetInstance()

Now:
- CertDatabase has no reference to NSSCertDatabase.
- NSSCertDatabase references CertDatabase.

BUG=NONE

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282594

[pneubeck (no reviews), 2014-07-04 09:21:58]

Small change that simplifies dependencies a bit for future readers and changes.

[pneubeck (no reviews), 2014-07-08 08:30:36]

+Matt, I think you touched this code last. ptal.

[mattm, 2014-07-08 21:13:06]

otherwise lgtm

https://codereview.chromium.org/370633003/diff/1/net/cert/nss_cert_database.cc
File net/cert/nss_cert_database.cc (right):

https://codereview.chromium.org/370633003/diff/1/net/cert/nss_cert_database.c...
net/cert/nss_cert_database.cc:72: // This makes sure that NSS has been
initialized.
previous comment was correct (CertDatabase constructor calls
crypto::EnsureNSSInit())

[Ryan Sleevi, 2014-07-08 21:36:53]

This still feels like a layering violation for me, and still has a bit of a
smell.

CertDatabase::Observer is an interface designed for having the CertDatabase push
notifications. But you've overridden it's responsibilities and implementation
(via GetObserver()) to use it to *push* notifications out (to the CertDatabase).

Put differently, this conflates push/pull models in a way that I think makes it
equally hard to reason about the correctness of.

[Ryan Sleevi, 2014-07-08 21:41:41]

To be more constructive: Possible solutions include:

1) CertDatabase describes a way for people to "push" notifications into it (as
part of a Single-Responsibility Principal of keeping observers as a way for
others to "pull" from the CertDatabase)

(NSSCertDatabase depends on CertDatabase)

2) Invert the design so that NSSCertDatabase has a way for people to "push"
notifications into it, and have CertDatabase both "push" notifications from
itself, and "pull" notifications from the NSSCertDatabase so that it can "push"
to it's Observers

(CertDatabase depends on NSSCertDatabase)

[pneubeck (no reviews), 2014-07-09 12:55:48]

On 2014/07/08 21:41:41, Ryan Sleevi wrote:
> To be more constructive: Possible solutions include:
> 
> 1) CertDatabase describes a way for people to "push" notifications into it (as
> part of a Single-Responsibility Principal of keeping observers as a way for
> others to "pull" from the CertDatabase)
> 
> (NSSCertDatabase depends on CertDatabase)
NSSCertDatabase only depends on the 'push notification' functions of
CertDatabase. Thus, having public 'CertDatabase::NotifyObserversOf...' functions
(not inherited from an interface) called by NSSCertDatabase would tighten this
dependency more than required. E.g. NSSCertDatabase does not AddUser certs
through CertDatabase.

> 
> 2) Invert the design so that NSSCertDatabase has a way for people to "push"
> notifications into it, and have CertDatabase both "push" notifications from
> itself, and "pull" notifications from the NSSCertDatabase so that it can
"push"
> to it's Observers
> 
> (CertDatabase depends on NSSCertDatabase)
NSSCertDatabase's consumers are not in net/ .
net/ uses CertDatabase and doesn't care about NSSCertDatabase. Thus the
dependency in 2) is IMO not helpful.


All of the proposed solutions (yours or mine) don't change one fact:
CertDatabase acts both as a source for events and as a receiver of events.
In the role of a receiver of events, it is an 'Observer of certificate changes'.
I think it's helpful to make clear in the code that the received events and sent
events are of the same kind (actually simply forwarded). IMO, this is clearer if
the Observer interfaces are the same.

I agree that patch set #2 had a contradiction between the usage of
CertDatabase::Observer and its documented meaning (being a subclass of
CertDatabase and its comment).
Thus, I went ahead and moved it out of CertDatabase and renamed it to something
more generic (CertificateChangeObserver).
Thus the definition should now better match its usage.

[Ryan Sleevi, 2014-07-09 13:31:10]

Unfortunately, I don't feel your patchset really addresses the concerns I had,
and I apologize, but I'm still having trouble understanding your objections.

To try and be clearer: I think the push and pull operations should be
identifiably distinct. I don't think re-using an Observer class for both helps
readability
I agree we have bad layering/deps, and I have no big preferences for A->B or
B->A, but reusing the same interface to do push and pull I think is wrong.

[pneubeck (no reviews), 2014-07-09 13:37:47]

On 2014/07/09 13:31:10, Ryan Sleevi wrote:
> Unfortunately, I don't feel your patchset really addresses the concerns I had,
> and I apologize, but I'm still having trouble understanding your objections.
> 
> To try and be clearer: I think the push and pull operations should be
> identifiably distinct. I don't think re-using an Observer class for both helps
> readability
> I agree we have bad layering/deps, and I have no big preferences for A->B or
> B->A, but reusing the same interface to do push and pull I think is wrong.

To fully understand your reasoning. Are you saying that there is a principal
difference in CertDatabase receiving a notification vs. any other subclass of
CertDatabase::Observer receiving a notification?

[Ryan Sleevi, 2014-07-09 13:49:42]

Yes, I'm saying there is and should be a difference.

Observer-type interfaces (where A is an Observer) is so that A can register with
B, without B having to know the details.

Here, you're having A be an observer, but it doesn't handle registration - B
does. So B has to know about A. This is still a conceptual circular dependency.

Having A implement explicit methods to push into it - explicitly, rather than
wrapping in an Observer - is a design acknowledgement that whoever wants to push
into A depends on A.

To try and restate differently, since I realize this is likely clear as
Mississippi mud - A should only implement observer if depends on B, and is
trying to prevent B from knowing about A. Here, B explicitly depends on A, A
shouldn't know or care about B - A should define its functionally specifically
under its own terms, under its own methods, and if B wants to abstract that (or,
say, abstract things so C doesn't have to depend on A), then B would be
responsible for wrapping things into an Observer.

A is CertDatabase, B is NSSCertDatabase. If anyone is going to create an
observer that notifies A, it should be B creating that helper class.

[pneubeck (no reviews), 2014-07-09 13:55:23]

On 2014/07/09 13:49:42, Ryan Sleevi wrote:
> Yes, I'm saying there is and should be a difference.
> 
> Observer-type interfaces (where A is an Observer) is so that A can register
with
> B, without B having to know the details.
> 
> Here, you're having A be an observer, but it doesn't handle registration - B
> does. So B has to know about A. This is still a conceptual circular
dependency.
> 
> Having A implement explicit methods to push into it - explicitly, rather than
> wrapping in an Observer - is a design acknowledgement that whoever wants to
push
> into A depends on A.
> 
> To try and restate differently, since I realize this is likely clear as
> Mississippi mud - A should only implement observer if depends on B, and is
> trying to prevent B from knowing about A. Here, B explicitly depends on A, A
> shouldn't know or care about B - A should define its functionally specifically
> under its own terms, under its own methods, and if B wants to abstract that
(or,
> say, abstract things so C doesn't have to depend on A), then B would be
> responsible for wrapping things into an Observer.
> 
> A is CertDatabase, B is NSSCertDatabase. If anyone is going to create an
> observer that notifies A, it should be B creating that helper class.

I see your point now. I'll adapt it accordingly. Thanks a lot for being patient
to clarify and sorry for appearing stubborn ;-)

[pneubeck (no reviews), 2014-07-10 10:14:18]

@Ryan, ptal

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

lgtm

https://codereview.chromium.org/370633003/diff/110001/net/cert/cert_database.h
File net/cert/cert_database.h (right):

https://codereview.chromium.org/370633003/diff/110001/net/cert/cert_database....
net/cert/cert_database.h:38: // Called with |cert| == NULL after an import of
multiple certificates.
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.

https://codereview.chromium.org/370633003/diff/110001/net/cert/cert_database....
net/cert/cert_database.h:46: // Called with |cert| == NULL after an import of
multiple certificates.
This comment feels like an implementation detail that shouldn't be in the
header.

https://codereview.chromium.org/370633003/diff/110001/net/cert/cert_database....
net/cert/cert_database.h:96: // Broadcasts notifications to all registered
observers.
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.

[pneubeck (no reviews), 2014-07-11 06:31:22]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-07-11 06:31:47]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pneubeck@chromium.org/370633003/130001

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

https://codereview.chromium.org/370633003/diff/110001/net/cert/cert_database.h
File net/cert/cert_database.h (right):

https://codereview.chromium.org/370633003/diff/110001/net/cert/cert_database....
net/cert/cert_database.h:38: // Called with |cert| == NULL after an import of
multiple certificates.
On 2014/07/10 21:17:14, Ryan Sleevi wrote:
> 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.

Done.

https://codereview.chromium.org/370633003/diff/110001/net/cert/cert_database....
net/cert/cert_database.h:46: // Called with |cert| == NULL after an import of
multiple certificates.
On 2014/07/10 21:17:13, Ryan Sleevi wrote:
> This comment feels like an implementation detail that shouldn't be in the
> header.

Done.

[commit-bot: I haz the power, 2014-07-11 09:28:25]

Change committed as 282594