Simplify WebEngineCertificateError

Removes certificateErrorControllerPrivate,
moves error handling logic to core.

Change-Id: I050f73f1e37eb9ae39ad471fe1673d2b6140cf89
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>

9 files changed, 69 insertions, 135 deletions

diff --git a/src/core/api/qwebenginecertificateerror.h b/src/core/api/qwebenginecertificateerror.h
index 67da69a78..6097f430c 100644
--- a/src/core/api/qwebenginecertificateerror.h
+++ b/src/core/api/qwebenginecertificateerror.h
@@ -46,6 +46,10 @@
 #include <QtCore/qurl.h>
 #include <QtNetwork/QSslCertificate>
 
+namespace QtWebEngineCore {
+class WebContentsDelegateQt;
+}
+
 QT_BEGIN_NAMESPACE
 
 class CertificateErrorController;
@@ -100,8 +104,7 @@ public:
     QList<QSslCertificate> certificateChain() const;
 
 private:
-    friend class QWebEnginePagePrivate;
-    friend class QQuickWebEngineViewPrivate;
+    friend class QtWebEngineCore::WebContentsDelegateQt;
     QWebEngineCertificateError(const QSharedPointer<CertificateErrorController> &controller);
     QSharedPointer<CertificateErrorController> d;
 };
diff --git a/src/core/certificate_error_controller.cpp b/src/core/certificate_error_controller.cpp
index 79f6a8cf2..7fa54bf5d 100644
--- a/src/core/certificate_error_controller.cpp
+++ b/src/core/certificate_error_controller.cpp
@@ -38,7 +38,6 @@
 ****************************************************************************/
 
 #include "certificate_error_controller.h"
-#include "certificate_error_controller_p.h"
 
 #include <net/base/net_errors.h>
 #include <net/cert/x509_certificate.h>
@@ -97,28 +96,21 @@ static int IsCertErrorFatal(int cert_error)
     return true;
 }
 
-
-CertificateErrorControllerPrivate::CertificateErrorControllerPrivate(int cert_error,
-                                                                     const net::SSLInfo& ssl_info,
-                                                                     const GURL &request_url,
-                                                                     bool strict_enforcement,
-                                                                     base::OnceCallback<void(content::CertificateRequestResultType)> cb
-                                                                    )
-    : certError(CertificateErrorController::CertificateError(cert_error))
-    , requestUrl(toQt(request_url))
-    , overridable(!IsCertErrorFatal(cert_error) && !strict_enforcement)
+CertificateErrorController::CertificateErrorController(
+        int cert_error, const net::SSLInfo &ssl_info, const GURL &request_url,
+        bool strict_enforcement, base::OnceCallback<void(content::CertificateRequestResultType)> cb)
+    : m_certError(CertificateErrorController::CertificateError(cert_error))
+    , m_requestUrl(toQt(request_url))
+    , m_overridable(!IsCertErrorFatal(cert_error) && !strict_enforcement)
 {
-    if (overridable) callback = std::move(cb);
+    if (m_overridable)
+        m_callback = std::move(cb);
     if (auto cert = ssl_info.cert.get()) {
-        validExpiry = toQt(cert->valid_expiry());
-        certificateChain = toCertificateChain(cert);
+        m_validExpiry = toQt(cert->valid_expiry());
+        m_certificateChain = toCertificateChain(cert);
     }
 }
 
