Refactor to simply and avoid QObject based QgsSslErrorHandler

nyalldawson · nyalldawson · commit 18f04fb4c3d9 · 2019-01-28T15:21:40.000+11:00
diff --git a/src/app/CMakeLists.txt b/src/app/CMakeLists.txt
@@ -252,7 +252,6 @@ SET (QGIS_APP_MOC_HDRS
   qgsalignrasterdialog.h
   qgsappbrowserproviders.h
   qgsappscreenshots.h
-  qgsappsslerrorhandler.h
   qgsjoindialog.h
   qgsaddtaborgroup.h
   qgsannotationwidget.h
diff --git a/src/app/qgsappsslerrorhandler.h b/src/app/qgsappsslerrorhandler.h
@@ -19,9 +19,8 @@
 
 class QgsAppSslErrorHandler : public QgsSslErrorHandler
 {
-    Q_OBJECT
 
-  public slots:
+  public:
 
     void handleSslErrors( QNetworkReply *reply, const QList<QSslError> &errors ) override;
 
diff --git a/src/core/qgsnetworkaccessmanager.cpp b/src/core/qgsnetworkaccessmanager.cpp
@@ -127,10 +127,7 @@ QgsNetworkAccessManager::QgsNetworkAccessManager( QObject *parent )
 void QgsNetworkAccessManager::setSslErrorHandler( std::unique_ptr<QgsSslErrorHandler> handler )
 {
   Q_ASSERT( sMainNAM == this );
-
   mSslErrorHandler = std::move( handler );
-  connect( this, &QgsNetworkAccessManager::sslErrorsOccurred, mSslErrorHandler.get(), &QgsSslErrorHandler::onSslErrors );
-  connect( mSslErrorHandler.get(), &QgsSslErrorHandler::sslErrorsHandled, this, &QgsNetworkAccessManager::afterSslErrorHandled );
 }
 
 void QgsNetworkAccessManager::insertProxyFactory( QNetworkProxyFactory *factory )
@@ -256,11 +253,13 @@ QNetworkReply *QgsNetworkAccessManager::createRequest( QNetworkAccessManager::Op
   return reply;
 }
 
+#ifndef QT_NO_SSL
 void QgsNetworkAccessManager::unlockAfterSslErrorHandled()
 {
   Q_ASSERT( QThread::currentThread() == QApplication::instance()->thread() );
   mSslErrorWaitCondition.wakeOne();
 }
+#endif
 
 void QgsNetworkAccessManager::abortRequest()
 {
@@ -346,6 +345,12 @@ void QgsNetworkAccessManager::afterSslErrorHandled( QNetworkReply *reply )
     qobject_cast< QgsNetworkAccessManager *>( reply->manager() )->unlockAfterSslErrorHandled(); // safe to call directly - the other thread will be stuck waiting for us
   }
 }
+
+void QgsNetworkAccessManager::handleSslErrors( QNetworkReply *reply, const QList<QSslError> &errors )
+{
+  mSslErrorHandler->handleSslErrors( reply, errors );
+  afterSslErrorHandled( reply );
+}
 #endif
 
 QString QgsNetworkAccessManager::cacheLoadControlName( QNetworkRequest::CacheLoadControl control )
@@ -422,14 +427,17 @@ void QgsNetworkAccessManager::setupDefaultProxyAndCache( Qt::ConnectionType conn
              connectionType );
 
     connect( this, &QgsNetworkAccessManager::requestEncounteredSslErrors, sMainNAM, &QgsNetworkAccessManager::requestEncounteredSslErrors );
-    connect( this, &QgsNetworkAccessManager::sslErrorsOccurred, sMainNAM, &QgsNetworkAccessManager::sslErrorsOccurred );
 #endif
   }
   else
   {
+#ifndef QT_NO_SSL
     setSslErrorHandler( qgis::make_unique< QgsSslErrorHandler >() );
+#endif
   }
-
+#ifndef QT_NO_SSL
+  connect( this, &QgsNetworkAccessManager::sslErrorsOccurred, sMainNAM, &QgsNetworkAccessManager::handleSslErrors );
+#endif
   connect( this, &QNetworkAccessManager::finished, this, &QgsNetworkAccessManager::onReplyFinished );
 
   // check if proxy is enabled
