Combine SSLCertErrorHandler and SSLErrorHandler

content/browser/ssl/ssl_error_handler.h

Index: content/browser/ssl/ssl_error_handler.h
diff --git a/content/browser/ssl/ssl_error_handler.h b/content/browser/ssl/ssl_error_handler.h
index 658ec01a40d13bece3745f0b574c2b1d5a003742..a713377f8c9d390d53bbf3a85c29f6f93556b4cc 100644
--- a/content/browser/ssl/ssl_error_handler.h
+++ b/content/browser/ssl/ssl_error_handler.h
@@ -13,17 +13,16 @@
 #include "content/common/content_export.h"
 #include "content/public/browser/global_request_id.h"
 #include "content/public/common/resource_type.h"
+#include "net/ssl/ssl_info.h"
 #include "url/gurl.h"
 
 namespace net {
-class SSLInfo;
 class URLRequest;
 }  // namespace net
 
 namespace content {
 
 class ResourceDispatcherHostImpl;
-class SSLCertErrorHandler;
 class SSLManager;
 class WebContents;
 
@@ -60,7 +59,12 @@ class SSLErrorHandler : public base::RefCountedThreadSafe<SSLErrorHandler> {
     virtual ~Delegate() {}
   };
 
-  virtual SSLCertErrorHandler* AsSSLCertErrorHandler();
+  // Construct on the IO thread.
+  SSLErrorHandler(const base::WeakPtr<Delegate>& delegate,
+                  ResourceType resource_type,
+                  const GURL& url,
+                  const net::SSLInfo& ssl_info,
+                  bool fatal);
 
   // Find the appropriate SSLManager for the net::URLRequest and begin handling
   // this error.
@@ -68,33 +72,29 @@ class SSLErrorHandler : public base::RefCountedThreadSafe<SSLErrorHandler> {
   // Call on UI thread.
   void Dispatch(const base::Callback<WebContents*(void)>& web_contents_getter);
 
-  // Available on either thread.
+  // These accessors are available on either thread
+  const net::SSLInfo& ssl_info() const { return ssl_info_; }
+  int cert_error() const { return cert_error_; }
+  bool fatal() const { return fatal_; }
   const GURL& request_url() const { return request_url_; }
-
-  // Available on either thread.
   ResourceType resource_type() const { return resource_type_; }
 
   // Cancels the associated net::URLRequest.
-  // This method can be called from OnDispatchFailed and OnDispatched.
   CONTENT_EXPORT void CancelRequest();
 
   // Continue the net::URLRequest ignoring any previous errors.  Note that some
   // errors cannot be ignored, in which case this will result in the request
   // being canceled.
-  // This method can be called from OnDispatchFailed and OnDispatched.
   void ContinueRequest();
 
   // Cancels the associated net::URLRequest and mark it as denied.  The renderer
   // processes such request in a special manner, optionally replacing them
   // with alternate content (typically frames content is replaced with a
   // warning message).
-  // This method can be called from OnDispatchFailed and OnDispatched.
   void DenyRequest();
 
   // Does nothing on the net::URLRequest but ensures the current instance ref
-  // count is decremented appropriately.  Subclasses that do not want to
-  // take any specific actions in their OnDispatched/OnDispatchFailed should
-  // call this.
+  // count is decremented appropriately.
   void TakeNoAction();
 
   // Returns the manager associated with this SSLErrorHandler.
@@ -102,16 +102,11 @@ class SSLErrorHandler : public base::RefCountedThreadSafe<SSLErrorHandler> {
   SSLManager* GetManager() const;
 
  protected:

[nasko, 2016/08/08 18:47:12]

Why not remove "protected:"?

[estark, 2016/08/08 20:13:00]

Done.

+ private:
   friend class base::RefCountedThreadSafe<SSLErrorHandler>;
 
-  // Construct on the IO thread.
-  SSLErrorHandler(const base::WeakPtr<Delegate>& delegate,
-                  ResourceType resource_type,
-                  const GURL& url);
-
   virtual ~SSLErrorHandler();
 
-  // The following 2 methods are the methods subclasses should implement.
   virtual void OnDispatchFailed();
 
   // Can use the manager_ member.
@@ -123,7 +118,6 @@ class SSLErrorHandler : public base::RefCountedThreadSafe<SSLErrorHandler> {
   // The delegate we are associated with.
   base::WeakPtr<Delegate> delegate_;
 
- private:
   // Completes the CancelRequest operation on the IO thread.
   // Call on the IO thread.
   void CompleteCancelRequest(int error);
@@ -137,18 +131,25 @@ class SSLErrorHandler : public base::RefCountedThreadSafe<SSLErrorHandler> {
   // Call on the IO thread.
   void CompleteTakeNoAction();
 
+  // A flag to make sure we notify the net::URLRequest exactly once.
+  // Should only be accessed on the IO thread
+  bool request_has_been_notified_;
+
+  // The below read-only members may be accessed on any thread.
+
   // The URL that we requested.
-  // This read-only member can be accessed on any thread.
   const GURL request_url_;
 
   // What kind of resource is associated with the requested that generated
   // that error.
-  // This read-only member can be accessed on any thread.
   const ResourceType resource_type_;
 

[nasko, 2016/08/08 18:47:12]

nit: No need for empty lines between the members, especially when there are no
comments.

[estark, 2016/08/08 20:13:01]

I ended up adding comments to each of them for consistency, so I left the empty
lines before each comment to break it up a bit (and consistent with your comment
below).

-  // A flag to make sure we notify the net::URLRequest exactly once.
-  // Should only be accessed on the IO thread
-  bool request_has_been_notified_;
+  const net::SSLInfo ssl_info_;
+
+  const int cert_error_;  // The error we represent.

[nasko, 2016/08/08 18:47:12]

I know this is a code move, but let's clean things up a bit. Usually comments on
member variables go right above them with an empty line before the comment.

[estark, 2016/08/08 20:13:01]

Done.

+
+  const bool fatal_;  // True if the error is from a host requiring
+                      // certificate errors to be fatal.
 
   DISALLOW_COPY_AND_ASSIGN(SSLErrorHandler);
 };