-CertificateErrorController::CertificateErrorController(CertificateErrorControllerPrivate *p) : d(p)
-{
-}
-
 CertificateErrorController::~CertificateErrorController()
 {
     if (!answered())
@@ -127,32 +119,32 @@ CertificateErrorController::~CertificateErrorController()
 
 CertificateErrorController::CertificateError CertificateErrorController::error() const
 {
-    return d->certError;
+    return m_certError;
 }
 
 QUrl CertificateErrorController::url() const
 {
-    return d->requestUrl;
+    return m_requestUrl;
 }
 
 bool CertificateErrorController::overridable() const
 {
-    return d->overridable;
+    return m_overridable;
 }
 
 bool CertificateErrorController::deferred() const
 {
-    return d->deferred;
+    return m_deferred;
 }
 
 void CertificateErrorController::defer()
 {
-    d->deferred = true;
+    m_deferred = true;
 }
 
 bool CertificateErrorController::answered() const
 {
-    return d->answered;
+    return m_answered;
 }
 
 void CertificateErrorController::accept(bool accepted)
@@ -160,14 +152,16 @@ void CertificateErrorController::accept(bool accepted)
     if (answered())
         return;
 
-    d->answered = true;
-    if (d->callback)
-        std::move(d->callback).Run(accepted ? content::CERTIFICATE_REQUEST_RESULT_TYPE_CONTINUE : content::CERTIFICATE_REQUEST_RESULT_TYPE_DENY);
+    m_answered = true;
+    if (m_callback)
+        std::move(m_callback)
+                .Run(accepted ? content::CERTIFICATE_REQUEST_RESULT_TYPE_CONTINUE
+                              : content::CERTIFICATE_REQUEST_RESULT_TYPE_DENY);
 }
 
 void CertificateErrorController::deactivate()
 {
-    d->callback.Reset();
+    m_callback.Reset();
 }
 
 static QString getQStringForMessageId(int message_id) {
@@ -180,13 +174,13 @@ QString CertificateErrorController::errorString() const
     // Try to use chromiums translation of the error strings, though not all are
     // consistently described and we need to use versions that does not contain HTML
     // formatted text.
-    switch (d->certError) {
+    switch (m_certError) {
     case SslPinnedKeyNotInCertificateChain:
         return getQStringForMessageId(IDS_CERT_ERROR_SUMMARY_PINNING_FAILURE_DETAILS);
     case CertificateCommonNameInvalid:
         return getQStringForMessageId(IDS_CERT_ERROR_COMMON_NAME_INVALID_DESCRIPTION);
     case CertificateDateInvalid:
-        if (QDateTime::currentDateTime() > d->validExpiry)
+        if (QDateTime::currentDateTime() > m_validExpiry)
             return getQStringForMessageId(IDS_CERT_ERROR_EXPIRED_DESCRIPTION);
         else
             return getQStringForMessageId(IDS_CERT_ERROR_NOT_YET_VALID_DESCRIPTION);
@@ -224,7 +218,7 @@ QString CertificateErrorController::errorString() const
 
 QList<QSslCertificate> CertificateErrorController::certificateChain() const
 {
-    return d->certificateChain;
+    return m_certificateChain;
 }
 
 QT_END_NAMESPACE
diff --git a/src/core/certificate_error_controller.h b/src/core/certificate_error_controller.h
index be5efd79d..1b2d102dd 100644
--- a/src/core/certificate_error_controller.h
+++ b/src/core/certificate_error_controller.h
@@ -52,19 +52,26 @@
 #define CERTIFICATE_ERROR_CONTROLLER_H
 
 #include "qtwebenginecoreglobal_p.h"
-
+#include "base/callback.h"
+#include "content/public/browser/certificate_request_result_type.h"
 #include <QtCore/QDateTime>
 #include <QtCore/QScopedPointer>
 #include <QtCore/QUrl>
 #include <QtNetwork/QSslCertificate>
 
-QT_BEGIN_NAMESPACE
+namespace net {
+class SSLInfo;
+}
+class GURL;
 
-class CertificateErrorControllerPrivate;
+QT_BEGIN_NAMESPACE
 
 class Q_WEBENGINECORE_PRIVATE_EXPORT CertificateErrorController {
 public:
-    CertificateErrorController(CertificateErrorControllerPrivate *p);
+    CertificateErrorController(
+            int cert_error, const net::SSLInfo &ssl_info, const GURL &request_url,
+            bool strict_enforcement,
+            base::OnceCallback<void(content::CertificateRequestResultType)> callback);
     ~CertificateErrorController();
 
     // We can't use QSslError::SslErrors, because the error categories doesn't map.
@@ -107,15 +114,17 @@ public:
     void rejectCertificate() { accept(false); }
 
     void deactivate();
-    static void clear(QList<QWeakPointer<CertificateErrorController>> &controllers) {
-        for (auto &&wc : controllers)
-            if (auto controller = wc.lock())
-                controller->deactivate();
-        controllers.clear();
-    }
+
+    CertificateErrorController::CertificateError m_certError;
+    const QUrl m_requestUrl;
+    QDateTime m_validExpiry;
+    bool m_overridable;
+    base::OnceCallback<void(content::CertificateRequestResultType)> m_callback;
+    QList<QSslCertificate> m_certificateChain;
+
+    bool m_answered = false, m_deferred = false;
 
 private:
-    QScopedPointer<CertificateErrorControllerPrivate> d;
     Q_DISABLE_COPY(CertificateErrorController)
 };
 
diff --git a/src/core/certificate_error_controller_p.h b/src/core/certificate_error_controller_p.h
deleted file mode 100644
index f46670cf5..000000000
--- a/src/core/certificate_error_controller_p.h
+++ /dev/null
@@ -1,76 +0,0 @@
-/****************************************************************************
-**
-** Copyright (C) 2016 The Qt Company Ltd.
-** Contact: https://www.qt.io/licensing/
-**
-** This file is part of the QtWebEngine module of the Qt Toolkit.
-**
-** $QT_BEGIN_LICENSE:LGPL$
-** Commercial License Usage
-** Licensees holding valid commercial Qt licenses may use this file in
-** accordance with the commercial license agreement provided with the
-** Software or, alternatively, in accordance with the terms contained in
-** a written agreement between you and The Qt Company. For licensing terms
-** and conditions see https://www.qt.io/terms-conditions. For further
-** information use the contact form at https://www.qt.io/contact-us.
-**
-** GNU Lesser General Public License Usage
-** Alternatively, this file may be used under the terms of the GNU Lesser
-** General Public License version 3 as published by the Free Software
-** Foundation and appearing in the file LICENSE.LGPL3 included in the
-** packaging of this file. Please review the following information to
-** ensure the GNU Lesser General Public License version 3 requirements
-** will be met: https://www.gnu.org/licenses/lgpl-3.0.html.
-**
-** GNU General Public License Usage
-** Alternatively, this file may be used under the terms of the GNU
-** General Public License version 2.0 or (at your option) the GNU General
-** Public license version 3 or any later version approved by the KDE Free
-** Qt Foundation. The licenses are as published by the Free Software
-** Foundation and appearing in the file LICENSE.GPL2 and LICENSE.GPL3
-** included in the packaging of this file. Please review the following
-** information to ensure the GNU General Public License requirements will
-** be met: https://www.gnu.org/licenses/gpl-2.0.html and
-** https://www.gnu.org/licenses/gpl-3.0.html.
-**
-** $QT_END_LICENSE$
-**
-****************************************************************************/
-
-#ifndef CERTIFICATE_ERROR_CONTROLLER_P_H
-#define CERTIFICATE_ERROR_CONTROLLER_P_H
-
-//
-//  W A R N I N G
-//  -------------
-//
-// This file is not part of the Qt API.  It exists purely as an
-// implementation detail.  This header file may change from version to
-// version without notice, or even be removed.
-//
-// We mean it.
-//
-
-#include "content/public/browser/content_browser_client.h"
-
-#include "certificate_error_controller.h"
-
-QT_BEGIN_NAMESPACE
-
-class CertificateErrorControllerPrivate {
-public:
-    CertificateErrorControllerPrivate(int cert_error, const net::SSLInfo& ssl_info, const GURL& request_url, bool strict_enforcement, base::OnceCallback<void(content::CertificateRequestResultType)> callback);
-
-    CertificateErrorController::CertificateError certError;
-    const QUrl requestUrl;
-    QDateTime validExpiry;
-    bool overridable;
-    base::OnceCallback<void(content::CertificateRequestResultType)> callback;
-    QList<QSslCertificate> certificateChain;
-
-    bool answered = false, deferred = false;
-};
-
-QT_END_NAMESPACE
-
-#endif // CERTIFICATE_ERROR_CONTROLLER_P_H
diff --git a/src/core/content_browser_client_qt.cpp b/src/core/content_browser_client_qt.cpp
index c39d28fef..1abf2387e 100644
--- a/src/core/content_browser_client_qt.cpp
+++ b/src/core/content_browser_client_qt.cpp
@@ -112,7 +112,6 @@
 #include "browser_main_parts_qt.h"
 #include "browser_message_filter_qt.h"
 #include "certificate_error_controller.h"
-#include "certificate_error_controller_p.h"
 #include "client_cert_select_controller.h"
 #include "devtools_manager_delegate_qt.h"
 #include "login_delegate_qt.h"
@@ -312,14 +311,8 @@ void ContentBrowserClientQt::AllowCertificateError(content::WebContents *webCont
 {
     WebContentsDelegateQt* contentsDelegate = static_cast<WebContentsDelegateQt*>(webContents->GetDelegate());
 
-    QSharedPointer<CertificateErrorController> errorController(
-            new CertificateErrorController(
-                    new CertificateErrorControllerPrivate(
-                            cert_error,
-                            ssl_info,
-                            request_url,
-                            strict_enforcement,
-                            std::move(callback))));
+    QSharedPointer<CertificateErrorController> errorController(new CertificateErrorController(
+            cert_error, ssl_info, request_url, strict_enforcement, std::move(callback)));
     contentsDelegate->allowCertificateError(errorController);
 }
 
diff --git a/src/core/core_chromium.pri b/src/core/core_chromium.pri
index 46ac5c8d2..09ca86cfc 100644
--- a/src/core/core_chromium.pri
+++ b/src/core/core_chromium.pri
@@ -148,7 +148,6 @@ HEADERS = \
         browsing_data_remover_delegate_qt.h \
         browser_main_parts_qt.h \
         browser_message_filter_qt.h \
-        certificate_error_controller_p.h \
         certificate_error_controller.h \
         client_cert_select_controller.h \
         clipboard_change_observer.h \
diff --git a/src/core/web_contents_adapter_client.h b/src/core/web_contents_adapter_client.h
index 0c4a05a8b..021a86e3e 100644
--- a/src/core/web_contents_adapter_client.h
+++ b/src/core/web_contents_adapter_client.h
@@ -71,6 +71,7 @@ QT_FORWARD_DECLARE_CLASS(QWebEngineRegisterProtocolHandlerRequest)
 QT_FORWARD_DECLARE_CLASS(QWebEngineUrlRequestInfo)
 QT_FORWARD_DECLARE_CLASS(QWebEngineUrlRequestInterceptor)
 QT_FORWARD_DECLARE_CLASS(QWebEngineContextMenuRequest)
+QT_FORWARD_DECLARE_CLASS(QWebEngineCertificateError);
 
 namespace content {
 struct DropData;
@@ -230,7 +231,7 @@ public:
     RenderProcessTerminationStatus renderProcessExitStatus(int);
     virtual void renderProcessTerminated(RenderProcessTerminationStatus terminationStatus, int exitCode) = 0;
     virtual void requestGeometryChange(const QRect &geometry, const QRect &frameGeometry) = 0;
-    virtual void allowCertificateError(const QSharedPointer<CertificateErrorController> &errorController) = 0;
+    virtual void allowCertificateError(const QWebEngineCertificateError &error) = 0;
     virtual void selectClientCert(const QSharedPointer<ClientCertSelectController> &selectController) = 0;
     virtual void updateScrollPosition(const QPointF &position) = 0;
     virtual void updateContentsSize(const QSizeF &size) = 0;
diff --git a/src/core/web_contents_delegate_qt.cpp b/src/core/web_contents_delegate_qt.cpp
index 03916baa0..ceee1d282 100644
--- a/src/core/web_contents_delegate_qt.cpp
+++ b/src/core/web_contents_delegate_qt.cpp
@@ -60,7 +60,7 @@
 #include "web_contents_view_qt.h"
 #include "web_engine_context.h"
 #include "web_engine_settings.h"
-
+#include "certificate_error_controller.h"
 #include "chrome/browser/custom_handlers/protocol_handler_registry_factory.h"
 #include "components/web_cache/browser/web_cache_manager.h"
 #include "content/browser/frame_host/render_frame_host_impl.h"
@@ -336,6 +336,10 @@ void WebContentsDelegateQt::EmitLoadStarted(const QUrl &url, bool isErrorPage)
 {
     if (m_lastLoadProgress >= 0 && m_lastLoadProgress < 100) // already running
         return;
+    for (auto &&wc : m_certificateErrorControllers)
+        if (auto controller = wc.lock())
+            controller->deactivate();
+    m_certificateErrorControllers.clear();
     m_viewClient->loadStarted(url, isErrorPage);
     m_viewClient->updateNavigationActions();
     m_viewClient->loadProgressChanged(0);
@@ -674,9 +678,15 @@ WebContentsDelegateQt::createWindow(std::unique_ptr<content::WebContents> new_co
             toQt(initial_pos), m_initialTargetUrl);
 }
 
-void WebContentsDelegateQt::allowCertificateError(const QSharedPointer<CertificateErrorController> &errorController)
+void WebContentsDelegateQt::allowCertificateError(
+        const QSharedPointer<CertificateErrorController> &controller)
 {
-    m_viewClient->allowCertificateError(errorController);
+    QWebEngineCertificateError error(controller);
+    m_viewClient->allowCertificateError(error);
+    if (!error.isOverridable() || (!error.deferred() && !error.answered()))
+        error.rejectCertificate();
+    else
+        m_certificateErrorControllers.append(controller);
 }
 
 void WebContentsDelegateQt::selectClientCert(const QSharedPointer<ClientCertSelectController> &selectController)
diff --git a/src/core/web_contents_delegate_qt.h b/src/core/web_contents_delegate_qt.h
index 6d15daf47..cecfd1ac4 100644
--- a/src/core/web_contents_delegate_qt.h
+++ b/src/core/web_contents_delegate_qt.h
@@ -52,8 +52,8 @@
 #include "favicon_manager.h"
 #include "find_text_helper.h"
 #include "javascript_dialog_manager_qt.h"
-
 #include <QtCore/qlist.h>
+#include <QWebEngineCertificateError>
 
 QT_FORWARD_DECLARE_CLASS(CertificateErrorController)
 QT_FORWARD_DECLARE_CLASS(ClientCertSelectController)
@@ -235,6 +235,7 @@ private:
     mutable bool m_pendingUrlUpdate = false;
 
     base::WeakPtrFactory<WebContentsDelegateQt> m_weakPtrFactory { this };
+    QList<QWeakPointer<CertificateErrorController>> m_certificateErrorControllers;
 };
 
 } // namespace QtWebEngineCore