Add button to page info to revoke user certificate decisions.

chrome/browser/ui/website_settings/website_settings_ui.h

Index: chrome/browser/ui/website_settings/website_settings_ui.h
diff --git a/chrome/browser/ui/website_settings/website_settings_ui.h b/chrome/browser/ui/website_settings/website_settings_ui.h
index e12485132e3df827ea361e869785bbe931439d40..119e477ff52557c11561c4ff6400b60e3a3aa86b 100644
--- a/chrome/browser/ui/website_settings/website_settings_ui.h
+++ b/chrome/browser/ui/website_settings/website_settings_ui.h
@@ -95,6 +95,11 @@ class WebsiteSettingsUI {
     // Textual description of the site's connection status that is displayed to
     // the user.
     std::string connection_status_description;
+    // Set when the user has explicitly bypassed an SSL error for this host.
+    // When |certificate_decision_made| is true, the connection area of the
+    // page info will include an option for the user to revoke their decision to
+    // bypass the SSL error for this host.

[Peter Kasting, 2014/08/07 03:56:58]

This comment is much better now!  Maybe the variable should be named
"[have_]bypassed_ssl_error" or "show_revoke_button" or something to be more
directly connected to the description you give.

It seems a little odd as well that this comment sounds like this is only about
"bypassing an error" but in the code we check if the user has allowed _or
denied_ a cert.  Is that an actual mismatch or am I just misinterpreting the
code?

[jww, 2014/08/08 16:48:50]

have_bypassed_ssl_error sounds great to me.
Good point. Unfortunately, it's a little of both. We do check both because,
theoretically, there is "Allow" and "Deny," but in practice, "Deny" is never
called. I suppose there is a theoretic world in which we implement a content
setting to "always deny" or something like that, but it doesn't exist.

I'm ambivalent about how to express this in the comment. I suppose it should
probably say something additionally like, "Set when the user has explicitly
bypassed an SSL error for this host or explicitly denied it (the latter of which
is not currently possible in the Chrome UI)." What do you think?

[Peter Kasting, 2014/08/08 17:15:12]

If denying isn't possible in the Chrome UI, I think the underlying APIs here
should reflect that.  So a function like "AllowOrDenyX" would be named "AllowX"
instead.  The confusion you're struggling with comes from the fact that the
existing APIs you're calling claim to expose a distinction that never
practically occurs, so the bug is really with them rather than your new code. 
Once the APIs only talk about "allow", the names and comments here should be
fairly obvious.

I hope making these changes is relatively straightforward.  If there are a lot
of then maybe this would have to happen as a separate patch.

[jww, 2014/08/11 19:21:28]

There might be a reason to blacklist specific certs someday, so I'd like to keep
that option open in the API (additionally, a different embeder other than Chrome
might want to do this), but I agree it's definitely confusing. Addtionally, the
underlying implementation would work if "Deny" was ever called, so if anyone
ever used "Deny," they would need to go and change a renamed "HasAllowedCert"
method to be more complex.

So instead of making "HasAllowedCert," I've renamed it to be slightly more
generic ("HasUserDecision"), which I hope makes it less confusing.This also has
the advantage that if someone does start using "Deny" to blacklist certs, the UI
will still do the correct thing. Let me know if that sounds okay, or if you
think we need to change the API further (I also changed the Revoke* methods to
be clearer).

[Peter Kasting, 2014/08/11 20:24:20]

This is dangerous reasoning.  If this doesn't come to pass, we've basically made
the code more confusing than it needs to be for no benefit.  Even if it does
come to pass, we still pay that cost in the meantime.  My argument is always: if
you have strong reason to believe this _will_ be changed within a definite
timeframe (e.g. six months), then this is OK; otherwise please make the design
reflect only the capabilities you need right now.  We can always expand the API
later if we do decide we need to implement blacklisting.

To be clear, I don't think your current design is hideously awful and the end of
the world.  I just think it could be even better, and probably should be better,
as long as the reason for having it this way is purely hypothetical.

+    bool certificate_decision_made;
   };
 
   typedef std::vector<CookieInfo> CookieInfoList;