@@ -539,14 +547,8 @@ QgsNetworkRequestParameters::QgsNetworkRequestParameters( QNetworkAccessManager:
 // QgsSslErrorHandler
 //
 
-void QgsSslErrorHandler::onSslErrors( QNetworkReply *reply, const QList<QSslError> &errors )
-{
-  Q_ASSERT( QThread::currentThread() == QApplication::instance()->thread() );
-  handleSslErrors( reply, errors );
-  emit sslErrorsHandled( reply );
-}
-
 void QgsSslErrorHandler::handleSslErrors( QNetworkReply *reply, const QList<QSslError> & )
 {
+  Q_UNUSED( reply );
   QgsDebugMsg( QStringLiteral( "SSL errors occurred accessing URL:\n%1" ).arg( reply->request().url().toString() ) );
 }
diff --git a/src/core/qgsnetworkaccessmanager.h b/src/core/qgsnetworkaccessmanager.h
@@ -162,22 +162,14 @@ class QgsNetworkAccessManager;
  * SSL error handler is set by calling QgsNetworkAccessManager::setSslErrorHandler().
  * By default an instance of the logging-only QgsSslErrorHandler base class is used.
  *
- * \since 3.6
+ * \since QGIS 3.6
  */
-class CORE_EXPORT QgsSslErrorHandler : public QObject
+class CORE_EXPORT QgsSslErrorHandler
 {
-    Q_OBJECT
-
-///@cond PRIVATE
-  public slots:
 
-    /**
-     * Called whenever SSL \a errors are encountered during a network \a reply.
-     */
-    void onSslErrors( QNetworkReply *reply, const QList<QSslError> &errors );
-///@endcond
+  public:
 
-  protected:
+    virtual ~QgsSslErrorHandler() = default;
 
     /**
      * Called whenever SSL \a errors are encountered during a network \a reply.
@@ -191,17 +183,6 @@ class CORE_EXPORT QgsSslErrorHandler : public QObject
      */
     virtual void handleSslErrors( QNetworkReply *reply, const QList<QSslError> &errors );
 
-  signals:
-
-    /**
-     * Emitted when the SSL errors for a \a reply have been handled (i.e. the
-     * subclass' handleSslErrors() implementation has completed).
-     *
-     * It is not necessary for subclasses to emit this signal manually, it will
-     * automatically be emitted at the correct time.
-     */
-    void sslErrorsHandled( QNetworkReply *reply );
-
 };
 #endif
 
@@ -414,16 +395,17 @@ class CORE_EXPORT QgsNetworkAccessManager : public QNetworkAccessManager
     void onReplyDownloadProgress( qint64 bytesReceived, qint64 bytesTotal );
 #ifndef QT_NO_SSL
     void onReplySslErrors( const QList<QSslError> &errors );
-#endif
-
-    void afterSslErrorHandled( QNetworkReply *reply );
 
+    void handleSslErrors( QNetworkReply *reply, const QList<QSslError> &errors );
+#endif
   protected:
     QNetworkReply *createRequest( QNetworkAccessManager::Operation op, const QNetworkRequest &req, QIODevice *outgoingData = nullptr ) override;
 
   private:
+#ifndef QT_NO_SSL
     void unlockAfterSslErrorHandled();
-
+    void afterSslErrorHandled( QNetworkReply *reply );
+#endif
     QList<QNetworkProxyFactory *> mProxyFactories;
     QNetworkProxy mFallbackProxy;
     QStringList mExcludedURLs;
diff --git a/tests/src/core/testqgsnetworkaccessmanager.cpp b/tests/src/core/testqgsnetworkaccessmanager.cpp
@@ -61,9 +61,7 @@ class BackgroundRequest : public QThread
 
 class TestSslErrorHandler : public QgsSslErrorHandler
 {
-    Q_OBJECT
-
-  protected:
+  public:
 
     void handleSslErrors( QNetworkReply *reply, const QList<QSslError> &errors ) override
     {

Original file line number	Diff line number	Diff line change
`@@ -19,9 +19,8 @@`
`19`	`19`
`20`	`20`	`class QgsAppSslErrorHandler : public QgsSslErrorHandler`
`21`	`21`	`{`
`22`		`- Q_OBJECT`
`23`	`22`
`24`		`- public slots:`
	`23`	`+ public:`
`25`	`24`
`26`	`25`	`void handleSslErrors( QNetworkReply *reply, const QList<QSslError> &errors ) override;`
`27`	`26`
Original file line number	Diff line number	Diff line change
`@@ -127,10 +127,7 @@ QgsNetworkAccessManager::QgsNetworkAccessManager( QObject *parent )`
`127`	`127`	`void QgsNetworkAccessManager::setSslErrorHandler( std::unique_ptr<QgsSslErrorHandler> handler )`
`128`	`128`	`{`
`129`	`129`	`Q_ASSERT( sMainNAM == this );`
`130`		`-`
`131`	`130`	`mSslErrorHandler = std::move( handler );`
`132`		`- connect( this, &QgsNetworkAccessManager::sslErrorsOccurred, mSslErrorHandler.get(), &QgsSslErrorHandler::onSslErrors );`
`133`		`- connect( mSslErrorHandler.get(), &QgsSslErrorHandler::sslErrorsHandled, this, &QgsNetworkAccessManager::afterSslErrorHandled );`
`134`	`131`	`}`
`135`	`132`
`136`	`133`	`void QgsNetworkAccessManager::insertProxyFactory( QNetworkProxyFactory *factory )`
`@@ -256,11 +253,13 @@ QNetworkReply *QgsNetworkAccessManager::createRequest( QNetworkAccessManager::Op`
`256`	`253`	`return reply;`
`257`	`254`	`}`
`258`	`255`
	`256`	`+#ifndef QT_NO_SSL`
`259`	`257`	`void QgsNetworkAccessManager::unlockAfterSslErrorHandled()`
`260`	`258`	`{`
`261`	`259`	`Q_ASSERT( QThread::currentThread() == QApplication::instance()->thread() );`
`262`	`260`	`mSslErrorWaitCondition.wakeOne();`
`263`	`261`	`}`
	`262`	`+#endif`
`264`	`263`
`265`	`264`	`void QgsNetworkAccessManager::abortRequest()`
`266`	`265`	`{`
`@@ -346,6 +345,12 @@ void QgsNetworkAccessManager::afterSslErrorHandled( QNetworkReply *reply )`
`346`	`345`	`qobject_cast< QgsNetworkAccessManager *>( reply->manager() )->unlockAfterSslErrorHandled(); // safe to call directly - the other thread will be stuck waiting for us`
`347`	`346`	`}`
`348`	`347`	`}`
	`348`	`+`
	`349`	`+void QgsNetworkAccessManager::handleSslErrors( QNetworkReply *reply, const QList<QSslError> &errors )`
	`350`	`+{`
	`351`	`+ mSslErrorHandler->handleSslErrors( reply, errors );`
	`352`	`+ afterSslErrorHandled( reply );`
	`353`	`+}`
`349`	`354`	`#endif`
`350`	`355`
`351`	`356`	`QString QgsNetworkAccessManager::cacheLoadControlName( QNetworkRequest::CacheLoadControl control )`
`@@ -422,14 +427,17 @@ void QgsNetworkAccessManager::setupDefaultProxyAndCache( Qt::ConnectionType conn`
`422`	`427`	`connectionType );`
`423`	`428`
`424`	`429`	`connect( this, &QgsNetworkAccessManager::requestEncounteredSslErrors, sMainNAM, &QgsNetworkAccessManager::requestEncounteredSslErrors );`
`425`		`- connect( this, &QgsNetworkAccessManager::sslErrorsOccurred, sMainNAM, &QgsNetworkAccessManager::sslErrorsOccurred );`
`426`	`430`	`#endif`
`427`	`431`	`}`
`428`	`432`	`else`
`429`	`433`	`{`
	`434`	`+#ifndef QT_NO_SSL`
`430`	`435`	`setSslErrorHandler( qgis::make_unique< QgsSslErrorHandler >() );`
	`436`	`+#endif`
`431`	`437`	`}`
`432`		`-`
	`438`	`+#ifndef QT_NO_SSL`
	`439`	`+ connect( this, &QgsNetworkAccessManager::sslErrorsOccurred, sMainNAM, &QgsNetworkAccessManager::handleSslErrors );`
	`440`	`+#endif`
`433`	`441`	`connect( this, &QNetworkAccessManager::finished, this, &QgsNetworkAccessManager::onReplyFinished );`
`434`	`442`
`435`	`443`	`// check if proxy is enabled`
`@@ -539,14 +547,8 @@ QgsNetworkRequestParameters::QgsNetworkRequestParameters( QNetworkAccessManager:`
`539`	`547`	`// QgsSslErrorHandler`
`540`	`548`	`//`
`541`	`549`
`542`		`-void QgsSslErrorHandler::onSslErrors( QNetworkReply *reply, const QList<QSslError> &errors )`
`543`		`-{`
`544`		`- Q_ASSERT( QThread::currentThread() == QApplication::instance()->thread() );`
`545`		`- handleSslErrors( reply, errors );`
`546`		`- emit sslErrorsHandled( reply );`
`547`		`-}`
`548`		`-`
`549`	`550`	`void QgsSslErrorHandler::handleSslErrors( QNetworkReply *reply, const QList<QSslError> & )`
`550`	`551`	`{`
	`552`	`+ Q_UNUSED( reply );`
`551`	`553`	`QgsDebugMsg( QStringLiteral( "SSL errors occurred accessing URL:\n%1" ).arg( reply->request().url().toString() ) );`
`552`	`554`	`}`
Original file line number	Diff line number	Diff line change
`@@ -61,9 +61,7 @@ class BackgroundRequest : public QThread`
`61`	`61`
`62`	`62`	`class TestSslErrorHandler : public QgsSslErrorHandler`
`63`	`63`	`{`
`64`		`- Q_OBJECT`
`65`		`-`
`66`		`- protected:`
	`64`	`+ public:`
`67`	`65`
`68`	`66`	`void handleSslErrors( QNetworkReply *reply, const QList<QSslError> &errors ) override`
`69`	`67`	`{`