Remember user decisions on invalid certificates behind a flag

content/public/browser/ssl_host_state_decisions.h

+// Copyright (c) 2014 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 CONTENT_BROWSER_SSL_HOST_STATE_DECISIONS_H_
+#define CONTENT_BROWSER_SSL_HOST_STATE_DECISIONS_H_
+
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "content/common/content_export.h"
+#include "net/cert/x509_certificate.h"
+
+class GURL;
+
+namespace content {
+
+// SSLHostStateDecisions must be implemented by the embedder, to provide the
+// storage strategy (if any) for certificate decisions.

[jam, 2014/07/31 22:01:08]

this sounds like it's required by the embedder, which is actually not true
looking at the code. so update the comment.

for background, that's the goal of the content api, which is for embedders to be
able to embed content easily, and then implement more interfaces to further
customize it

[jww, 2014/07/31 23:53:10]

Done.

+class CONTENT_EXPORT SSLHostStateDecisions {

[jam, 2014/07/31 22:01:08]

for consistency, this should be something like SSLHostStateDelegate. you can add
SSLHostState in https://codereview.chromium.org/418133012/, and have a
SetDelegate method on it to take an implementation from the embedder

nit: no export needed

[jww, 2014/07/31 23:53:10]

I've changed it to SSLHostStateDecisionsDelegate since it's not a delegate for
*all* of SSLHostState, only the decisions portion.

I am a bit confused, though. I thought one of the points of the KeyedServices
was for providing singletons from the embedder, which this needs to be.

Is your suggestion that we create a static method on BrowserContext called
"CreateSSLHostStateDecisionsDelegate" or something like that, where we pass in
the SSLHostState and have it set the delegate? And if so, isn't that just
somewhat repeating the functionality of the KeyedServices?

Sorry for my confusion. I'm sure the answer is obvious, and it's just me being a
n00b to content :-)
Done.

[jww, 2014/08/01 00:21:35]

Okay, so I think I understand a bit better what you're suggesting. Basically,
implement something similar to how the DownloadManager works, in particular
GetDownloadManager on BrowserContext.

So the complication is that we won't need SSLHostState in content/public until
we finish https://codereview.chromium.org/418133012/, which is much lower
priority (it's sort of a nice addition to the experiment, but this CL, the
experiment itself, is of much higher priority).

Thus, my suggestion is, if you're cool with it, we hold off on making this a
proper delegate until we move SSLHostState to content/public, and we thus leave
SSLHostStateDecisions as is for now. Let me know if this doesn't make sense.

In either case, for now, I'll upload my patch with the other fixes.

+ public:
+  virtual void DenyCert(const GURL& url,

[jam, 2014/07/31 22:01:08]

nit: please comment all these methods

[jww, 2014/07/31 23:53:10]

Done.

+                        net::X509Certificate* cert,
+                        net::CertStatus error) = 0;
+  virtual void AllowCert(const GURL& url,
+                         net::X509Certificate* cert,
+                         net::CertStatus error) = 0;
+  virtual void Clear() = 0;
+  virtual net::CertPolicy::Judgment QueryPolicy(const GURL& url,
+                                                net::X509Certificate* cert,
+                                                net::CertStatus error) = 0;
+  virtual void RevokeAllowAndDenyPreferences(const GURL& url) = 0;
+  virtual bool HasAllowedOrDeniedCert(const GURL& url) = 0;
+
+ protected:
+  virtual ~SSLHostStateDecisions() {}
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_SSL_HOST_STATE_DECISIONS_H_